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 a new attribute called writingsuggestions to control UA-provided writing assistance #10018

Merged
merged 40 commits into from
Mar 13, 2024

Conversation

sanketj
Copy link
Member

@sanketj sanketj commented Dec 22, 2023

This PR proposes the addition of a new attribute called writingsuggestions to control UA-provided writing assistance, addressing #9065. Some browsers provide this capability to users (see https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/WritingSuggestions/explainer.md#use-case) but there are scenarios in which developers may want to turn off browser provided writing assistance, such as writing extensions like Grammarly or sites providing their own solutions. This new attribute will have values "on" and "off", and each element will have a default behavior as determined by the UA. This attribute's state is also inheritable from ancestor elements.


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Mar 11, 2024, 5:56 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

      <!DOCTYPE html>
      <html>
      <head>
          <meta name="viewport" content="width=device-width, initial-scale=1">
          <meta name="robots" content="noindex">
          <style>body,html{height:100%;margin:0}body{display:flex;align-items:center;justify-content:center;flex-direction:column;-webkit-font-smoothing:antialiased;text-rendering:optimizeLegibility}p{text-align:center;font-family:-apple-system,BlinkMacSystemFont,Segoe UI,Roboto,Oxygen,Ubuntu,Cantarell,Fira Sans,Droid Sans,Helvetica Neue,sans-serif;color:#000;font-size:14px;margin-top:-50px}p.code{font-size:24px;font-weight:500;border-bottom:1px solid #e0e1e2;padding:0 20px 15px}p.text{margin:0}a,a:visited{color:#aaa}</style>
      </head>
      <body>
      <p class="code">
        Error
      </p>
      <p class="text">
        We encountered an error when trying to load your application and your page could not be served. Check the logs for your application in the App Platform dashboard.
      </p>
        <div style="display:none;">
          <h1>
    upstream_reset_before_response_started{connection_termination} (503 UC)      </h1>
          <p data-translate="connection_timed_out">App Platform failed to forward this request to the application.</p>
      </div>
      </body>
      </html>
    

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member Author

@sanketj sanketj 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 the review. I've addressed/responded to all the comments so far.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
webkit-commit-queue pushed a commit to rr-codes/WebKit that referenced this pull request Feb 17, 2024
https://bugs.webkit.org/show_bug.cgi?id=266824
rdar://114989563

Reviewed by Aditya Keerthi.

Implement support for the new DOM `writingsuggstions` web API attribute, as defined by the
corresponding spec PR (whatwg/html#10018).

* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WTF/wtf/PlatformEnable.h:
* Source/WebCore/dom/Element.cpp:
(WebCore::Element::isWritingSuggestionsEnabled const):
* Source/WebCore/dom/Element.h:
* Source/WebCore/editing/VisibleSelection.cpp:
(WebCore::VisibleSelection::canEnableWritingSuggestions const):
* Source/WebCore/editing/VisibleSelection.h:
* Source/WebCore/html/HTMLAttributeNames.in:
* Source/WebCore/html/HTMLElement.cpp:
(WebCore::HTMLElement::writingsuggestions const):
(WebCore::HTMLElement::setWritingsuggestions):
* Source/WebCore/html/HTMLElement.h:
* Source/WebCore/html/HTMLElement.idl:
* Source/WebCore/html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::supportsWritingSuggestions const):
* Source/WebCore/html/HTMLInputElement.h:
* Source/WebKit/Shared/EditorState.cpp:
(WebKit::operator<<):
* Source/WebKit/Shared/EditorState.h:
* Source/WebKit/Shared/EditorState.serialization.in:
* Source/WebKit/Shared/FocusedElementInformation.h:
* Source/WebKit/Shared/FocusedElementInformation.serialization.in:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView _updateTextInputTraits:]):
* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(WebKit::WebViewImpl::postLayoutDataForContentEditable):
(WebKit::WebViewImpl::allowsInlinePredictions const):
* Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:
(WebKit::WebPage::getPlatformEditorStateCommon const):
* Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::focusedElementInformation):
* Tools/TestWebKitAPI/SourcesCocoa.txt:
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/WritingSuggestions.mm: Added.
(-[WritingSuggestionsWebAPIWKWebView initWithHTMLString:]):
(-[WritingSuggestionsWebAPIWKWebView focusElementAndEnsureEditorStateUpdate:]):
(TEST):

Canonical link: https://commits.webkit.org/274912@main
@dandclark
Copy link
Contributor

@annevk and @domenic, I went ahead and added Telephone to the list of valid input types for writingsuggestions in this PR based on comments here: [1], [2].

@annevk, do you have thoughts on my answer here regarding the attribute not returning different values for elements that don't support writing suggestions? I’d like to try to come to a decision on this one way or another to unblock this spec.

@annevk
Copy link
Member

annevk commented Feb 21, 2024

@dandclark if as you say implementations can do whatever and none of it is exposed to script, how is any of it actually relevant?

@dandclark
Copy link
Contributor

@annevk That section is an attempt to follow the example of spellcheck, which has a similar section ("User agents must only consider the following pieces of text as checkable for the purposes of this feature..."). It describes which kinds of elements should be spell-checkable by user agents, but those details are not exposed directly to script/HTML.

