Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed the unused getClientRectsPonyfill #33

Conversation

oleksandr-danylchenko
Copy link
Contributor

Changes Made

I noticed that since September the Firefox-specific handling for obtaining client rects has been deactivated. And if since then there were no issues w/ Firefox - I think that we can remove the unused ponyfill

@oleksandr-danylchenko oleksandr-danylchenko marked this pull request as ready for review February 12, 2024 12:29
@oleksandr-danylchenko oleksandr-danylchenko changed the title Removed unused getClientRectsPonyfill Removed the unused getClientRectsPonyfill Feb 12, 2024
@rsimon
Copy link
Member

rsimon commented Feb 18, 2024

I'll leave this open for now. Need to revisit. The problem is that Firefox deals with highlight computation differently (in a bad way) as soon as block-level elements are involved. Leading to the following difference:

Chrome (= the behavior we want)

Bildschirmfoto 2024-02-18 um 09 34 21

Firefox (bad)

Bildschirmfoto 2024-02-18 um 09 36 09

The ponyfill temporarily modifies the DOM, inserts SPANs, and then measures highlights from the SPANs rather than the Ranges. This is a pretty heavy computation, and occasionally introduces unwanted side effects on the Range object. Because of this, I currently disabled it. But it remains an open issue.

@oleksandr-danylchenko
Copy link
Contributor Author

That's crazy 🤯
Thanks for the clarification! Didn't know about such a weird Firefox behavior before

@oleksandr-danylchenko oleksandr-danylchenko marked this pull request as draft February 21, 2024 12:39
# Conflicts:
#	packages/text-annotator/src/state/spatialTree.ts
#	packages/text-annotator/src/utils/index.ts
@oleksandr-danylchenko
Copy link
Contributor Author

Seems like the Ponyfill introduces its own set of weird selection behaviors 😅
#74 (comment)

@oleksandr-danylchenko oleksandr-danylchenko marked this pull request as ready for review March 5, 2024 17:11
@rsimon
Copy link
Member

rsimon commented Sep 3, 2024

It's safe to say that we won't be using the ponyfill anymore. I removed it manually (rather than resolving the conflicts that have come up recently.)

@rsimon rsimon closed this Sep 3, 2024
@oleksandr-danylchenko oleksandr-danylchenko deleted the removeFirefoxPonyfill branch September 3, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants