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

stagent.dev: Clicking email field on sign up form does not allow input until you click a second time #4950

Merged
merged 1 commit into from Oct 6, 2022

Conversation

pxlcoder
Copy link
Member

@pxlcoder pxlcoder commented Oct 3, 2022

f3b946f

stagent.dev: Clicking email field on sign up form does not allow input until you click a second time
https://bugs.webkit.org/show_bug.cgi?id=245976
rdar://98341809

Reviewed by Ryosuke Niwa.

Safari's AutoFill heuristic recently began (correctly) detecting email fields
on stagent.dev as autofillable. Consequently, Safari now adds an autofill
button to their email field when it is clicked.

WebKit has a longstanding bug where the selection is lost when an input
decoration is added while the selection is inside the input. Adding a decoration,
such as the autofill button, changes the structure of the shadow subtree.
Specifically, the contenteditable element is removed from the shadow root and
added to a separate container. The removal of the contenteditable element wipes
out the selection. To work around this issue, Safari has logic to restore the
selection after adding an autofill button.

However, Safari's workaround is not robust, as the HTML spec limits which input
types support the input selection APIs, such as `setSelectionStart` and
`setSelectionEnd`. Email inputs do not support the selection API, hence Safari's
workaround fails to apply to email fields where an autofill button is added.

To fix, restore the selection in WebKit, following the addition of an input
decoration, such as the autofill button. Note that this change still results in
two "selectionchange" events getting dispatched when focusing an autofillable
field for the first time. However, this matches behavior in shipping Safari.

An alternate solution considered was to avoid moving the contenteditable element
when adding a decoration. However, this change would have a much larger surface
area, and is too risky in the short term.

* LayoutTests/fast/forms/auto-fill-button/show-auto-fill-button-while-selection-inside-field-expected.txt: Added.
* LayoutTests/fast/forms/auto-fill-button/show-auto-fill-button-while-selection-inside-field.html: Added.
* Source/WebCore/html/HTMLTextFormControlElement.h:

Expose a method returning the enumerated value of the selection direction so
that the variant of `setSelectionRange` that does not dispatch "select" events
can be used.

* Source/WebCore/html/TextFieldInputType.cpp:
(WebCore::TextFieldInputType::createContainer):

Restore the selection after an input decoration is added.

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

c221a57

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch ❌ πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@pxlcoder pxlcoder self-assigned this Oct 3, 2022
@pxlcoder pxlcoder added Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) Other labels Oct 3, 2022
Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 4, 2022
@pxlcoder pxlcoder removed the merging-blocked Applied to prevent a change from being merged label Oct 4, 2022
@pxlcoder
Copy link
Member Author

pxlcoder commented Oct 4, 2022

Thanks for the review!

Updated the test to work on iOS.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 5, 2022
@pxlcoder pxlcoder removed the merging-blocked Applied to prevent a change from being merged label Oct 5, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 6, 2022
@pxlcoder pxlcoder added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Oct 6, 2022
…t until you click a second time

https://bugs.webkit.org/show_bug.cgi?id=245976
rdar://98341809

Reviewed by Ryosuke Niwa.

Safari's AutoFill heuristic recently began (correctly) detecting email fields
on stagent.dev as autofillable. Consequently, Safari now adds an autofill
button to their email field when it is clicked.

WebKit has a longstanding bug where the selection is lost when an input
decoration is added while the selection is inside the input. Adding a decoration,
such as the autofill button, changes the structure of the shadow subtree.
Specifically, the contenteditable element is removed from the shadow root and
added to a separate container. The removal of the contenteditable element wipes
out the selection. To work around this issue, Safari has logic to restore the
selection after adding an autofill button.

However, Safari's workaround is not robust, as the HTML spec limits which input
types support the input selection APIs, such as `setSelectionStart` and
`setSelectionEnd`. Email inputs do not support the selection API, hence Safari's
workaround fails to apply to email fields where an autofill button is added.

To fix, restore the selection in WebKit, following the addition of an input
decoration, such as the autofill button. Note that this change still results in
two "selectionchange" events getting dispatched when focusing an autofillable
field for the first time. However, this matches behavior in shipping Safari.

An alternate solution considered was to avoid moving the contenteditable element
when adding a decoration. However, this change would have a much larger surface
area, and is too risky in the short term.

* LayoutTests/fast/forms/auto-fill-button/show-auto-fill-button-while-selection-inside-field-expected.txt: Added.
* LayoutTests/fast/forms/auto-fill-button/show-auto-fill-button-while-selection-inside-field.html: Added.
* Source/WebCore/html/HTMLTextFormControlElement.h:

Expose a method returning the enumerated value of the selection direction so
that the variant of `setSelectionRange` that does not dispatch "select" events
can be used.

* Source/WebCore/html/TextFieldInputType.cpp:
(WebCore::TextFieldInputType::createContainer):

Restore the selection after an input decoration is added.

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

Committed 255229@main (f3b946f): https://commits.webkit.org/255229@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit f3b946f into WebKit:main Oct 6, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 6, 2022
@pxlcoder pxlcoder deleted the eng/245976 branch November 8, 2022 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.)
Projects
None yet
5 participants