Bug 194764 – Some refinements for Node and Document

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Some refinements for Node and Document

[Darin Adler]

Created attachment 362243 [details]
Patch

[EWS Watchlist]

Comment on attachment 362243 [details]
Patch

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

New failing tests:
editing/inserting/inserting-trailing-space-and-letter.html
fast/writing-mode/english-bt-text-with-spelling-marker.html
editing/pasteboard/paste-image-using-image-data.html
fast/text/scroll-text-overflow-ellipsis.html
fast/dom/blur-contenteditable.html
fast/layers/scroll-rect-to-visible.html

[EWS Watchlist]

Created attachment 362251 [details]
Archive of layout-test-results from ews103 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 362243 [details]
Patch

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

New failing tests:
editing/inserting/inserting-trailing-space-and-letter.html
fast/text/scroll-text-overflow-ellipsis.html
fast/layers/scroll-rect-to-visible.html
fast/dom/blur-contenteditable.html
editing/pasteboard/paste-image-using-image-data.html

[EWS Watchlist]

Created attachment 362252 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

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

[EWS Watchlist]

Comment on attachment 362243 [details]
Patch

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

New failing tests:
editing/inserting/inserting-trailing-space-and-letter.html
fast/writing-mode/english-bt-text-with-spelling-marker.html
editing/pasteboard/paste-image-using-image-data.html
fast/text/scroll-text-overflow-ellipsis.html
fast/dom/blur-contenteditable.html
fast/layers/scroll-rect-to-visible.html

[EWS Watchlist]

Created attachment 362255 [details]
Archive of layout-test-results from ews116 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 362243 [details]
Patch

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

New failing tests:
fast/layers/scroll-rect-to-visible.html
fast/dom/blur-contenteditable.html
editing/input/ios/rtl-keyboard-input-on-focus.html
fast/writing-mode/english-bt-text-with-spelling-marker.html

[EWS Watchlist]

