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

REGRESSION (259904@main): @math operation cannot trigger in Quip app #16492

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

whsieh
Copy link
Member

@whsieh whsieh commented Aug 8, 2023

457eb42

REGRESSION (259904@main): @math operation cannot trigger in Quip app
https://bugs.webkit.org/show_bug.cgi?id=259944
rdar://113229516

Reviewed by Ryosuke Niwa.

Currently, the `@math` feature in Quip is broken in Safari with Live Range Selection enabled; this
is because Quip's JavaScript editor focuses an input element, and expects `selection.rangeCount` to
return `1` instead of `0` (the latter of which causes Quip to believe that the user is no longer
editing the text field, and therefore leads to Quip dismissing the input field immediately after
presenting it).

This happens because the `rangeCount` and ranges we expose on `DOMSelection` are different with live
range selection in Safari 17, as opposed to both Safari 16 and Chrome (testing against both stable
and canary). Whereas we used to expose a single range that was collapsed to the start of the text
field, we now expose no ranges at all.

To avoid this incompatibility, we restore shipping behavior when the selection is inside of shadow
roots, by exposing a selection range that's equivalent to a caret before the shadow host.

See also: <w3c/selection-api#166>.

Test: fast/forms/shadow-tree-exposure-live-range.html

* LayoutTests/fast/forms/shadow-tree-exposure-live-range-expected.txt:
* LayoutTests/fast/forms/shadow-tree-exposure-live-range.html:

Rebaseline this layout test, such that it verifies that the ranges exposed via the Selection API
have the same behavior without live ranges, vs. with live ranges enabled; this also matches the
behavior in latest Chrome Canary.

* LayoutTests/imported/w3c/web-platform-tests/editing/other/editable-state-and-focus-in-shadow-dom-in-designMode.tentative-expected.txt:
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/editing/other/editable-state-and-focus-in-shadow-dom-in-designMode.tentative-expected.txt:
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/editing/other/editable-state-and-focus-in-shadow-dom-in-designMode.tentative-expected.txt:

Rebaseline a WPT — attempts to change the selection in a UA shadow root in this test now result in
the wrong base/anchor nodes and offsets after this patch, rather than the `IndexSizeError`s we
currently get.

* Source/WebCore/page/DOMSelection.cpp:
(WebCore::selectionShadowAncestor):

Remove an assertion that's no longer relevant, now that we use this in both live/legacy codepaths.

(WebCore::DOMSelection::type const):

