Bug 54441

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

Created attachment 93610 [details] WIP Fix a layout test

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

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

Awesome! Sorry for the inferior test.

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.

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.

Created attachment 93727 [details] A patch in progress

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

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 ?

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.

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

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.

Resolved by bug 61410.