Created attachment 362258 [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 362243 [details]
Patch

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

New failing tests:
fast/dom/blur-contenteditable.html
fast/text/scroll-text-overflow-ellipsis.html
fast/layers/scroll-rect-to-visible.html
editing/inserting/inserting-trailing-space-and-letter.html
fast/writing-mode/english-bt-text-with-spelling-marker.html

[EWS Watchlist]

Created attachment 362261 [details]
Archive of layout-test-results from ews205 for win-future

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

[Darin Adler]

Created attachment 362291 [details]
Patch

[EWS Watchlist]

Comment on attachment 362291 [details]
Patch

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

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 362294 [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

[Darin Adler]

Created attachment 362295 [details]
Patch

[Darin Adler]

Patch compiles and passes all tests; now looking for a reviewer. I think Chris or Ryosuke might be best since this affects DOM and specifically has a small amount of refactoring/changes for shadow DOM support.

[Ryosuke Niwa]

Comment on attachment 362295 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:748
> +    switch (change.type) {

Maybe we can add a helper like affectsElements() to ChildChange?

> Source/WebCore/dom/Document.cpp:749
> +        for (auto& node : composedTreeDescendants(*this)) {
> +            if (!is<Element>(node))

This will make access key work in shadow trees.
We probably want to add a test for that.

e.g. add an element with accesskey inside a shadow tree, then emulate it via eventSender.
The element should be focused and such.

> Source/WebCore/dom/Element.cpp:1560
> +        else if (name == classAttr)

Weird that this one doesn't have HTMLNames qualification...

> Source/WebCore/dom/Node.cpp:1013
> +    // FIXME: Why is this implementation so different from containsIncludingShadowDOM
> +    // function, given that the two functions' purposes are nearly identical?
> +    return other && (isDescendantOf(*other) || other->contains(shadowHost()));

This function is clearly broken.
This element's shadow tree's host could be inside another shadow tree.
And this function doesn't take care of that.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:172
> -        && !(shadowAncestorNode && shadowAncestorNode->renderer() && shadowAncestorNode->renderer()->isTextControl())
> +        && !(shadowHost && shadowHost->renderer() && shadowHost->renderer()->isTextControl())

We should probably just use enclosingTextFormControl(firstPositionInNode(shadowHost)).

> Source/WebCore/html/HTMLAreaElement.cpp:72
> -    } else if (name == altAttr || name == accesskeyAttr) {
> +    } else if (name == altAttr) {

Hm.. I wonder if we can write a test for this? Is there any behavior difference here??

> Source/WebCore/html/HTMLSelectElement.cpp:-310
> -    else if (name == accesskeyAttr) {

Ditto. I feel like this might be artifact of WebKit not making these elements focusable somehow...
If we had accesskey, we need to make the element focusable.

> Source/WebCore/html/HTMLTextAreaElement.cpp:-197
> -    } else if (name == accesskeyAttr) {
> -        // ignore for the moment

Ditto. We should probably add a test for these three elements.

> Source/WebCore/loader/DocumentWriter.cpp:193
> -TextResourceDecoder* DocumentWriter::createDecoderIfNeeded()
> +TextResourceDecoder& DocumentWriter::decoder()

ensureEncoder per our naming convention?

> Source/WebCore/page/DragController.cpp:353
> +static bool hasEnabledColorInputAsShadowHost(Node& node)

I'd call this isInShadowTreeOfEnabledColorInput instead.

[Darin Adler]

Comment on attachment 362295 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:749
>> +            if (!is<Element>(node))
> 
> This will make access key work in shadow trees.
> We probably want to add a test for that.
> 
> e.g. add an element with accesskey inside a shadow tree, then emulate it via eventSender.
> The element should be focused and such.

This is not intended to be a change in behavior. Note that the old version of this function already recursed into the shadow root.

Yes, I could add a test, just to make sure I’m not breaking it. Or I can find the existing test.

>> Source/WebCore/dom/Element.cpp:1560
>> +        else if (name == classAttr)
> 
> Weird that this one doesn't have HTMLNames qualification...

Good point. Maybe none of them need it.

>> Source/WebCore/dom/Node.cpp:1013
>> +    return other && (isDescendantOf(*other) || other->contains(shadowHost()));
> 
> This function is clearly broken.
> This element's shadow tree's host could be inside another shadow tree.
> And this function doesn't take care of that.

OK. Would you like me to do something about it? I was preserving behavior, but I’d also be happy to go further and fix the function or get rid of it; need your advice on the direction to take.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:172
>> +        && !(shadowHost && shadowHost->renderer() && shadowHost->renderer()->isTextControl())
> 
> We should probably just use enclosingTextFormControl(firstPositionInNode(shadowHost)).

I’ll look into making that change in a follow-up patch.

>> Source/WebCore/html/HTMLAreaElement.cpp:72
>> +    } else if (name == altAttr) {
> 
> Hm.. I wonder if we can write a test for this? Is there any behavior difference here??

No, there’s no behavior difference. This code has no effect on whether the acesskey attribute is "respected" for these elements.

It’s normally harmless to not call through to the base class. In fact, I’d prefer that the pattern be that we always call through; we get a small performance boost from not calling through that I would be getting rid of if I made that change broadly.

I’m pretty sure that it’s not important for altAttr to not call through so I could have deleted that case case well. Since I added code for accesskeyAttr in the base class, I needed to remove all places like this that choose not to call through to it, and that’s why I did this.

If we don’t want accesskey to have an effect on area elements, we need a code change elsewhere. This code tha not calling through to parseAttribute had no effect on whether the attribute was respected.

>> Source/WebCore/html/HTMLSelectElement.cpp:-310
>> -    else if (name == accesskeyAttr) {
> 
> Ditto. I feel like this might be artifact of WebKit not making these elements focusable somehow...
> If we had accesskey, we need to make the element focusable.

No, that’s not right. This code had no effect before and did not prevent accesskey attributes from having an effect on select elements.

It’s true, perhaps, that we don’t want accesskey to work on these, but to accomplish that we need to make a change to another function; parseAttribute has no effect on that.

>> Source/WebCore/html/HTMLTextAreaElement.cpp:-197
>> -        // ignore for the moment
> 
> Ditto. We should probably add a test for these three elements.

There’s no behavior change for the same reason as above. Adding a test is fine, but has little to do with this patch.

>> Source/WebCore/loader/DocumentWriter.cpp:193
>> +TextResourceDecoder& DocumentWriter::decoder()
> 
> ensureEncoder per our naming convention?

Do we really need to add the word "ensure"? I’ll do it if you insist, but I’d prefer to just leave it named decoder().

I can’t remember what we decided on for our convention. I personally prefix the thing()/existingThing() pairing, but maybe I was outvoted?

>> Source/WebCore/page/DragController.cpp:353
>> +static bool hasEnabledColorInputAsShadowHost(Node& node)
> 
> I'd call this isInShadowTreeOfEnabledColorInput instead.

OK. I will rename it.

[Ryosuke Niwa]

Comment on attachment 362295 [details]
Patch

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

>>> Source/WebCore/dom/Document.cpp:749
>>> +            if (!is<Element>(node))
>> 
>> This will make access key work in shadow trees.
>> We probably want to add a test for that.
>> 
>> e.g. add an element with accesskey inside a shadow tree, then emulate it via eventSender.
>> The element should be focused and such.
> 
> This is not intended to be a change in behavior. Note that the old version of this function already recursed into the shadow root.
> 
> Yes, I could add a test, just to make sure I’m not breaking it. Or I can find the existing test.

Oh, I see. Sorry, I missed the part where it recursed into buildAccessKeyCache.

> Source/WebCore/dom/Document.cpp:755
> +            map.add(key, &element);

Hm... because this would override the existing entry in the map, there is a subtle behavior change.
Namely, if an element with the same accesskey appears inside the shadow tree
as well as its slotted content, this would change which element will be associated with that accesskey.

In the following example, Option+Control currently focuses the slotted content.
I expect after this code change, the shadow content will be focused.
(You may need to swap the location of slot & button in the shadow tree to reproduce the difference).

<!DOCTYPE html>
<html>
<body>
<button href="javascript:void" accesskey="w">before</button>
<div id="host"><button href="javascript:void" accesskey="w">slotted</button></div>
<script>
const shadowRoot = host.attachShadow({mode: 'closed'});
shadowRoot.innerHTML = `<slot></slot><div><button href="javascript:void" accesskey="w">shadow</button>`;
</script>
</body>
</html>

None of this seems to be defined anywhere so I filed https://github.com/whatwg/html/issues/4385

>>> Source/WebCore/html/HTMLAreaElement.cpp:72
>>> +    } else if (name == altAttr) {
>> 
>> Hm.. I wonder if we can write a test for this? Is there any behavior difference here??
> 
> No, there’s no behavior difference. This code has no effect on whether the acesskey attribute is "respected" for these elements.
> 
> It’s normally harmless to not call through to the base class. In fact, I’d prefer that the pattern be that we always call through; we get a small performance boost from not calling through that I would be getting rid of if I made that change broadly.
> 
> I’m pretty sure that it’s not important for altAttr to not call through so I could have deleted that case case well. Since I added code for accesskeyAttr in the base class, I needed to remove all places like this that choose not to call through to it, and that’s why I did this.
> 
> If we don’t want accesskey to have an effect on area elements, we need a code change elsewhere. This code tha not calling through to parseAttribute had no effect on whether the attribute was respected.

Okay.

>>> Source/WebCore/html/HTMLSelectElement.cpp:-310
>>> -    else if (name == accesskeyAttr) {
>> 
>> Ditto. I feel like this might be artifact of WebKit not making these elements focusable somehow...
>> If we had accesskey, we need to make the element focusable.
> 
> No, that’s not right. This code had no effect before and did not prevent accesskey attributes from having an effect on select elements.
> 
> It’s true, perhaps, that we don’t want accesskey to work on these, but to accomplish that we need to make a change to another function; parseAttribute has no effect on that.

The spec explicitly talks about these elements having accesskey.
So perhaps it would be a good idea to test some tests.
Maybe this used to be disabled purposefully but presumably that's no longer the case now,
which coincidentally matches that the HTML5 spec says:
https://html.spec.whatwg.org/multipage/interaction.html#assigned-access-key

>>> Source/WebCore/loader/DocumentWriter.cpp:193
>>> +TextResourceDecoder& DocumentWriter::decoder()
>> 
>> ensureEncoder per our naming convention?
> 
> Do we really need to add the word "ensure"? I’ll do it if you insist, but I’d prefer to just leave it named decoder().
> 
> I can’t remember what we decided on for our convention. I personally prefix the thing()/existingThing() pairing, but maybe I was outvoted?

Oh, I guess we landed on
StyleResolver* styleResolverIfExists()
StyleResolver& styleResolver()
https://lists.webkit.org/pipermail/webkit-dev/2013-June/025074.html

Maybe we should update the style guidelines...

[Darin Adler]

Comment on attachment 362295 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:755
>> +            map.add(key, &element);
> 
> Hm... because this would override the existing entry in the map, there is a subtle behavior change.
> Namely, if an element with the same accesskey appears inside the shadow tree
> as well as its slotted content, this would change which element will be associated with that accesskey.
> 
> In the following example, Option+Control currently focuses the slotted content.
> I expect after this code change, the shadow content will be focused.
> (You may need to swap the location of slot & button in the shadow tree to reproduce the difference).
> 
> <!DOCTYPE html>
> <html>
> <body>
> <button href="javascript:void" accesskey="w">before</button>
> <div id="host"><button href="javascript:void" accesskey="w">slotted</button></div>
> <script>
> const shadowRoot = host.attachShadow({mode: 'closed'});
> shadowRoot.innerHTML = `<slot></slot><div><button href="javascript:void" accesskey="w">shadow</button>`;
> </script>
> </body>
> </html>
> 
> None of this seems to be defined anywhere so I filed https://github.com/whatwg/html/issues/4385

What you say is not quite right:

1) HashMap::add does *not* override existing entries in a map.
2) I believe the new code still processes an element before it processes its descendants. That’s the same as the old code.

I must admit that, yes, I did change behavior. If two elements have the same accesskey, the old code would make the later one in document order win (because it uses HashMap::set) and my new code would make the earlier one in document order win (because it uses HashMap::add). I could change the add back into a set to get rid of the behavior change. Or write tests for this edge case. I am tempted to do neither, but you can keep me honest.

>>>> Source/WebCore/html/HTMLSelectElement.cpp:-310
>>>> -    else if (name == accesskeyAttr) {
>>> 
>>> Ditto. I feel like this might be artifact of WebKit not making these elements focusable somehow...
>>> If we had accesskey, we need to make the element focusable.
>> 
>> No, that’s not right. This code had no effect before and did not prevent accesskey attributes from having an effect on select elements.
>> 
>> It’s true, perhaps, that we don’t want accesskey to work on these, but to accomplish that we need to make a change to another function; parseAttribute has no effect on that.
> 
> The spec explicitly talks about these elements having accesskey.
> So perhaps it would be a good idea to test some tests.
> Maybe this used to be disabled purposefully but presumably that's no longer the case now,
> which coincidentally matches that the HTML5 spec says:
> https://html.spec.whatwg.org/multipage/interaction.html#assigned-access-key

If it was disabled, that was a very long time ago. Our accesskey implementation hasn’t depended on this for a long time, if ever. I think someone might have *thought* this disabled something but not sure it ever did.

>>>> Source/WebCore/loader/DocumentWriter.cpp:193
>>>> +TextResourceDecoder& DocumentWriter::decoder()
>>> 
>>> ensureEncoder per our naming convention?
>> 
>> Do we really need to add the word "ensure"? I’ll do it if you insist, but I’d prefer to just leave it named decoder().
>> 
>> I can’t remember what we decided on for our convention. I personally prefix the thing()/existingThing() pairing, but maybe I was outvoted?
> 
> Oh, I guess we landed on
> StyleResolver* styleResolverIfExists()
> StyleResolver& styleResolver()
> https://lists.webkit.org/pipermail/webkit-dev/2013-June/025074.html
> 
> Maybe we should update the style guidelines...

Hooray, I can name it decoder!

[Ryosuke Niwa]

For posterity, I've posted a patch to update the style guideline in https://bugs.webkit.org/show_bug.cgi?id=194930.

[Darin Adler]

OK, I created a test and it shows that, before the patch:

1) accesskey does follow a "last node in document order wins" rule
2) accesskey does work in shadow trees
3) when both the slotted button and the shadow button have the same accesskey, somehow the slotted button always seems to win, regardless of where the slot is

That behavior (3) is a bit mystifying.

I will add the test to my patch. I do intend to change rule (1) so that "first node in document order wins". As an aside, HTML specification says that "first node to claim the key wins", which is a rule about timing, not document order. We can return to fix that at some point.

I’d like to understand this (3) thing better.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #22)
> OK, I created a test and it shows that, before the patch:
> 
> 1) accesskey does follow a "last node in document order wins" rule
> 2) accesskey does work in shadow trees
> 3) when both the slotted button and the shadow button have the same
> accesskey, somehow the slotted button always seems to win, regardless of
> where the slot is
> 
> That behavior (3) is a bit mystifying.