I think the purpose of having a section like this for spellcheck and writingSuggestions is to give authors an idea of the widest possible set of elements that user agents might apply those features to. However, an alternative approach could be to replace it with a non-normative note that describes more generally the types of places wshere user agents might apply the feature, i.e. anywhere there's editable text. I could make that change for the writingSuggestions if you think it would be preferable.

@annevk annevk added the addition/proposal New features or enhancements label Mar 4, 2024
@annevk
Copy link
Member

annevk commented Mar 4, 2024

Spellchecking also has the concept of "true-by-default", but it's not clear that's actually implemented (it doesn't seem implemented in WebKit at least). Not sure what to make of it. Unless we have some way of testing it I suppose I rather we don't make requirements around this and file a follow-up issue against spellcheck to remove it there as well. Curious to know if @domenic would agree with that or if he sees some kind of model here that is reasonable.

@domenic
Copy link
Member

domenic commented Mar 5, 2024

I'd rather keep consistency with spellcheck=""'s exclusion criteria. In general I find the spellcheck="" model reasonable; that's why I suggested copying it.

It's true that spellcheck="" has more possibilities for defaulting, whereas we've decided writingsuggestions="" is always inherit-by-default if there's a parent / true-by-default if there's no parent for the non-excluded elements. That model does seem more simple and I'd support moving spellcheck="" to it, if nobody implements false-by-default.

We do have ways of testing; we can test that element.writingSuggestions === "false" for the elements for which it's excluded from working on.

@annevk
Copy link
Member

annevk commented Mar 5, 2024

No we can't test that. I suggested testing that it's false for disabled form controls for instance, but as @dandclark pointed out nothing requires the IDL attribute to be false for that case.

@domenic
Copy link
Member

domenic commented Mar 5, 2024

Oh, I see what you mean.

I still think the simplified-spellcheck model (i.e., without true/false by default) is a reasonable one. writingSuggestions and spellcheck IDL attributes control and return according to the content attribute and DOM tree ancestors, even on excluded elements.

Here's a crux case:

<div writingsuggestions="true" spellcheck="true">
  <span>
    <input type="text">
  </span>
</div>

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=12426

Both models under consideration suggest that inputEl.writingSuggestions === "true" and inputEl.spellcheck === true.

However, the model in this PR suggests that div.writingSuggestions === "true", matching div.spellcheck === true. The model you are proposing, where div.writingSuggestions is influenced by the element exclusions, suggests div.writingSuggestions === "false". This seems to be against the way that reflect-ish IDL attributes usually work, and means there's no JavaScript API beyond getAttribute() for actually probing the DOM state.

Similarly, the model in this PR suggests span.writingSuggestions === "true" (matching span.spellcheck === true). In this case it's probing the DOM state, but doing so by traversing up the tree. That still seems reasonable to me.

@annevk
Copy link
Member

annevk commented Mar 5, 2024

I don't think I suggested that the div element in that example should be false? Just that <input disabled> should be.

Anyway, my further point was that if we are not going to take into account <input disabled> in the API, why even say something about it?

@domenic
Copy link
Member

domenic commented Mar 5, 2024

I don't think I suggested that the div element in that example should be false? Just that <input disabled> should be.

I thought the suggestion was to reflect all of "User agents must only offer suggestions within an element's scope if the result of running the following algorithm given element returns true:", except I guess not the user-preference portions per #10018 (comment), in the API.

If that's not the suggestion, maybe you could make it more concrete exactly which portions you're proposing to reflect in the API, and why those portions specifically you think are best reflected in the API and not others?

Anyway, my further point was that if we are not going to take into account <input disabled> in the API, why even say something about it?

It seems reasonable for the spec to require that the user not be bothered by writing suggestions in disabled input elements, just like it currently requires that the user not be bothered by spellchecking in such elements. Since the whole section is about browser UI, I agree that it's not 100% clear we're providing value with such requirements; it's not web-observable and probably implementers would figure that out anyway. But I can imagine authors being confused why they get red squiggles or "write for me" popups in <input disabled> on browser X and not browser Y, so adding some basic interop suggestions seems reasonable to me.

We could soften both spellcheck and writing suggestions from "must" to "should", though. That would be congruent with how we treat most other browser UI suggestions.

@dandclark
Copy link
Contributor

We could soften both spellcheck and writing suggestions from "must" to "should", though. That would be congruent with how we treat most other browser UI suggestions.

@annevk, would this address your concern?

@annevk
Copy link
Member

annevk commented Mar 11, 2024

Yeah, I think that would work, thanks!

@dandclark
Copy link
Contributor

Yeah, I think that would work, thanks!

Great! I've made that change.

@annevk , @domenic , any other concerns or could we consider landing this PR?

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 your patience!

@domenic domenic merged commit a187fec into whatwg:main Mar 13, 2024
2 checks passed
@sanketj
Copy link
Member Author

sanketj commented Mar 13, 2024

Thanks everyone for the feedback and reviews.

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
Development

Successfully merging this pull request may close these issues.

6 participants