Bug 54441

Summary: Tab traversal doesn’t enter <keygen>
Product: WebKit Reporter: Dominic Cooney <dominicc>
Component: AccessibilityAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, dglazkov, hayato, morrita
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 59812, 61410    
Bug Blocks: 61409    
Attachments:
Description Flags
WIP Adds a failing layout test.
none
WIP Fix a layout test
none
A patch in progress none

Description Dominic Cooney 2011-02-15 01:17:59 PST
Since bug 51379 <keygen> is implemented using shadow DOM. However tab traversal does not visit form elements in shadow DOM. Thus it is not possible to interact with <keygen> using the keyboard.
Comment 1 Dominic Cooney 2011-02-15 01:21:59 PST
Created attachment 82427 [details]
WIP Adds a failing layout test.
Comment 2 Hayato Ito 2011-05-15 23:14:26 PDT
Created attachment 93610 [details]
WIP Fix a layout test
Comment 3 Dominic Cooney 2011-05-15 23:17:00 PDT
This looks just like the previous WIP patch, modulo ChangeLog?
Comment 4 Hayato Ito 2011-05-15 23:19:28 PDT
(In reply to comment #3)
> This looks just like the previous WIP patch, modulo ChangeLog?

It seems the previous test, https://bug-54441-attachments.webkit.org/attachment.cgi?id=82427, just contained a typo, 'shift' should be 'shiftKey'.

The test itself is expected to pass. We need another test which verifies that element in shadow-dom can receives a key event when its host node is focused. I am working on it.
Comment 5 Dominic Cooney 2011-05-15 23:22:14 PDT
Awesome! Sorry for the inferior test.
Comment 6 Dominic Cooney 2011-05-15 23:24:07 PDT
I wonder if you could send a space/enter with EventSender, then a down arrow, and use layoutTestController.shadowRoot to inspect the state of the <select> in the shadow.

That won’t work on platforms that implement <2 algorithms (eg GTK) but this test is already skipped on those IIRC.
Comment 7 Hayato Ito 2011-05-15 23:30:38 PDT
Yeah, that's exactly what I am trying. Thank you!

(In reply to comment #6)
> I wonder if you could send a space/enter with EventSender, then a down arrow, and use layoutTestController.shadowRoot to inspect the state of the <select> in the shadow.
> 
> That won’t work on platforms that implement <2 algorithms (eg GTK) but this test is already skipped on those IIRC.
Comment 8 Hayato Ito 2011-05-16 18:51:06 PDT
Created attachment 93727 [details]
A patch in progress
Comment 9 Hayato Ito 2011-05-16 18:58:26 PDT
I posted a patch, which is still under development.
I guess we need a general solution for shadow-dom, not specific to a <keygen> element.

There are a lot of use cases we must handle. The patch is incomplete.  I'll continue improving.
Maybe I'll file other bugs to separate the problem into small pieces.
Comment 10 Dominic Cooney 2011-05-16 19:29:15 PDT
Comment on attachment 93727 [details]
A patch in progress

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

This looks like good progress—some minor comments inline.

> LayoutTests/fast/forms/keygen-tab-focused-expected.txt:4
> +Before keydown is pressed: 2048 (High Grade)

This will need to be skipped on GTK (and maybe Qt?) because last I checked they don’t implement any key algorithms.

> LayoutTests/fast/forms/keygen-tab-focused.html:28
> +if (window.layoutTestController) {

Consider writing a helper function to extract the value to reduce repetition.

> Source/WebCore/page/FocusController.cpp:173
> +// FIXME: Merge these into SharowRoot.h?

SharowRoot → ShadowRoot

> Source/WebCore/page/FocusController.cpp:180
> +static Node* lastChild(Node* node)

I would name this something else, since it isn’t really lastChild but "last descendant" or something like that.

> Source/WebCore/page/FocusController.cpp:199
> +                            : document->previousFocusableNode(lastChild(shadowRootNode), event);

What does WebKit style say about this? Maybe align the : with the ?
Comment 11 Hayato Ito 2011-05-17 04:54:38 PDT
Hi Dominic, thank you for the review.

I just had another idea today. To avoid cluttering FocusController class further, how about having a 'Universal' traversalNextNode function in Node. That function traverses every reachable nodes, visiting elements in a Frame and a Shadow-dom as well.

Let me try implement such a universalTraverseNode (tentative name) function. I'd like to know whether we can implement that in clean way.

(In reply to comment #10)
> (From update of attachment 93727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93727&action=review
> 
> This looks like good progress—some minor comments inline.
> 
> > LayoutTests/fast/forms/keygen-tab-focused-expected.txt:4
> > +Before keydown is pressed: 2048 (High Grade)
> 
> This will need to be skipped on GTK (and maybe Qt?) because last I checked they don’t implement any key algorithms.
> 
> > LayoutTests/fast/forms/keygen-tab-focused.html:28
> > +if (window.layoutTestController) {
> 
> Consider writing a helper function to extract the value to reduce repetition.
> 
> > Source/WebCore/page/FocusController.cpp:173
> > +// FIXME: Merge these into SharowRoot.h?
> 
> SharowRoot → ShadowRoot
> 
> > Source/WebCore/page/FocusController.cpp:180
> > +static Node* lastChild(Node* node)
> 
> I would name this something else, since it isn’t really lastChild but "last descendant" or something like that.
> 
> > Source/WebCore/page/FocusController.cpp:199
> > +                            : document->previousFocusableNode(lastChild(shadowRootNode), event);
> 
> What does WebKit style say about this? Maybe align the : with the ?
Comment 12 Dominic Cooney 2011-05-19 22:06:30 PDT
Sorry for the slow reply--traversalNextNode in Node, or even a separate class for traversal SGTM.

You mentioned that Node already has methods for traversal... we could start with a nested class and refactor the traversal methods into that class, then deport them.
Comment 13 Hajime Morrita 2011-05-19 22:10:02 PDT
(In reply to comment #12)
> Sorry for the slow reply--traversalNextNode in Node, or even a separate class for traversal SGTM.
> 
> You mentioned that Node already has methods for traversal... we could start with a nested class and refactor the traversal methods into that class, then deport them.

And we already have Bug 59812 for that.
Comment 14 Dominic Cooney 2011-05-19 22:53:08 PDT
I've made 59812 block this bug, but in fact we should tackle 59812 in pieces; focus traversal could be the first piece that introduces the new file. I don't think we need to nail a generic/polymorphic abstraction since a given call site always uses a specific kind of traversal. We just need to make it convenient to use at a given site.
Comment 15 Hayato Ito 2011-06-08 22:36:42 PDT
Resolved by bug 61410.