I don't think that's surprising. In the original code, we were always traversing the shadow tree, which DOES NOT have any slotted content before traversing the descendants of the shadow host. Because a slotted element is always a descendent, in fact, it's always a direct child of the shadow host, so it would always appear later in our traversal than any element of the shadow tree.

[Darin Adler]

Comment on attachment 362295 [details]
Patch

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

>> Source/WebCore/dom/ContainerNode.cpp:748
>> +    switch (change.type) {
> 
> Maybe we can add a helper like affectsElements() to ChildChange?

I added it, but for now just as a separate function in this file, not as a member function in ChildChange.

>>>> Source/WebCore/dom/Document.cpp:749
>>>> +            if (!is<Element>(node))
>>> 
>>> This will make access key work in shadow trees.
>>> We probably want to add a test for that.
>>> 
>>> e.g. add an element with accesskey inside a shadow tree, then emulate it via eventSender.
>>> The element should be focused and such.
>> 
>> This is not intended to be a change in behavior. Note that the old version of this function already recursed into the shadow root.
>> 
>> Yes, I could add a test, just to make sure I’m not breaking it. Or I can find the existing test.
> 
> Oh, I see. Sorry, I missed the part where it recursed into buildAccessKeyCache.

Added a test.

>>> Source/WebCore/dom/Document.cpp:755
>>> +            map.add(key, &element);
>> 
>> Hm... because this would override the existing entry in the map, there is a subtle behavior change.
>> Namely, if an element with the same accesskey appears inside the shadow tree
>> as well as its slotted content, this would change which element will be associated with that accesskey.
>> 
>> In the following example, Option+Control currently focuses the slotted content.
>> I expect after this code change, the shadow content will be focused.
>> (You may need to swap the location of slot & button in the shadow tree to reproduce the difference).
>> 
>> <!DOCTYPE html>
>> <html>
>> <body>
>> <button href="javascript:void" accesskey="w">before</button>
>> <div id="host"><button href="javascript:void" accesskey="w">slotted</button></div>
>> <script>
>> const shadowRoot = host.attachShadow({mode: 'closed'});
>> shadowRoot.innerHTML = `<slot></slot><div><button href="javascript:void" accesskey="w">shadow</button>`;
>> </script>
>> </body>
>> </html>
>> 
>> None of this seems to be defined anywhere so I filed https://github.com/whatwg/html/issues/4385
> 
> What you say is not quite right:
> 
> 1) HashMap::add does *not* override existing entries in a map.
> 2) I believe the new code still processes an element before it processes its descendants. That’s the same as the old code.
> 
> I must admit that, yes, I did change behavior. If two elements have the same accesskey, the old code would make the later one in document order win (because it uses HashMap::set) and my new code would make the earlier one in document order win (because it uses HashMap::add). I could change the add back into a set to get rid of the behavior change. Or write tests for this edge case. I am tempted to do neither, but you can keep me honest.

