Bug 62358 – Needs a unified way to handle a case when a shadow host is focused.

• Component: DOM
• Status: RESOLVED FIXED

[Hayato Ito]

Let me summarize the current status here:

 * <input type=text> element, and <textarea> element
    - They use a shadow root internally. They have a focusable element, <DIV>, in their shadow root.
    - They do extra works in their focus() method. Due to their extra works, a FocusController should treat them specially.
    - Tab traversal also treats them specially. Tab traversal doesn't look for a focusable element in their shadow root.

 * <kengen> elements, as a representative of usual shadow host elements.
    - <keygen> elements has a focusable 'select' element in the shadow root.
    - <keygen> element doesn't do any extra works in focus() method, so even if we call keygenelement.focus(), a internal 'select' element cannot get focused. Which means we can not change the value by keyboard, <Up> or <Down>.
    - Tab traversal can find a internal <select> element and can focus it. Which means that once we focus it by <Tab> key, we can change the value by keyboard, <UP> or <Down>.

It is nice to get rid of this 'gap'. Let me update this bugzilla entty once I have some ideas to handle this gap nicely.

[Hayato Ito]

How about the following simple rule?

  - If a shadow host is focused (either its focus() is called or it is traversed by tab key), we look for the fist focusable element in its shadow root further and focus that. If such an element is not found in a shadow root, focus the shadow host itself.
  - If a shadow host doesn't like this behavior, the shadow host should set tabindex of all focusable elements in the shadow root to '-1' beforehand so that the shadow host itself is focused.
    We should update <input> or <textarea> element to set tabindex of their focusable elements in their shadow root to '-1' when they are created.

I tried this approach and it seems to work well so far.
If you have any suggestions or opinions, that would be highly welcome.

[Dominic Cooney]

I like the simplicity of that rule. I think it covers most of the use cases.

It is possible to create pathological cases where we want *both* the host and something in the shadow to be focused. For example:

<button>foo</button>

var b = document.createElement('button');
b.textContent = bar;
document.querySelector('button').appendChild(b);

This makes a button-on-a-button and you can focus them individually. So if we want to implement button using shadow DOM, is it possible to support a case like this?

[Dimitri Glazkov (Google)]

(In reply to comment #1)
> How about the following simple rule?
> 
>   - If a shadow host is focused (either its focus() is called or it is traversed by tab key), we look for the fist focusable element in its shadow root further and focus that. If such an element is not found in a shadow root, focus the shadow host itself.
>   - If a shadow host doesn't like this behavior, the shadow host should set tabindex of all focusable elements in the shadow root to '-1' beforehand so that the shadow host itself is focused.
>     We should update <input> or <textarea> element to set tabindex of their focusable elements in their shadow root to '-1' when they are created.
> 
> I tried this approach and it seems to work well so far.
> If you have any suggestions or opinions, that would be highly welcome.

SGTM. We should write this done someplace.... Like a spec or something :)

[Hayato Ito]

Created attachment 98475 [details]
Patch

[Hayato Ito]

I posted a patch, which should be a first step to handle a focus event on a shadow-host.

There remains a lot to do as follows. These might be addressed separately in other patches:

- We should set tabindex=-1 to some elements in a shadow root of shadow host elements, such as <input> or <video>.  This patch simply ignores such shadow host elements so that this does not break existing layout tests.
- Elements in a shadow <content> is not considered in this patch. These elements won't be automatically focused.

[Hajime Morrita]

Comment on attachment 98475 [details]
Patch

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

> LayoutTests/fast/dom/shadow/shadow-host-transfer-focus.html:16
> +    return layoutTestController.ensureShadowRoot(shadowHost);

I guess dominicc moved this method to window.internals.

> LayoutTests/fast/dom/shadow/shadow-host-transfer-focus.html:49
> +

Illustrative comments for the tree shape is welcome ;-)

> LayoutTests/fast/dom/shadow/shadow-host-transfer-focus.html:59
> +    var element = window[id];

we prefer getElementById()

> Source/WebCore/dom/Element.cpp:1590
> +        if (Page* page = doc->page()) {

I think this logic should be owned by ShadowRoot, not Element.

> Source/WebCore/page/FocusController.cpp:174
> +            // skipping these elements is the safest way to keep an compatibility.

nit: an -> a

[Hayato Ito]

Created attachment 98704 [details]
Patch

[Hayato Ito]

Comment on attachment 98475 [details]
Patch

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

>> LayoutTests/fast/dom/shadow/shadow-host-transfer-focus.html:16
>> +    return layoutTestController.ensureShadowRoot(shadowHost);
> 
> I guess dominicc moved this method to window.internals.

Done. Now it uses window.internals.ensureShadowRoot(...)

>> LayoutTests/fast/dom/shadow/shadow-host-transfer-focus.html:49
>> +
> 
> Illustrative comments for the tree shape is welcome ;-)

