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

Add the autocorrect attribute #5841

Merged
merged 10 commits into from
Aug 27, 2024
Merged

Add the autocorrect attribute #5841

merged 10 commits into from
Aug 27, 2024

Conversation

whsieh
Copy link
Contributor

@whsieh whsieh commented Aug 18, 2020

@othermaciej
Copy link

@whsieh to pass the participation check, you need to make your membership in https://github.com/apple-whatwg publicly visible.

@whsieh
Copy link
Contributor Author

whsieh commented Aug 18, 2020

@whsieh to pass the participation check, you need to make your membership in https://github.com/apple-whatwg publicly visible.

Done, thanks!

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

I think it would be good if @kojiishi and @dtapuska took a look as well to see if it matches their expectations.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@mtrootyy
Copy link

mtrootyy commented Aug 19, 2020

  1. The following sentence should be added.

User agents can support autocorrection of editable text, either in form controls (such as the value of textarea elements), or in elements in an editing host (e.g. using contenteditable).

Because it is difficult that all user agents support autocorrection of all natural language.
There are similar sentence in the section of "Spelling and grammar checking". (https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-checking)

  1. The following sentence should be added.

The autocorrect attribute never causes autocorrection to be enabled for input elements whose type attribute is in one of the URL, E-mail, or Password states.

Because autocorrection interfere with input or editing on input elements whose type attribute is in one of the URL, E-mail, or Password states.
Correct spelling as a natural language is not required on input elements whose type attribute is in one of the URL, E-mail, or Password states.
There are similar sentence in the section of "Autocapitalization". (https://html.spec.whatwg.org/multipage/interaction.html#autocapitalization:attr-autocapitalize-4)

@whsieh
Copy link
Contributor Author

whsieh commented Aug 19, 2020

Thank you for the comments, @annevk and @mtrootyy!

I've updated the pull request accordingly, and will add a new subtest to the accompanying web-platform-tests PR that will cover autocorrection in password, email and URL fields.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Looking good; thanks so much for working on this!

Most comments are editorial, but there's one potentially substantial issue in the processing model, related to presence/absence of the attribute vs. its on/off state.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic

This comment was marked as resolved.

@whsieh

This comment was marked as resolved.

@xfq xfq added the i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. label Oct 27, 2020
Base automatically changed from master to main January 15, 2021 07:58
@domenic

This comment was marked as resolved.

@fschroiff

This comment was marked as resolved.

@tomlube

This comment was marked as resolved.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I have tried to take care of the remaining comments. I left two questions, one for @whsieh and one for @domenic. I haven't looked at whether the tests deal with the autocapitalize-and-autocorrect-inheriting elements.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@r12a
Copy link

r12a commented Oct 11, 2022

Should autocorrection be language-dependent? How does the browser know which dictionary to use to look up words, and does it alter its triggers for lookup for a script that doesn't use spaces, such as Chinese or Japanese or Thai (the latter has spaces but for phrase separation, rather than word), or for scripts that use different punctuation marks than English? Is it able to tell which language a user is using when they fill in form fields if that user fills in forms in more than one language?

@annevk
Copy link
Member

annevk commented Oct 11, 2022

Much like with autocapitalization, this does not attempt to define how autocorrection works. It merely acknowledges that it's a feature of input methods such as virtual keyboards that at times is useful to disable.

@annevk annevk requested a review from domenic October 11, 2022 16:43
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thanks for picking this back up! What's the reason for not using the same behavior of inheriting from the editing host as autocapitalize does?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Oct 12, 2022

What's the reason for not using the same behavior of inheriting from the editing host as autocapitalize does?

I'm not sure, but note that autocapitalize doesn't reflect this in its getter. It's non-script-observable behavior. So in that sense we could always change this later if it turns out to be a problem. (The autocapitalize getter is worse in that it does not acknowledge URL, Email, and Password controls either. That might be something we want to try to fix, but that getter is even older so it might be high risk for little reward.)

@whsieh thoughts?

@smaug----
Copy link

How would a webpage know whether autocorrection is supported by the UA or not? Site might provide its own correction as a fallback.
I could imagine that on less common OSes and especially when using less common languages autocorrection isn't always available.
I guess there are similar issues with spellchecking too though.

@annevk
Copy link
Member

annevk commented Oct 13, 2022

Yeah that would apply to spellchecking and autocapitalization as well. I could see two options:

  1. Don't expose these features. (As long as you can fingerprint the OS this should not add to the fingerprinting surface.)
  2. Add some kind of supports() thing. (Per rationale above should also not add to the fingerprinting surface.)

I'm not sure we should do either until it becomes a concrete problem for someone though as there's also a question to what extent web developers would care about less common OSes.

@annevk annevk added the agenda+ To be discussed at a triage meeting label Aug 14, 2024
@annevk
Copy link
Member

annevk commented Aug 14, 2024

Thanks for the nudge @danielhjacobs. This clearly got lost. I rebased this (again) and added commits to account for recent HTML refactoring efforts. At least as far as the bots are concerned this is now in a state where it can be merged, but I'll double check in the next WHATNOT call I can attend.

@annevk annevk changed the title Add the autocorrect attribute to the HTML spec. Add the autocorrect attribute Aug 14, 2024
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@past past removed the agenda+ To be discussed at a triage meeting label Aug 15, 2024
@annevk
Copy link
Member

annevk commented Aug 19, 2024

@smaug---- @masayuki-nakano I think this is ready now, but happy to wait a bit so you can take a look as well before it lands.

annevk added a commit to annevk/WebKit that referenced this pull request Aug 20, 2024
https://bugs.webkit.org/show_bug.cgi?id=246344
rdar://101036922

Reviewed by NOBODY (OOPS!).

Align with the standardized version of the autocorrect attribute, which
does not support Email, URL, and Password fields and does not treat the
empty string value in a special way. For details see
whatwg/html#5841

Test is upstreamed at
web-platform-tests/wpt#25073

* Source/WebCore/html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::shouldAutocorrect const):
<var>element</var>'s <span>form owner</span>'s <code data-x="attr-autocorrect">autocorrect</code>
attribute.</p></li>

