Update sequential focus navigation to include shadow trees

[rakina]

Another part of #2013 and whatwg/dom#768. Note this doesn't include the behavior for focus() with delegatesFocus yet, since this PR is already big enough I'll put that in another PR.

WPT PR: web-platform-tests/wpt#17523

---

/infrastructure.html ( diff )
/interaction.html ( diff )

[rniwa]

This is super confusing way of ordering things. Can we just traverse nodes from shadow root, slot, & document and define what's in each scope instead of defining the scope of an element and then defining the order between them separately?

[rakina]

Actually reading this again, we can just us shadow-including tree order on all of them, and updated the latest patch to use that. What do you think? (I guess we can also not order the focus navigation scope, and use shadow-including tree order directly in tabindex's ordering section...)

[rniwa]

This was the biggest complain I had about the shadow DOM spec as well but I really hate that this approach defines a whole bunch of scopes, and ways to slit nodes into each scope, and then merges them back together.

Can we have a unified algorithm instead? That is, whenever we currently find the next element, why can't we add a step to also find any focus scope owner and then go into the scope as needed? That would make reasoning about the sequential focus navigation order way more easy.

[rakina]

Hmm, somehow I slightly prefer this way because with this, the sequential focus navigation order is defined explicitly and can be used anywhere, but open to changing it the other way too.

I guess roughly the algorithm will be like:
Sequential focus navigation order contains every tabindex ordered focus navigation scope, except those with owner(or host if owner is a shadowroot)'s whose tabindex is negative (applies recursively), and doesn't include hosts with delegatesFocus=true

Get next: will get the next element in the same scope as the current.
If at the start/end of a scope (depending on direction), get next in the owner(or host if owner is a shadowroot)'s scope.

cc @domenic?

[rniwa]

I really don't understand why we want a definition of sequential focus navigation order which doesn't consider shadow roots or slots. We literally don't use that anywhere else in the platform. The most straight forward spec text would be to define one that considers shadow roots, slots, and iframes while finding the next sequentially focusable element.

[rakina]

Updated the "focus navigation search algorithm", what do you think?

[rniwa]

Btw, I really appreciate your work here. We really need to get delegatesFocus spec'ed and support'ed everywhere.

[domenic]

Yay, this is great detail. Overall style nits:

• 1 space indent, not 2
• Use <p> inside each <li>

[rniwa]

This defines what's in sequential focus navigation order but without quite defining the order between each focusable area as far as I can tell.

I think using the concept of shadow-including tree order is a good idea but I think we need to amend it to really walk across focus scopes.

Conceptually, each scope should consist of elements and inner focus scopes (for shadow hosts & slots). Each item (either element or inner focus scope) has some effective tabindex specified (e.g. 0 for non-delegating shadow roots & slots without an explicit tabindex), and the order between them is determined by the effective tabindex. Then the sequential navigation order is really about a tree traversal of focus scopes similar to the one defined for shadow including trees for DOM; We start at the document's focus scope, and then look for the highest tabindex element or scope. If it's a scope and the scope itself isn't focusable, we go into the scope and repeat. etc...

[rakina]

But wouldn't that be similar to defining the overall sequential focus navigation order with the merging algorithm? #4735 (comment)

[rniwa]

I wouldn't say so. The thing that's confusing about the merging algorithm is that we're talking about merging of lists. Given an element, it's unclear what the next element in the sequential navigation order is because we'd have to first locate that element in the merged list, then find the next element in the merged list. But the browser engine isn't not gonna have a literal list so in practice, we'd have to work backwards and figure out what would have been the next list in the merged list without having such a list physically in memory.

[rakina]

OK, I guess it's more or less just moving the newly modified parts of the sequential navigation search algorithm - just making sure before I change the spec again, do you prefer what you described above over the modified sequential navigation search algorithm?

[rniwa]

Yes, I think so. The primary thing is that I want to be able to tell to which element the focus would move under sequential focus navigation given the currently focused element. I'd argue that the imperative approach of defining the algorithm based on the focus scope would be better than creating an abstract list of elements and how to merge them together for that purpose.

[rakina]

Updated, please take a look

[rniwa]

Yeah, the new definition is way easier to understand than the manipulation of lists of lists. Thanks for the change!

[domenic]

OK, I've pushed a round of editorial fixes, and at this point I'm pretty happy with what we have here. Further reviews welcome, especially from @rakina to make sure I didn't screw up anything while refactoring. (I.e., to make sure my changes were all basically editorial.)

To be clear, this is building on the spec's existing model, which is:

• There is a big list of focusable areas for the whole document (the document's "sequential focus navigation order")
• When you do sequential focus (tab/shift-tab), the "sequential navigation search algorithm" finds a "suitable sequentially focusable area" inside that list, forward/backward relative to the starting point value.

Previously, that big list was defined mostly implicitly, with the only rules governing its contents being in the https://html.spec.whatwg.org/#the-tabindex-attribute section.

After this PR, that list is defined very explicitly (it's the "flattened tabindex-ordered focus navigation scope" of the Document). Granted, that explicit definition is recursive, and is composed through several definitions ("flattened tabindex-ordered focus navigation scope" derived from "tabindex-ordered focus navigation scope" derived from "focus navigation scope"). But decomposing big definitions into sub-definitions, just like decomposing big functions into sub-functions, is a useful and valid technique. I think any attempts to flatten the definitions further would probably just create more complexity.

If I am understanding correctly, some of the comments on the PR indicate that @rniwa might find it clearer if we moved entirely away from the "big list of focusable areas" model that the spec currently uses, instead having a "just in time" get-next-focusable-area algorithm. I think that is too big of a change to tackle right now. Even putting aside shadow trees, it's not at all clear how to translate the current rules in https://html.spec.whatwg.org/#the-tabindex-attribute section into something that would work in that fashion. If we think that's desirable, I'd rather we pursue that separately, on top of this PR and its improvements on the existing infrastructure. It could be informed by work that WebKit does to implement this spec in its infrastructure.

Anyway, I hope this spec is clear enough to review. I'd love to get this merged ASAP, as from what I understand we have multi-implementer interest in having shadow DOM work with sequential focus navigation, and this just gets the basics (no delegatesFocus) in place. Let me know if that sounds reasonable, @rniwa and @smaug----. And maybe also check out the tests at web-platform-tests/wpt#17523. (Of course, if you find problems during your review, we need to fix those first.)

[rniwa]

Yeah, I agree turning this into more of on-demand algorithm can be done in a separate patch. The new definition is already miles better than the old algorithm which operated on a set of abstract lists.

[rakina]

Thanks a lot @domenic! Everything looks great :)

[domenic]

So I'm thinking of merging this within the next day or two, along with the accompanying tests. Let me know if anyone wants to delay to review further. I imagine we might encounter some minor issues as implementations start happening, but we can fix those as we discover them; I don't think we'll be able to do much more by just reviewing a PR in the abstract.

We'll hold off on filing bugs until the second PR @rakina mentions (about delegatesFocus + focus() integration) lands, so there's a single bug for each browser covering all of the focus + shadow DOM stuff.

[domenic]

The focusable area thing needs to be made precise. Once it is, here is a draft commit message:

This defines an explicit list for the document's "sequential focus
navigation order", whose contents are defined to include elements in
shadow trees. Previously the contents of the sequential focus navigation
order were defined mostly implicitly, in the tabindex section.

This also expands the ordering requirements for sequential focus
navigation order to account for shadow trees and slotted elements.

Finally, this has a minor connection to delegatesFocus, in that it
expands the list of potential focusable areas to ... XXX what does it
do exactly?

Part of #2013.

Tests: https://github.com/web-platform-tests/wpt/pull/17523

[rniwa]

I really dislike this merging of two lists approach. I'd rather define a relationship between the relative order of things within each scope with its owner's scope. In practice, no browser is going to create these lists of elements and merge them so it's super confusing to define the order between elements this way.

[domenic]

Hi Ryosuke, we discussed this previously in #4735 (review). I think moving from a list that is only implicitly defined, to one that is explicitly defined, is a big step forward for the spec, and I'd like to move forward with it. Further editorial changes can happen in future PRs.

[smaug----]

I agree with @rniwa. I'd rather see this being closer to the implementations. But I can live with the list for now.

[domenic]

OK, LGTM! Commit message for when we get multi-implementer sign-off:

This defines an explicit list for the document's "sequential focus
navigation order", whose contents are defined to include elements in
shadow trees. Previously the contents of the sequential focus navigation
order were defined mostly implicitly, in the tabindex section.

This also expands the ordering requirements for sequential focus
navigation order to account for shadow trees and slotted elements.

Finally, this has a minor connection to delegatesFocus, in that it
excludes elements that are shadow hosts with delegatesFocus = true from
being focusable areas.

Part of #2013.

Tests: https://github.com/web-platform-tests/wpt/pull/17523

[smaug----]

I'd see some explanation for the template element handling and one needs to ensure all the algorithms deal with that. Perhaps easiest is to just make template explicitly not a focus navigation scope owner?

[rakina]

I'd see some explanation for the template element handling and one needs to ensure all the algorithms deal with that. Perhaps easiest is to just make template explicitly not a focus navigation scope owner?

Removed that part, thanks! Do you think this PR is ok now?

inadvertent template host treatment removed