Done.

>> LayoutTests/fast/dom/shadow/shadow-host-transfer-focus.html:59
>> +    var element = window[id];
> 
> we prefer getElementById()

We can not use getElementById here because the element doesn't have 'id' yet. :)
We set it in the following line.

>> Source/WebCore/dom/Element.cpp:1590
>> +        if (Page* page = doc->page()) {
> 
> I think this logic should be owned by ShadowRoot, not Element.

That sounds nice. But it turned out that it wouldn't be so nice to have such a method in ShadowRoot because we should pass the shadow host to FocusController::deepFocusableElement as an argument, 
So I've moved the code into FocusController as FocusController::transferFocusToElementInShadowRoot.

>> Source/WebCore/page/FocusController.cpp:174
>> +            // skipping these elements is the safest way to keep an compatibility.
> 
> nit: an -> a

Done.

[Hajime Morrita]

Comment on attachment 98704 [details]
Patch

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

Thanks for the update!
I'd like to have one more turn.

> Source/WebCore/ChangeLog:14
> +        That should be addressed in another patch.

Could you file a bug for this? It would block 59802.

> Source/WebCore/ChangeLog:21
> +        (WebCore::FocusController::deepFocusableNode):

Could you update the description?

> Source/WebCore/page/FocusController.cpp:159
> +bool FocusController::transferFocusToElementInShadowRoot(Element* shadowHost, bool restorePreviousSelection)

Could you define a two-value enum instead of bool? It's a recent WebCore trend.

> Source/WebCore/page/FocusController.cpp:162
> +    Node* node = deepFocusableNode(FocusDirectionForward, shadowHost, 0);

I'd love to have more informative name.

> Source/WebCore/page/FocusController.cpp:163
> +    if (shadowHost != node) {

How about to exit  early? It's more preferable in general.

> Source/WebCore/page/FocusController.cpp:186
> +            if (node->hasTagName(inputTag) || node->hasTagName(textareaTag) || node->hasTagName(videoTag) || node->hasTagName(audioTag))

I might be better to extract this conditional to a function like hasCustomFocusLogic or something.
Then we can use it for an assertion.

[Hayato Ito]

Comment on attachment 98704 [details]
Patch

Thank you for the review.

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

>> Source/WebCore/ChangeLog:14
>> +        That should be addressed in another patch.
> 
> Could you file a bug for this? It would block 59802.

I've filed it as bug 63522.

>> Source/WebCore/ChangeLog:21
>> +        (WebCore::FocusController::deepFocusableNode):
> 
> Could you update the description?

Ops. Sorry. 
I've updated the description.

>> Source/WebCore/page/FocusController.cpp:159
>> +bool FocusController::transferFocusToElementInShadowRoot(Element* shadowHost, bool restorePreviousSelection)
> 
> Could you define a two-value enum instead of bool? It's a recent WebCore trend.

Sure. I've defined enum IsFocusTransfered.

>> Source/WebCore/page/FocusController.cpp:162
>> +    Node* node = deepFocusableNode(FocusDirectionForward, shadowHost, 0);
> 
> I'd love to have more informative name.

Okay. I've renamed it to more descriptive one.

>> Source/WebCore/page/FocusController.cpp:163
>> +    if (shadowHost != node) {
> 
> How about to exit  early? It's more preferable in general.

Done.

>> Source/WebCore/page/FocusController.cpp:186
>> +            if (node->hasTagName(inputTag) || node->hasTagName(textareaTag) || node->hasTagName(videoTag) || node->hasTagName(audioTag))
> 
> I might be better to extract this conditional to a function like hasCustomFocusLogic or something.
> Then we can use it for an assertion.

I've extracted this to 'hasCustomFocusLogic' as static inline function.
To keep consistency, I've also replaced a token of 'inline static' with a 'static inline' in the same file because 'static inline' seems to be more common on WebKit code base.

[Hayato Ito]

Created attachment 98893 [details]
Patch

[Hayato Ito]

Created attachment 99055 [details]
Patch

[Hayato Ito]

I've updated a patch, as Morita-san told me that we should use enum for 'bool restorePreviousSelection', not for a return value.
Because it will be likely that we should touch a tons of files in order to change all 'bool restorePreviousSelection' to enum, such a refactoring would be better to be done in another patch.

>> +bool FocusController::transferFocusToElementInShadowRoot(Element* shadowHost, bool restorePreviousSelection)

(In reply to comment #12)
> Created an attachment (id=99055) [details]
> Patch

[WebKit Review Bot]

Comment on attachment 99055 [details]
Patch

Clearing flags on attachment: 99055

Committed r90004: <http://trac.webkit.org/changeset/90004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.