ELEMENTS-1372: fix selection on Safari #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gabez0r
force-pushed
the
fix-ELEMENTS-1372-selection
branch
3 times, most recently
from
8e94dc8 to
aef33db
Compare
mnixo
approved these changes
Jun 1, 2021
nelsonsilva
approved these changes
Jun 2, 2021
| if (!this.hasFocus()) return; | ||
| const native = this.getNativeRange(); | ||
| // when might need to hack the offset on Safari, when we are dealing with the first character of a row | ||
| const hackOffset = (native.start.offset === 0 && |
There was a problem hiding this comment.
shouldn't we fix the offset in the getNativeRange() function instead where we're already "fixing" it ?
There was a problem hiding this comment.
I though about that, but then getNativeRange is also used by getRange, setRange and format, which are then used in other places, so it has a greater potential to introducing side effects...
| @@ -4,7 +4,7 @@ const SUPPORTS_BEFORE_INPUT = typeof window.InputEvent.prototype.getTargetRanges | |||
| const IS_FIREFOX = window.navigator.userAgent.toLowerCase().indexOf('firefox') > -1; | |||
There was a problem hiding this comment.
Do we need to handle FF differently ?
There was a problem hiding this comment.
Differently than Safari? Yes, it does not need to use the ShadowSelection class. It can do everything using document.getSelection().
Gabez0r
force-pushed
the
fix-ELEMENTS-1372-selection
branch
from
aef33db to
6a0fde6
Compare
richardsd
approved these changes
Jun 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here we're replacing the broken shadow-selection-polyfill with an alternate polyfill. However, there are some issues with the polyfill that prevent it from correctly working OOTB with Quill:
rangeCountproperty, a member of the public Selection APIgetSelection, you never know if you are not getting an outdated version of the selection or not, because the event might not have been fired yet.addRangedoesn't actually set the range on the contenteditable elementProblem 1) is quite easy to fix, but just implementing the missing getter.
Problem 2) is quite hard to get around without proper support from Safari, or without fully controlling the component (quill in this case). Quill runs an
updatewhenever there is a DOM mutation. In thisupdate, it will read the selection and, based on it, explicitly set the selection in some situations, using theaddRange(problem 3). Problem 3 can be worked around by explicitly setting the selection on the Window, but only if this is not being called from the polyfill, or else it can generate a loop.Problem 2) remains unaddressed. Luckily, it only seems to happen when filling the first character of a row, probably related to how quill replaces the
brelement with a text node. Increasing the selection offset by 1 in this particular case seems to do the trick, but I am not sure if this will always work.