Added a test that shows the change in the behavior.

>>> Source/WebCore/dom/Element.cpp:1560
>>> +        else if (name == classAttr)
>> 
>> Weird that this one doesn't have HTMLNames qualification...
> 
> Good point. Maybe none of them need it.

Added a HTMLNames qualifier here, but didn’t remove the "using namespace HTMLNames" at the top of this file. Could do that later.

>>> Source/WebCore/dom/Node.cpp:1013
>>> +    return other && (isDescendantOf(*other) || other->contains(shadowHost()));
>> 
>> This function is clearly broken.
>> This element's shadow tree's host could be inside another shadow tree.
>> And this function doesn't take care of that.
> 
> OK. Would you like me to do something about it? I was preserving behavior, but I’d also be happy to go further and fix the function or get rid of it; need your advice on the direction to take.

I left this alone for now.

>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:172
>>> +        && !(shadowHost && shadowHost->renderer() && shadowHost->renderer()->isTextControl())
>> 
>> We should probably just use enclosingTextFormControl(firstPositionInNode(shadowHost)).
> 
> I’ll look into making that change in a follow-up patch.

I left this alone for now.

>>>>> Source/WebCore/html/HTMLSelectElement.cpp:-310
>>>>> -    else if (name == accesskeyAttr) {
>>>> 
>>>> Ditto. I feel like this might be artifact of WebKit not making these elements focusable somehow...
>>>> If we had accesskey, we need to make the element focusable.
>>> 
>>> No, that’s not right. This code had no effect before and did not prevent accesskey attributes from having an effect on select elements.
>>> 
>>> It’s true, perhaps, that we don’t want accesskey to work on these, but to accomplish that we need to make a change to another function; parseAttribute has no effect on that.
>> 
>> The spec explicitly talks about these elements having accesskey.
>> So perhaps it would be a good idea to test some tests.
>> Maybe this used to be disabled purposefully but presumably that's no longer the case now,
>> which coincidentally matches that the HTML5 spec says:
>> https://html.spec.whatwg.org/multipage/interaction.html#assigned-access-key
> 
> If it was disabled, that was a very long time ago. Our accesskey implementation hasn’t depended on this for a long time, if ever. I think someone might have *thought* this disabled something but not sure it ever did.

We have a test that makes sure that accesskey works on all types of elements already, and it passes both with and without the patch. Could add more later, but for now there's no real problem here.

>>> Source/WebCore/page/DragController.cpp:353
>>> +static bool hasEnabledColorInputAsShadowHost(Node& node)
>> 
>> I'd call this isInShadowTreeOfEnabledColorInput instead.
> 
> OK. I will rename it.

Did that.

[Darin Adler]

Committed r241932: <https://trac.webkit.org/changeset/241932>

[Radar WebKit Bug Importer]

<rdar://problem/48303160>

[Truitt Savell]

The new test fast/forms/access-key-shadow-and-ordering.html

added in https://trac.webkit.org/changeset/241932/webkit

is failing on iOS Simulator. History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fforms%2Faccess-key-shadow-and-ordering.html

Diff:
--- /Volumes/Data/slave/ios-simulator-12-debug-tests-wk2/build/layout-test-results/fast/forms/access-key-shadow-and-ordering-expected.txt
+++ /Volumes/Data/slave/ios-simulator-12-debug-tests-wk2/build/layout-test-results/fast/forms/access-key-shadow-and-ordering-actual.txt
@@ -1,15 +1,4 @@
 This test checks to see what happens when the same accesskey is set on multiple elements including cases where the elements are in the a shadow tree.
 
 1c 1w   3c   4w  5w  6c
-1: correct button focused, earlier in document
-1: correct button clicked, earlier in document
-2: correct button focused, in shadow
-2: correct button clicked, in shadow
-3: correct button focused, earlier in document
-3: correct button clicked, earlier in document
-4: correct button focused, earlier in document and in shadow
-4: correct button clicked, earlier in document and in shadow
-5: correct button focused, earlier in document and in shadow
-5: correct button clicked, earlier in document and in shadow
-6: correct button focused, earlier in document in slot
-6: correct button clicked, earlier in document in slot
+

[Ryosuke Niwa]

(In reply to Truitt Savell from comment #27)
> The new test fast/forms/access-key-shadow-and-ordering.html
> 
> added in https://trac.webkit.org/changeset/241932/webkit
> 
> is failing on iOS Simulator. History:
> http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Fforms%2Faccess-key-shadow-and-ordering.
> html

We probably need to skip it on iOS for now.

[Ryosuke Niwa]

Skipped the test on iOS for now:
https://trac.webkit.org/changeset/241973