'-webkit-user-select: all' is not respected in editable views
Created attachment 353209 [details] Patch
<rdar://problem/34945944>
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
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
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
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
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
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
I guess I'll undo the designmode bit
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
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
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
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
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
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
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
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
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.
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.
Created attachment 353313 [details] Patch
(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.
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.
Can we land this patch??
(In reply to Ryosuke Niwa from comment #23) > Can we land this patch?? Go for it!
Created attachment 367376 [details] Updated for trunk
The new patch still has the issues I pointed out earlier.
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
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
*** Bug 72019 has been marked as a duplicate of this bug. ***
Created attachment 451950 [details] Patch
Comment on attachment 451950 [details] Patch Latest patch addresses review comments and adds additional tests.
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"?
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.
Created attachment 451955 [details] Patch
(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.
Created attachment 451973 [details] Patch
Created attachment 451989 [details] Patch
Comment on attachment 451989 [details] Patch Attempting to implement UIScriptController hooks on GTK.
Created attachment 451992 [details] Patch
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).
Created attachment 452099 [details] Patch for landing
(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.
Created attachment 452134 [details] Patch for landing
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].