Bug 180086 – Alternative Presentation Button: Improve substitution heuristic

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Daniel Bates]

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>

[Daniel Bates]

Created attachment 328039 [details]
Patch and layout test

[EWS Watchlist]

Comment on attachment 328039 [details]
Patch and layout test

Attachment 328039 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5427658

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328039 [details]
Patch and layout test

Attachment 328039 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5427759

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328039 [details]
Patch and layout test

Attachment 328039 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5427629

New failing tests:
svg/custom/object-sizing-explicit-height.xhtml
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328039 [details]
Patch and layout test

Attachment 328039 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5427716

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[Daniel Bates]

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.

[EWS Watchlist]

Comment on attachment 328136 [details]
Patch and layout test

Attachment 328136 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5459193

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328136 [details]
Patch and layout test

Attachment 328136 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5459194

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328136 [details]
Patch and layout test

Attachment 328136 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5459263

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[Daniel Bates]

Created attachment 328179 [details]
Patch and layout test

[EWS Watchlist]

Comment on attachment 328179 [details]
Patch and layout test

Attachment 328179 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5460733

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328179 [details]
Patch and layout test

Attachment 328179 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5460623

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328179 [details]
Patch and layout test

Attachment 328179 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5460714

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328179 [details]
Patch and layout test

Attachment 328179 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5460707

New failing tests:
svg/custom/object-sizing-explicit-height.xhtml
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[Daniel Bates]

Created attachment 328350 [details]
Patch and layout test

[EWS Watchlist]

Comment on attachment 328350 [details]
Patch and layout test

Attachment 328350 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5488740

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328350 [details]
Patch and layout test

Attachment 328350 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5488818

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 328350 [details]
Patch and layout test

Attachment 328350 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5488720

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

[Ryosuke Niwa]

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.

[Daniel Bates]

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);"

[Daniel Bates]

The approach for the alternative presentation button has since changed. This bug is no longer applicable.