<li><p>Return <span data-x="concept-autocorrection-on">on</span>.</p></li>

Choose a reason for hiding this comment

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

As long as looking the implementations of both Blink and WebKit, it is on by default.

Someone may want on-by-default like spellcheck attribute, but it is OK for me.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Aug 27, 2024
For whatwg/html#5841.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@annevk annevk merged commit 7bab05a into whatwg:main Aug 27, 2024
2 checks passed
annevk added a commit to annevk/WebKit that referenced this pull request Aug 27, 2024
https://bugs.webkit.org/show_bug.cgi?id=246344
rdar://101036922

Reviewed by NOBODY (OOPS!).

Align with the standardized version of the autocorrect attribute, which
does not support Email, URL, and Password fields and does not treat the
empty string value in a special way. For details see
whatwg/html#5841

Test is upstreamed at
web-platform-tests/wpt@2767f5f

* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/w3c-import.log: Added.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection-expected.txt: Added.
* Source/WebCore/html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::shouldAutocorrect const):
annevk added a commit to annevk/WebKit that referenced this pull request Aug 27, 2024
https://bugs.webkit.org/show_bug.cgi?id=246344
rdar://101036922

Reviewed by NOBODY (OOPS!).

Align with the standardized version of the autocorrect attribute, which
does not support Email, URL, and Password fields and does not treat the
empty string value in a special way. For details see
whatwg/html#5841

Test is upstreamed at
web-platform-tests/wpt@2767f5f

* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/w3c-import.log: Added.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection-expected.txt: Added.
* Source/WebCore/html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::shouldAutocorrect const):
webkit-commit-queue pushed a commit to annevk/WebKit that referenced this pull request Aug 27, 2024
https://bugs.webkit.org/show_bug.cgi?id=246344
rdar://101036922

Reviewed by Wenson Hsieh.

Align with the standardized version of the autocorrect attribute, which
does not support Email, URL, and Password fields and does not treat the
empty string value in a special way. For details see
whatwg/html#5841

Test is upstreamed at
web-platform-tests/wpt@2767f5f

* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/w3c-import.log: Added.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/editing/editing-0/autocorrection/autocorrection-expected.txt: Added.
* Source/WebCore/html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::shouldAutocorrect const):

Canonical link: https://commits.webkit.org/282792@main
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 28, 2024
…, a=testonly

Automatic update from web-platform-tests
Add tests for the autocorrect attribute

For whatwg/html#5841.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
--

wpt-commits: 2767f5f395242e24ed3d7c29eed464c99b06514e
wpt-pr: 25073
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Aug 29, 2024
…, a=testonly

Automatic update from web-platform-tests
Add tests for the autocorrect attribute

For whatwg/html#5841.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
--

wpt-commits: 2767f5f395242e24ed3d7c29eed464c99b06514e
wpt-pr: 25073
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Aug 29, 2024
…, a=testonly

Automatic update from web-platform-tests
Add tests for the autocorrect attribute

For whatwg/html#5841.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
--

wpt-commits: 2767f5f395242e24ed3d7c29eed464c99b06514e
wpt-pr: 25073
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. topic: editing
Development

Successfully merging this pull request may close these issues.