Bug 61410

Summary: Pressing <tab> key should traverse elements in shadow content.
Product: WebKit Reporter: Hayato Ito <hayato>
Component: DOMAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, morrita, rolandsteiner
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 61413, 61839    
Bug Blocks: 61409, 54441    
Attachments:
Description Flags
WIP - shadow-root as a scope of tab traversal
none
visits-elements-in-shadow-root
none
Patch dglazkov: review+

Description Hayato Ito 2011-05-24 19:17:54 PDT
Tab traversal does not visit elements in shadow DOM.
We should traverse focusable elements of shadow content by pressing <tab> key.
Comment 1 Hayato Ito 2011-05-26 23:58:04 PDT
Created attachment 95126 [details]
WIP - shadow-root as a scope of tab traversal
Comment 2 Hayato Ito 2011-05-27 00:21:59 PDT
I posted a patch,  which is almost complete, but there remains issues:


- Some elements (e.g. HTMLInputElement and HTMLTextAreaElement) do extra work in their focus(). So focusing elements on their shadow-root might cause an undesired behavior. I'm investigating.

- We might have some control mechanism by which shadow host can choose whether only shadow host can be a target of tab traversal or not. Some shadow host elements might dislike that elements on their shadow root can be automatically tab traversable. 
  shadow-hosts can set 'tabindex=-1' to focusable elements in the shadow root if they want. But that might be annoying..?
Comment 3 Dimitri Glazkov (Google) 2011-05-31 09:13:54 PDT
Moving of the methods can be a simple refactoring patch in itself.
Comment 4 Hayato Ito 2011-06-01 00:43:47 PDT
Yeah, I posted a patch, containing only refactoring, in another bug 61839.

(In reply to comment #3)
> Moving of the methods can be a simple refactoring patch in itself.
Comment 5 Hayato Ito 2011-06-03 04:50:16 PDT
Created attachment 95889 [details]
visits-elements-in-shadow-root
Comment 6 Dimitri Glazkov (Google) 2011-06-07 15:32:05 PDT
Comment on attachment 95889 [details]
visits-elements-in-shadow-root

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

A few nits:

> LayoutTests/fast/dom/shadow/tab-order-iframe-and-shadow.html:104
> +</html>

might as well skip this test on Win, since it doesn't support ensureShadowRoot

> Source/WebCore/page/FocusController.cpp:341
> +Node* FocusController::deepFocusableNode(FocusDirection direction, Node* node, KeyboardEvent* event)

I am not sure why you moved this code. It's hard to see the diff (if any) between new and old code.

> Source/WebCore/page/FocusController.cpp:430
> +    startingTabIndex = (start && start->tabIndex()) ? start->tabIndex() : SHRT_MAX;

Why did you change this?
Comment 7 Hayato Ito 2011-06-07 23:42:26 PDT
Created attachment 96389 [details]
Patch
Comment 8 Hayato Ito 2011-06-07 23:47:40 PDT
Comment on attachment 95889 [details]
visits-elements-in-shadow-root

Thank you for the review.

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

>> LayoutTests/fast/dom/shadow/tab-order-iframe-and-shadow.html:104
>> +</html>
> 
> might as well skip this test on Win, since it doesn't support ensureShadowRoot

Thank you. I was not aware of it.

It seems that all tests in fast/dom/shadow are already skipped in Win.
http://trac.webkit.org/changeset/84533

>> Source/WebCore/page/FocusController.cpp:341
>> +Node* FocusController::deepFocusableNode(FocusDirection direction, Node* node, KeyboardEvent* event)
> 
> I am not sure why you moved this code. It's hard to see the diff (if any) between new and old code.

I guess I moved the function to this position only because that depends on static shadowRoot and isTreeScopeOwner functions.

Okay. Instead of moving this code, I decided to keep this function in original position. Instead, I moved two static functions above this function because these static functions are much smaller.
But, the diff still doesn't look nice in trac.. Sad..

>> Source/WebCore/page/FocusController.cpp:430
>> +    startingTabIndex = (start && start->tabIndex()) ? start->tabIndex() : SHRT_MAX;
> 
> Why did you change this?

Thank you. That's unintentional. It seemed that I wrongly 3-way merged.
I reverted this change.
Comment 9 Dimitri Glazkov (Google) 2011-06-08 09:11:52 PDT
Comment on attachment 96389 [details]
Patch

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

> Source/WebCore/page/FocusController.cpp:174
> +            // FIXME: Some elements (e.g. HTMLInputElement and HTMLTextAreaElement) do extra work in their focus() methods.

File a bug and mention here so that we know when it's safe to remove this comment.

> Source/WebCore/page/FocusController.cpp:176
> +            if (node->nodeName() == "INPUT" || node->nodeName() == "TEXTAREA")

Please  use hasTagName here.
Comment 10 Hayato Ito 2011-06-08 18:56:17 PDT
Hi Dimitri,
Thank you for the review.

I'll address the comments and land this patch after bug 61413 is landed. Without the patch of bug 61413, we leak an element of shadow root.

(In reply to comment #9)
> (From update of attachment 96389 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96389&action=review
> 
> > Source/WebCore/page/FocusController.cpp:174
> > +            // FIXME: Some elements (e.g. HTMLInputElement and HTMLTextAreaElement) do extra work in their focus() methods.
> 
> File a bug and mention here so that we know when it's safe to remove this comment.
> 
> > Source/WebCore/page/FocusController.cpp:176
> > +            if (node->nodeName() == "INPUT" || node->nodeName() == "TEXTAREA")
> 
> Please  use hasTagName here.
Comment 11 Hayato Ito 2011-06-08 22:34:10 PDT
Committed r88421: <http://trac.webkit.org/changeset/88421>