Created attachment 95126 [details] WIP - shadow-root as a scope of tab traversal

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..?

Moving of the methods can be a simple refactoring patch in itself.

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.

Created attachment 95889 [details] visits-elements-in-shadow-root

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?

Created attachment 96389 [details] Patch

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 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.

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.

Committed r88421: <http://trac.webkit.org/changeset/88421>