Summary: | Needs a unified way to handle a case when a shadow host is focused. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hayato Ito <hayato> | ||||||||||
Component: | DOM | Assignee: | Hayato Ito <hayato> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, dominicc, morrita, rolandsteiner, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 61409 | ||||||||||||
Attachments: |
|
Description
Hayato Ito
2011-06-08 23:37:49 PDT
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. 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? (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 :) Created attachment 98475 [details]
Patch
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. 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 Created attachment 98704 [details]
Patch
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. 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. 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. Created attachment 98893 [details]
Patch
Created attachment 99055 [details]
Patch
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 Comment on attachment 99055 [details] Patch Clearing flags on attachment: 99055 Committed r90004: <http://trac.webkit.org/changeset/90004> All reviewed patches have been landed. Closing bug. |