Bug 190977 – '-webkit-user-select: all' is not respected in editable views

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Tim Horton]

'-webkit-user-select: all' is not respected in editable views

[Tim Horton]

Created attachment 353209 [details]
Patch

[Tim Horton]

<rdar://problem/34945944>

[EWS Watchlist]

Comment on attachment 353209 [details]
Patch

Attachment 353209 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9743899

New failing tests:
fast/selectors/read-only-read-write-style-update.html
fast/selectors/read-only-read-write-designmode-basics.html
fast/css/read-only-read-write-designmode-basics.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html

[EWS Watchlist]

Created attachment 353211 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353209 [details]
Patch

Attachment 353209 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9743918

New failing tests:
fast/selectors/read-only-read-write-style-update.html
fast/selectors/read-only-read-write-designmode-basics.html
fast/css/read-only-read-write-designmode-basics.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html

[EWS Watchlist]

Created attachment 353212 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353209 [details]
Patch

Attachment 353209 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9744022

New failing tests:
fast/selectors/read-only-read-write-style-update.html
fast/selectors/read-only-read-write-designmode-basics.html
fast/css/read-only-read-write-designmode-basics.html

[EWS Watchlist]

Created attachment 353214 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Tim Horton]

I guess I'll undo the designmode bit

[EWS Watchlist]

Comment on attachment 353209 [details]
Patch

Attachment 353209 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9743986

New failing tests:
fast/selectors/read-only-read-write-style-update.html
fast/selectors/read-only-read-write-designmode-basics.html
fast/css/read-only-read-write-designmode-basics.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html

[EWS Watchlist]

Created attachment 353215 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353209 [details]
Patch

Attachment 353209 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9744724

New failing tests:
fast/selectors/read-only-read-write-style-update.html
fast/selectors/read-only-read-write-designmode-basics.html
fast/css/read-only-read-write-designmode-basics.html
imported/w3c/web-platform-tests/html/semantics/selectors/pseudo-classes/readwrite-readonly.html

[EWS Watchlist]

Created attachment 353216 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353209 [details]
Patch

Attachment 353209 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9745172

New failing tests:
fast/selectors/read-only-read-write-style-update.html
fast/selectors/read-only-read-write-designmode-basics.html
fast/css/read-only-read-write-designmode-basics.html

[EWS Watchlist]

Created attachment 353220 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 353209 [details]
Patch

Attachment 353209 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9746179

New failing tests:
fast/selectors/read-only-read-write-style-update.html
fast/selectors/read-only-read-write-designmode-basics.html
fast/css/read-only-read-write-designmode-basics.html

[EWS Watchlist]

Created attachment 353224 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Darin Adler]

Comment on attachment 353209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353209&action=review

> Source/WebCore/dom/Node.cpp:778
> +    bool documentIsEditable = document.frame() && document.frame()->page() && document.frame()->page()->isEditable();
> +    if (is<HTMLDocument>(document))
> +        documentIsEditable |= downcast<HTMLDocument>(document).inDesignMode();

This (and the existing code that this was moved from) is peculiar. The inDesignMode function is in Document. Maybe at the original time the code was written it was specific to HTMLDocument. I think we should remove the is<HTMLDocument> check and the downcast as well.

> Tools/DumpRenderTree/mac/UIScriptControllerMac.mm:203
> +    WebView *webView = [mainFrame webView];
> +    [webView setEditable:editable];

Not sure we really need a local variable here.

[Ryosuke Niwa]

Comment on attachment 353209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353209&action=review

> Source/WebCore/dom/Node.cpp:751
> +        if (documentIsEditable)
> +            return Node::Editability::CanEditRichly;
> +

This isn't right. It's totally possible to have -webkit-user-modify: read-only inside a design mode document.
As done in editabilityFromContentEditableAttr, we need to wait until we get out of this loop.
r- because of this and the test failures.

[Tim Horton]

Created attachment 353313 [details]
Patch

[Tim Horton]

