Bug 54441 – Tab traversal doesn’t enter <keygen>

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Cooney]

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.

[Dominic Cooney]

Created attachment 82427 [details]
WIP Adds a failing layout test.

[Hayato Ito]

Created attachment 93610 [details]
WIP Fix a layout test

[Dominic Cooney]

This looks just like the previous WIP patch, modulo ChangeLog?

[Hayato Ito]

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

[Dominic Cooney]

Awesome! Sorry for the inferior test.

[Dominic Cooney]

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.

[Hayato Ito]

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.

[Hayato Ito]

Created attachment 93727 [details]
A patch in progress

[Hayato Ito]

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.

[Dominic Cooney]

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 ?

[Hayato Ito]

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 ?

[Dominic Cooney]

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.

[Hajime Morrita]

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

[Dominic Cooney]

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.

[Hayato Ito]

Resolved by bug 61410.