Improve substitution heuristic. Currently we may attach the Alternative Presentation Button shadow DOM to an arbitrary element. This approach is problematic when the element that is substituted has an existing shadow DOM or supports an author shadow DOM because the we do not support more than one shadow root per node. There is also a concern that the implementation of some elements may interfere with the Alternative Presentation Button shadow DOM. We should determine a set of elements that behave well ("safe") when the Alternative Presentation Button shadow DOM is attached and change the substitution heuristic to only perform the substitution if the there exists at least one "safe" element in the set of elements to be substituted.
<rdar://problem/34916961>
Created attachment 328052[details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328053[details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 328054[details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328055[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 328136[details]
Patch and layout test
We likely can simplify the logic for picking the element for alternative presentation in Editor::substituteWithAlternativePresentationButton(), including removing the need to expose Element::canAttachAuthorShadowRoot(). We are still iterating on the algorithm and I want to avoid a programmer error of inadvertently whitelisting an element that can have an author shadow root. Once we finalize the algorithm we will be in a position to determine whether it is necessary to expose Element::canAttachAuthorShadowRoot() to catch such errors. I added more test cases to LayoutTests/fast/forms/alternative-presentation-button/replacement.html and expect the EWS to fail this test. Will grab results from EWS and update patch.
Created attachment 328172[details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328174[details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 328178[details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328190[details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 328192[details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328195[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.12.6
Created attachment 328196[details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328358[details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328359[details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 328361[details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 328350[details]
Patch and layout test
View in context: https://bugs.webkit.org/attachment.cgi?id=328350&action=review> Source/WebCore/editing/Editor.cpp:3866
> + for (auto* tagName : tagNames) {
> + if (element.hasTagName(*tagName))
> + return true;
> + }
> + return false;
Instead of for looping, we should create a NeverDestroyed HashSet.
> Source/WebCore/editing/Editor.cpp:3868
> + auto findElementForAlternativePresentation = [&] (const Vector<Ref<Element>>& ancestorElements) {
What's the point of having this inner lambda? It would a lot cleaner to have this function as a separate static local function.
> Source/WebCore/editing/Editor.cpp:3904
> + // Fix up the the list of elements to hide so that we do not hide the element for alternative presentation.
Instead of having a comment like this, it's a lot better to extract a static local function
of a descriptive name like elementsToHideExcludingAlternativePresentation.
> LayoutTests/fast/forms/alternative-presentation-button/replacement.html:43
> + <a>a</a>
You should add a test case with href: <a href="">
By the way, can't we use dump-as-markup.js in these tests? That would make the output a lot more readable & platform independent.
Created attachment 328618[details]
Patch for landing
Patch that incorporates the feedback from Ryosuke Niwa. This patch depends on the patch for bug #180381. Otherwise, the layout test fast/forms/alternative-presentation-button/replacement.html will crash due to an assertion failure when executing "document.body.removeChild(testContainer);"
2017-11-30 15:26 PST, Daniel Bates
2017-11-30 16:30 PST, EWS Watchlist
2017-11-30 16:38 PST, EWS Watchlist
2017-11-30 16:44 PST, EWS Watchlist
2017-11-30 16:48 PST, EWS Watchlist
2017-12-01 12:46 PST, Daniel Bates
2017-12-01 15:49 PST, EWS Watchlist
2017-12-01 15:56 PST, EWS Watchlist
2017-12-01 16:11 PST, EWS Watchlist
2017-12-01 16:13 PST, Daniel Bates
2017-12-01 17:34 PST, EWS Watchlist
2017-12-01 17:36 PST, EWS Watchlist
2017-12-01 17:44 PST, EWS Watchlist
2017-12-01 17:44 PST, EWS Watchlist
2017-12-04 09:21 PST, Daniel Bates
ews-watchlist: commit-queue-
2017-12-04 10:22 PST, EWS Watchlist
2017-12-04 10:33 PST, EWS Watchlist
2017-12-04 10:36 PST, EWS Watchlist
2017-12-06 13:46 PST, Daniel Bates