(In reply to Ryosuke Niwa from comment #19)
> Comment on attachment 353209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353209&action=review
> 
> > Source/WebCore/dom/Node.cpp:751
> > +        if (documentIsEditable)
> > +            return Node::Editability::CanEditRichly;
> > +
> 
> This isn't right. It's totally possible to have -webkit-user-modify:
> read-only inside a design mode document.

But not inside an editable=YES web view. So like I promised in an earlier comment, I reverted the design-mode-related changes, which were unnecessary in the first place.

> As done in editabilityFromContentEditableAttr, we need to wait until we get
> out of this loop.

I don't think that works? We need to bail here in the editable web view case, or we'll respect user-modify there (which defaults to read-only, and thus breaks editable=YES). I made documentIsEditable just about editable=YES, so it should be OK. We're just moving the editable check downstream, past the user-select checks, instead of affecting user-modify at all.

> r- because of this and the test failures.

[Ryosuke Niwa]

Comment on attachment 353313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353313&action=review

r=me once the following amendments are made.

> Source/WebCore/html/HTMLElement.cpp:407
>      if (auto* startElement = is<Element>(node) ? &downcast<Element>(node) : node.parentElement()) {
>          for (auto& element : lineageOfType<HTMLElement>(*startElement)) {

This loop has the same issue as computeEditabilityFromComputedStyle.
You need to add a check for documentIsEditable before the switch.
Also, please add a test (add a test where the web view is editable & there is a contenteditable=false content.
Make sure that content is editable. We should also add a test for -webkit-user-modify at the same time too.

> Source/WebCore/html/HTMLElement.cpp:433
> -    return editabilityFromContentEditableAttr(*this) != Editability::ReadOnly;
> +    return editabilityFromContentEditableAttr(*this, false) != Editability::ReadOnly;

Please add an enum class instead of using a boolean like this.

> LayoutTests/ChangeLog:10
> +        * editing/selection/user-select-all-in-editable-view-expected.txt: Added.
> +        * editing/selection/user-select-all-in-editable-view.html: Added.

A better place to put more tests I'm asking you to add would be LayoutTests/editing/editability/
But I suppose these tests could stay here.

> LayoutTests/editing/selection/user-select-all-in-editable-view.html:4
> +<p id="description">This tests that moving the cursor inside a 'user-select: all' element expands the selection to the entire element. The cursor should not end up inside the 'user-select: all' element, but rather in the middle of "after".</p>

Please add an instruction on how to run this test manually in the browser.
e.g. you can execute setBaseAndExtent outside testRunner / UI helper check.
Otherwise, it's gonna be really hard to verify that it works in the browser later.

[Ryosuke Niwa]

Can we land this patch??

[Tim Horton]

(In reply to Ryosuke Niwa from comment #23)
> Can we land this patch??

Go for it!

[Ryosuke Niwa]

Created attachment 367376 [details]
Updated for trunk

[Ryosuke Niwa]

The new patch still has the issues I pointed out earlier.

[EWS Watchlist]

Comment on attachment 367376 [details]
Updated for trunk

Attachment 367376 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11860969

New failing tests:
editing/input/ios/rtl-keyboard-input-on-focus.html

[EWS Watchlist]

Created attachment 367379 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Tim Nguyen (:ntim)]

*** Bug 72019 has been marked as a duplicate of this bug. ***

[Aditya Keerthi]

Created attachment 451950 [details]
Patch

[Aditya Keerthi]

Comment on attachment 451950 [details]
Patch

Latest patch addresses review comments and adds additional tests.

[Wenson Hsieh]

Comment on attachment 451950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451950&action=review

> Source/WebCore/dom/Node.cpp:796
> +    auto& document = this->document();

This raw reference makes me a bit nervous, especially since we trigger a style update right below.

But maybe it's okay here because the caller presumably keeps this node alive, which then keeps its document alive?

> Tools/ChangeLog:11
> +        Remove TestRunner::setWebViewEditable as it is used.

Nit - do you mean "unused"?

[Darin Adler]

Comment on attachment 451950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451950&action=review

>> Source/WebCore/dom/Node.cpp:796
>> +    auto& document = this->document();
> 
> This raw reference makes me a bit nervous, especially since we trigger a style update right below.
> 
> But maybe it's okay here because the caller presumably keeps this node alive, which then keeps its document alive?

No, you are right to be worried. Nodes can be moved between documents.

[Aditya Keerthi]

Created attachment 451955 [details]
Patch

[Aditya Keerthi]

(In reply to Wenson Hsieh from comment #32)
> Comment on attachment 451950 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451950&action=review
> 
> > Source/WebCore/dom/Node.cpp:796
> > +    auto& document = this->document();
> 
> This raw reference makes me a bit nervous, especially since we trigger a
> style update right below.
> 
> But maybe it's okay here because the caller presumably keeps this node
> alive, which then keeps its document alive?

Made this a `Ref`.
 
> > Tools/ChangeLog:11
> > +        Remove TestRunner::setWebViewEditable as it is used.
> 
> Nit - do you mean "unused"?

Fixed.

[Aditya Keerthi]

Created attachment 451973 [details]
Patch

[Aditya Keerthi]

Created attachment 451989 [details]
Patch

[Aditya Keerthi]

Comment on attachment 451989 [details]
Patch

Attempting to implement UIScriptController hooks on GTK.

[Aditya Keerthi]

Created attachment 451992 [details]
Patch

[Wenson Hsieh]

Comment on attachment 451992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451992&action=review

> Source/WebCore/dom/Node.cpp:797
> +    auto documentIsEditable = document->page() && document->page()->isEditable() ? DocumentIsEditable::Yes : DocumentIsEditable::No;

`DocumentIsEditable` seems okay to me, but I think a name like `PageIsEditable` would be a bit more explicit, since this concept of "full web view editability" is scoped to the entire Page/WebPage rather than Document. I think it might also help to disambiguate it from the case where `document.designMode` is "on".

> LayoutTests/editing/editability/user-select-all-in-editable-view.html:17
> +        var selection = window.getSelection();
> +        selection.setBaseAndExtent(container, 0, container, 0);

Nit - I think this comment should still be addressed:

> Please add an instruction on how to run this test manually in the browser.
> e.g. you can execute setBaseAndExtent outside testRunner / UI helper check.

While this test won't work in Safari due to requiring `-_setEditable:YES` on the web view, someone who's running the test manually could still run it manually using the Editable web view mode of MiniBrowser to exercise the behavior.
I think we can wrap this in an async function block and do something like:

…

if (window.testRunner && testRunner.runUIScript)
    await UIHelper.setWebViewEditable(true);

const selection = window.getSelection();
selection.setPosition(container, 0);

…

(with a note in the description paragraph above reminding the user to test in an editable web view if they're going to test manually).

[Aditya Keerthi]

Created attachment 452099 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Wenson Hsieh from comment #40)
> Comment on attachment 451992 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451992&action=review

Thanks for the review!
 
> > Source/WebCore/dom/Node.cpp:797
> > +    auto documentIsEditable = document->page() && document->page()->isEditable() ? DocumentIsEditable::Yes : DocumentIsEditable::No;
> 
> `DocumentIsEditable` seems okay to me, but I think a name like
> `PageIsEditable` would be a bit more explicit, since this concept of "full
> web view editability" is scoped to the entire Page/WebPage rather than
> Document. I think it might also help to disambiguate it from the case where
> `document.designMode` is "on".

Changed to `PageIsEditable`.
 
> > LayoutTests/editing/editability/user-select-all-in-editable-view.html:17
> > +        var selection = window.getSelection();
> > +        selection.setBaseAndExtent(container, 0, container, 0);
> 
> Nit - I think this comment should still be addressed:
> 
> > Please add an instruction on how to run this test manually in the browser.
> > e.g. you can execute setBaseAndExtent outside testRunner / UI helper check.
> 
> While this test won't work in Safari due to requiring `-_setEditable:YES` on
> the web view, someone who's running the test manually could still run it
> manually using the Editable web view mode of MiniBrowser to exercise the
> behavior.
> I think we can wrap this in an async function block and do something like:
> 
> …
> 
> if (window.testRunner && testRunner.runUIScript)
>     await UIHelper.setWebViewEditable(true);
> 
> const selection = window.getSelection();
> selection.setPosition(container, 0);
> 
> …
> 
> (with a note in the description paragraph above reminding the user to test
> in an editable web view if they're going to test manually).

Updated all tests.

[Aditya Keerthi]

Created attachment 452134 [details]
Patch for landing

[EWS]

Committed r290098 (247447@main): <https://commits.webkit.org/247447@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452134 [details].