Simplify SelectionChangeObserver update logic #1095
Conversation
This removes the fallback to polling for selection changes. The selectionchange API has ben supported by all browsers that Trix supports for a long time now. The last browsers to add support for it was Firefox in version 52, and Chrome for Android, both in 2017.
This checks causes problems on Safari 17 when you drag the cursor to select text. Safari weirdly reports the selection range as being collapsed, even when you select multiple characters. This causes the SelectionManager to think that the selection is collapsed and not trying to find where it ends. The end result for the user is that they can not apply formatting or links to the whole selected text. This is ultimately a Safari bug, but it's not clear that we need to check for DOM range equality in the first place. The only reason we do this is because the original implementation of the SelectionChangeObserver used polling to check for selection changes, instead of relying on the `selectionchange` event. Now that we're using the `selectionchange` event, we can assume that if a selection change event fires, the selection has changed.
There was a problem hiding this comment.
This looks great! Definitely much simpler, and solves the problem.
As discussed, the broken selection behaviour isn't a Safari bug after all. It's really a Trix bug that happens to surface there.
The problem stems from the fact that getRangeAt returns a reference to the selection range. The selectionchange event fires multiple times while a selection is being made, however when we try to compare the new range to the previous range, we're actually comparing references to the same underlying object. As a result, the code acts as if the selection hasn't changed, and ignores the event.
The effect of this is that we only process the first selectionchange event, which corresponds to the initial collapsed selection. We don't process those later events that actually have the full selected range in them.
|
👍 for reference, this is the part of the spec that specifies that the range is a reference, not a copy. The question now is how this works in other browsers. But, in any case, the fix is the same. |
This introduces some changes to simplify
SelectionChangeObserver. The main motivation for the changes is to fix some bugs that came up in iOS 17, but the end result should be simpler logic too.Firstly, this removes the fallback to polling for selection changes when
selectionchangeevent is not available. TheselectionchangeAPI has been available in all browsers for a long time now. The last browsers to add support for it were Firefox in version 52, and Chrome for Android, both in 2017.Secondly, it removes a condition to check if the selected for DOM range has changed since the last time the
SelectionChangeObserverwas called. This check causes problems on Safari 17 when you drag the cursor to select text. Safari weirdly reports the selection range as being collapsed, even when you select multiple characters. This, in turn, causes theSelectionManagerto think that the selection is collapsed and not trying to find where it ends. The end result for the user is that they can not apply formatting or links to the whole selected text.This is ultimately a Safari bug, but it's not clear that we need to check for DOM range equality in the first place. We only do this because the original implementation of the
SelectionChangeObserverused polling to check for selection changes, instead of relying on theselectionchangeevent. Now that we're using theselectionchangeevent, we can assume that if a selection change event fires, the selection has changed.