Keep both `fast/forms/shadow-tree-exposure-live-range.html` and
`editing/selection/user-select-js-property.html` passing by adjusting our logic for returning the
selection type. Currently, a selection in a shadow root always results in a type of `None`, but this
diverges from Chrome (and Safari 16's) behavior, which returns the actual type of the selected
range, regardless of whether it's in the shadow tree.

(WebCore::DOMSelection::rangeCount const):

This is the main fix — return a `rangeCount` of 1 in the case where there are no associated live
ranges, if the range is still inside a shadow root.

(WebCore::createLiveRangeBeforeShadowHostWithSelection):
(WebCore::DOMSelection::getRangeAt):

Keep `getRangeAt` consistent with `rangeCount` by returning the caret range at the start of the
shadow host in the case where the selected range is in the shadow tree.

(WebCore::DOMSelection::shadowAdjustedNode const):
(WebCore::DOMSelection::shadowAdjustedOffset const):

These methods need to be adjusted as well, to keep the `(base|anchor|focus|extent)(Node|Offset)`
properties consistent with the `rangeCount` and selection ranges in the case where the selection is
inside of a shadow root. Since the behavior with or without live ranges when the selection is in a
shadow tree is now consistent, we no longer require live-range-specific codepaths, so we can
simplify this code a bit by just removing the `liveRangeSelectionEnabled()` checks.

Canonical link: https://commits.webkit.org/266781@main

9c520b5

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh self-assigned this Aug 8, 2023
@whsieh whsieh added the HTML Editing For bugs in HTML editing support (including designMode and contentEditable). label Aug 8, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 8, 2023
@whsieh
Copy link
Member Author

whsieh commented Aug 8, 2023

(Whoops, I need to guard the LoA behind a PLATFORM(COCOA) to fix the build).

I'm also going to try and come up with a reduction so that we can better understand (exactly) what behavior Quip is relying on...

(edit: after some more testing, it seems the @math widget in Quip is also broken in Safari. We might need a site quirk to fix that in the short term, as well)

@whsieh
Copy link
Member Author

whsieh commented Aug 9, 2023

As it turns out, Quip was depending on the fact that DOMSelection.rangeCount returned 1 in the case where the selection is in a focused text field. The minimal repro is:

<input />
<script>
document.querySelector("input").focus();
document.write(`rangeCount := ${getSelection().rangeCount}`);
</script>

...though, the above test case behaves different in different browsers:

  1. Chrome: rangeCount := 1
  2. Firefox: rangeCount := 0
  3. Safari (with live ranges disabled): rangeCount := 1
  4. Safari (with live ranges enabled): rangeCount := 0

So we now match Gecko's behavior on Safari 17, whereas we used to match Chrome.

@rniwa
Copy link
Member

rniwa commented Aug 9, 2023

That is very subtle!

@whsieh
Copy link
Member Author

whsieh commented Aug 9, 2023


So we now match Gecko's behavior on Safari 17, whereas we used to match Chrome.

After some more testing: it seems the bug does not reproduce in Firefox. This is because FF doesn't actually mutate the DOM selection when focusing a text field, so if the rangeCount in Quip were 1 before (as is the case when typing @math, then it will be 1 after as well — thus, avoiding the bug).

As such, a better test case is:

<input />
<script>
getSelection().setPosition(document.body);
document.querySelector("input").focus();
document.write(`rangeCount := ${getSelection().rangeCount}`);
</script>

...this yields 0 in Safari 17 with live ranges enabled, and 1 everywhere else.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 10, 2023
@whsieh whsieh removed the merging-blocked Applied to prevent a change from being merged label Aug 10, 2023
@rniwa
Copy link
Member

rniwa commented Aug 10, 2023

By the way, can we add a test with nested shadow roots as a follow up?

@whsieh
Copy link
Member Author

whsieh commented Aug 10, 2023

By the way, can we add a test with nested shadow roots as a follow up?

Sounds good — will add a test in https://bugs.webkit.org/show_bug.cgi?id=260011.

Thanks for the review!

@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Aug 10, 2023
https://bugs.webkit.org/show_bug.cgi?id=259944
rdar://113229516

Reviewed by Ryosuke Niwa.

Currently, the `@math` feature in Quip is broken in Safari with Live Range Selection enabled; this
is because Quip's JavaScript editor focuses an input element, and expects `selection.rangeCount` to
return `1` instead of `0` (the latter of which causes Quip to believe that the user is no longer
editing the text field, and therefore leads to Quip dismissing the input field immediately after
presenting it).

This happens because the `rangeCount` and ranges we expose on `DOMSelection` are different with live
range selection in Safari 17, as opposed to both Safari 16 and Chrome (testing against both stable
and canary). Whereas we used to expose a single range that was collapsed to the start of the text
field, we now expose no ranges at all.

To avoid this incompatibility, we restore shipping behavior when the selection is inside of shadow
roots, by exposing a selection range that's equivalent to a caret before the shadow host.

See also: <w3c/selection-api#166>.

Test: fast/forms/shadow-tree-exposure-live-range.html

* LayoutTests/fast/forms/shadow-tree-exposure-live-range-expected.txt:
* LayoutTests/fast/forms/shadow-tree-exposure-live-range.html:

Rebaseline this layout test, such that it verifies that the ranges exposed via the Selection API
have the same behavior without live ranges, vs. with live ranges enabled; this also matches the
behavior in latest Chrome Canary.

* LayoutTests/imported/w3c/web-platform-tests/editing/other/editable-state-and-focus-in-shadow-dom-in-designMode.tentative-expected.txt:
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/editing/other/editable-state-and-focus-in-shadow-dom-in-designMode.tentative-expected.txt:
* LayoutTests/platform/ios/imported/w3c/web-platform-tests/editing/other/editable-state-and-focus-in-shadow-dom-in-designMode.tentative-expected.txt:

Rebaseline a WPT — attempts to change the selection in a UA shadow root in this test now result in
the wrong base/anchor nodes and offsets after this patch, rather than the `IndexSizeError`s we
currently get.

* Source/WebCore/page/DOMSelection.cpp:
(WebCore::selectionShadowAncestor):

Remove an assertion that's no longer relevant, now that we use this in both live/legacy codepaths.

(WebCore::DOMSelection::type const):

Keep both `fast/forms/shadow-tree-exposure-live-range.html` and
`editing/selection/user-select-js-property.html` passing by adjusting our logic for returning the
selection type. Currently, a selection in a shadow root always results in a type of `None`, but this
diverges from Chrome (and Safari 16's) behavior, which returns the actual type of the selected
range, regardless of whether it's in the shadow tree.

(WebCore::DOMSelection::rangeCount const):

This is the main fix — return a `rangeCount` of 1 in the case where there are no associated live
ranges, if the range is still inside a shadow root.

(WebCore::createLiveRangeBeforeShadowHostWithSelection):
(WebCore::DOMSelection::getRangeAt):

Keep `getRangeAt` consistent with `rangeCount` by returning the caret range at the start of the
shadow host in the case where the selected range is in the shadow tree.

(WebCore::DOMSelection::shadowAdjustedNode const):
(WebCore::DOMSelection::shadowAdjustedOffset const):

These methods need to be adjusted as well, to keep the `(base|anchor|focus|extent)(Node|Offset)`
properties consistent with the `rangeCount` and selection ranges in the case where the selection is
inside of a shadow root. Since the behavior with or without live ranges when the selection is in a
shadow tree is now consistent, we no longer require live-range-specific codepaths, so we can
simplify this code a bit by just removing the `liveRangeSelectionEnabled()` checks.

Canonical link: https://commits.webkit.org/266781@main
@webkit-commit-queue
Copy link
Collaborator

Committed 266781@main (457eb42): https://commits.webkit.org/266781@main

Reviewed commits have been landed. Closing PR #16492 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 457eb42 into WebKit:main Aug 10, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 10, 2023
@whsieh whsieh deleted the eng/259944 branch August 10, 2023 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTML Editing For bugs in HTML editing support (including designMode and contentEditable).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants