Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Focus Navigation for slots #375

Closed
rniwa opened this issue Jan 28, 2016 · 17 comments
Closed

Focus Navigation for slots #375

rniwa opened this issue Jan 28, 2016 · 17 comments

Comments

@rniwa
Copy link
Collaborator

rniwa commented Jan 28, 2016

We should update the section 6.2 Focus Navigation to reflect the discussion we had during TPAC last year.

I think what we want is akin to putting nodes inside a shadow DOM and a slot into its own nested browsing context. We should probably edit the HTML spec to create some sort of scoping mechanism, and make shadow DOM / slot element / browsing context use that concept.

@rniwa
Copy link
Collaborator Author

rniwa commented Jan 28, 2016

Could @hober or @annevk refactor the HTML spec as needed?

@hayatoito
Copy link
Contributor

I'm interested in how this new feature would interact with the proposal in #126.

If either of them is an backward incompatible feature, we can not ship Shadow DOM v1 unless both are resolved.

@TakayoshiKochi
Could you lead the discussion? I'd like to know how we should handle the proposal in issue #126.

@rniwa
Copy link
Collaborator Author

rniwa commented Jan 29, 2016

I don't think they're incompatible with one another since we still need to define the behavior when delegateFocus is missing.

@hayatoito
Copy link
Contributor

I guess @TakayoshiKochi has an opinion about this.
To me, they look apparently relevant if deleteFocus is enabled since either of them tries to define how tab focus navigation should work.

@TakayoshiKochi
Copy link
Member

The discussion at TPAC was that basically we navigate focusable elements via TAB presses in a way that those within the same shadow root including distributed nodes are grouped, and in order of composed tree?

e.g.

<my-nameinput>
    <#shadow-root>
         Firstname: <slot name="firstname"></slot>
         Lastname: <slot name="lastname"></slot>
    </#shadow-root>
    <input id="first" slot="firstname">
    <input id="last" slot="lastname">
</my-nameinput>

will be rendered as

<my-nameinput>
     Firstname: <input id="first">
     Lastname: <input id="last">
</my-nameinput>

But if the order in the #shadow-root is flipped, rendering will be

<my-nameinput>
     Lastname: <input id="last">
     Firstname: <input id="first">
</my-nameinput>

With the current spec, TAB navigation order is fixed for both, #first -> #last, as in the order of
the document DOM tree. But preferably, the order should be #first -> #last for the former and #last -> #first for the latter.

Is this the bottomline?
I feel we can define this without nested browsing context, just with composed tree.

(but once tabIndex is involved, things may get complicated, like #126)

@rniwa
Copy link
Collaborator Author

rniwa commented Feb 2, 2016

Right, we want #first -> #last for the former and #last -> #first for the latter.

We're definitely not defining a nested browsing context because that'd be a wrong semantics. The reason I bring that up is that we need some sort of grouping/scoping mechanism for each shadow root as well as each slot (e.g. if tabindex was set on #first and #last, that shouldn't affect the order by which they're tab-focused). Since the HTML5 spec currently treats elements in each nested browsing context as if it were in such a scope, we should refactor the spec to extract that concept and make the concept of browsing context use it instead of the focus navigation section directly referring to browsing context. I've already asked @hober to do this and it looks like @LJWatson made w3c/html#44 to track this.

@rniwa
Copy link
Collaborator Author

rniwa commented Feb 2, 2016

Let's say we have the following DOM (nobody should be setting tabindex like this here but I can't come up with a better example for now):

<contect-card>
    Name first: <input id="first" slot="name" tabindex="1">
    Middle name: <input id="middle" slot="name" tabindex="4">
    Last name: <input id="last" slot="name" tabindex="2"><br>
    Email: <input id="email" tabindex="3">
</contect-card>
<input id="search" type="search" tabindex="5">

and let's say contect-card's shadow DOM is defined as follows:

<dl>
    <dt>Email</dt>
    <dd><slot name="email"></slot></dd>
    <dt>Name</dt>
    <dd><slot name="name"></slot></dd>
</dl>

Then the composed three will look like

<my-nameinput>
    <dl>
        <dt>Email</dt>
        <dd><input id="first" slot="name" tabindex="1"
        ><input id="middle" slot="name" tabindex="4"
        ><input id="last" slot="name" tabindex="2"></dd>
        <dt>Name</dt>
        <dd><input id="email" tabindex="3"></dd>
    </dl>
</my-nameinput>
<input id="search" type="search" tabindex="5">

In this composed tree, the tab focusing order should be #first, #last, #middle, #email, then #search because shadow root and slot each defines its own focus scope/group.

@LJWatson
Copy link

LJWatson commented Feb 2, 2016

@rniwa

This approach makes sense I think. Just thinking through some of the possibilities though...

If the DOM looked like this instead:

<contect-card>
    Name first: <input id="first" slot="name" tabindex="1">
    Middle name: <input id="middle" slot="name">
    Last name: <input id="last" slot="name"><br>
    Email: <input id="email" tabindex="3">
</contect-card>

Using the same shadow DOM you described, leading to the following composed tree:

<my-nameinput>
    <dl>
        <dt>Name</dt>
        <dd><input id="first" slot="name" tabindex="1">
        ><input id="middle" slot="name">
        ><input id="last" slot="name"></dd>
        <dt>Email</dt>
        <dd><input id="email" tabindex="3"></dd>
    </dl>
</my-nameinput>

The tab order would be #first, #middle, #last, then #email? In other words would elements without tabindex also be scoped to the slot and/or shadow root?

@rniwa
Copy link
Collaborator Author

rniwa commented Feb 2, 2016

Yes. Just like nested browsing context, elements will be ordered first by the descending order of tabindex value and then the tree order.

Just as another example, if the DOM looked like:

<contect-card>
    Name first: <input id="first" slot="name" tabindex="2">
    Middle name: <input id="middle" slot="name">
    Last name: <input id="last" slot="name" tabindex="1"><br>
    Email: <input id="email" tabindex="3">
</contect-card>

and the composed tree looked like:

<contect-card>
    <dl>
        <dt>Name</dt>
        <dd><input id="first" slot="name" tabindex="2">
        ><input id="middle" slot="name">
        ><input id="last" slot="name" tabindex="1"></dd>
        <dt>Email</dt>
        <dd><input id="email" tabindex="3"></dd>
    </dl>
</contect-card>

instead, then the focus navigation order will be #last, #first, #middle, then #email.

@hayatoito
Copy link
Contributor

I like an idea that each slot has a scope as well as a component tree has.
I think this approach is doable and spec-able. I support it.

A couple of thoughts:

  • I recommend not to use the term of "order of the composed tree" because it does not reflect the approach correctly.
  • I recommend not to use the composed tree to explain the order because the composed tree is the result of flatting scopes. Its difficult to see in what order elements are traversed there because a tabindex should be encapsulated in each scope.
  • It would be better to consider the case where a slot is assigned to another slot.
  • The following is the behavior what I am expecting, using more comprehensive tree of trees:

e.g.

document tree:

  <input id=i0 tabindex=0></input>
  <x-foo tabindex=0>
    <input id=i2 slot=s1 tabindex=2></input>
    <input id=i1 slot=s1 tabindex=1></input>
  </x-foo>

<x-foo>'s shadow tree:

  <x-bar tabindex=4>
    <input id=j1 slot=s2 tabindex=1></input>
    <slot id=s1 name=s1 slot=s2></slot>
    <input id=j0 slot=s2 tabindex=0></input>
    <input id=j2 slot=s2 tabindex=2></input>
  </x-bar>
  <input id=j4 tabindex=4></input>
  <input id=j3 tabindex=3></input>

<x-bar>'s shadow tree:

  <input id=k0 tabindex=0></input>
  <slot id=s2 name=s2></slot>
  <input id=k1 tabindex=1></input>

The sequential focus navigation in each scope will be:

  • document tree: [i0 -> [x-foo]]
  • x-foo's shadow tree: [j3 -> [x-bar] -> j4]
  • slot #s1: [i1 -> i2]
  • x-bar's shadow tree: [k1 -> k0 -> [s2]]
  • slot #s2: [j1 -> j2 -> [s1] -> j0]

Thus, if flattened, the (global) sequential focus navigation would be: [i0 -> j3 -> k1 -> k0 -> j1 -> j2 -> i1 -> i2 -> j0 -> j4]

Additional question: Should we honor the tabindex value of a slot element? The motivation is to control where a slot's scope is inserted in the enclosing scope. e.g. We might want to control where s1's scope, [s1], is inserted in [s2].

In the example, a slot's scope was inserted into the enclosing scope as if the slot element had tabindex=0.

A slot element itself is NOT focusable even if it has a tabindex. That does not change. A slot element is not a focusable by definition because it does not participate in the composed tree.

My preference is that we should honor the tabindex value of a slot element so that users can control the position of a slot's scope in the enclosing scope.

[Update]
I'm assuming that x-foo element itself, as well as x-bar, does not join the focus navigation here. See #126 for details.

@TakayoshiKochi
Copy link
Member

Note that in the above example, if you put tabindex attribute in <x-bar>, x-bar itself becomes
focusable, and focus stops at x-bar, then the navigation gets inside x-bar. The delegatesFocus
flag controls the behavior, whether the focus navigation slips through the element or not.
I feel like it is natural to turn it on by default for a shadow host, and can be turned of if requested, as some component may want to take focus as a whole.

@TakayoshiKochi
Copy link
Member

I support the idea of honoring <slot>'s tabIndex in the bottom of #375 (comment) .

@TakayoshiKochi
Copy link
Member

@rniwa in #375 (comment) 's composed tree example, as <my-nameinput> doesn't have tabindex, I guess #search is visited first, then dive inside <my-nameinput>, because in that scope there are only <my-nameinput> and <input> and even we implicitly treat <my-nameinput>'s scope order as tabindex=0.

So even if you were just a web author using the third-party <contact-card> component, you have to be aware that there are separate tabindex scopes, one in <contact-card> and one in the main document.
If it's <contact-card>, it can be imaginable that the element is probably a custom element with shadow DOM, but in the case when it is a <div> with a shadow DOM, it will be hard to distinguish scopes at looking at the main document source alone?

Let me think more...

@hayatoito
Copy link
Contributor

I agree that introducing scope for a slot is surprising for component users in some cases.

In fact, that's exactly the reason I could not take this approach in the past in Blink: This approach would break tabindex semantics for <details>/<summary> elements because <details>/<summary> is using Shadow DOM in Blink.

However, I think pros is much bigger than cons.

The cons happens in the limited case, I think, if the all of the following conditions are met:

  • A shadow host is not a custom element
  • A shadow host's child does not use a slot attribute, and it is assigned to the default slot

In other cases, developers can be aware that that "This tabindex is used in assigned slot's scope, rather than the current component tree's scope", I think.

Having said that, I still think that pros is much bigger than cons. If there is a way to avoid this cons somehow, it would be nice.

@TakayoshiKochi
Copy link
Member

Just for reference, the discussion about focus and tab order for <details>/<symmary> is here:
https://bugs.webkit.org/show_bug.cgi?id=92050

@rniwa
Copy link
Collaborator Author

rniwa commented Feb 4, 2016

Perhaps what you need is a new attribute on slot element like focusScope that decides whether to define a new scope or not.

TakayoshiKochi added a commit to TakayoshiKochi/webcomponents that referenced this issue Feb 12, 2016
As discussed in
WICG#375

This adds a clarification about focus navigation order for
nodes distributed under a slot element.

Also added handling tabindex="-1" case.
@hayatoito
Copy link
Contributor

I think this is a blocking issue for v1. @TakayoshiKochi is working on having a formal definition in #381, I think.

@hayatoito hayatoito changed the title Update Section 6.2 Focus Navigation to reflect TPAC discussion Focus Navigation for slots Mar 17, 2016
TakayoshiKochi added a commit to TakayoshiKochi/webcomponents that referenced this issue Mar 18, 2016
As discussed in
WICG#375

This adds a clarification about focus navigation order for
nodes distributed under a slot element.

Also added handling tabindex="-1" case.
TakayoshiKochi added a commit to TakayoshiKochi/webcomponents that referenced this issue Mar 18, 2016
As discussed in
WICG#375

This adds a clarification about focus navigation order for
nodes distributed under a slot element.

Also added handling tabindex="-1" case.
TakayoshiKochi added a commit to TakayoshiKochi/webcomponents that referenced this issue Mar 18, 2016
As discussed in
WICG#375

This adds a clarification about focus navigation order for
nodes distributed under a slot element.

Also added handling tabindex="-1" case.
TakayoshiKochi added a commit to TakayoshiKochi/webcomponents that referenced this issue Mar 18, 2016
As discussed in
WICG#375

This adds a clarification about focus navigation order for
nodes distributed under a slot element.

Also added handling tabindex="-1" case.
kisg pushed a commit to paul99/webkit-mips that referenced this issue May 16, 2016
https://bugs.webkit.org/show_bug.cgi?id=151379

Reviewed by Antti Koivisto.

Source/WebCore:

Implemented the sequential focus navigation ordering as discussed on
WICG/webcomponents#375

New behavior treats each shadow root and slot as a "focus scope". The focus navigation ordering
is defined within each "focus scope" using tabindex, treating any "focus scope owner"
(e.g. shadow host or a slot) as if it was having tabindex=0 if it wasn't itself focusable.

This patch modifies FocusNavigationScope to support a focus scope defined for a slot element in
addition to the one defined for a shadow tree and a document as previously supported.

Tests: fast/shadow-dom/focus-across-details-element.html
       fast/shadow-dom/focus-navigation-across-slots.html

* dom/Node.cpp:
(WebCore::parentShadowRoot): Extracted from assignedSlot.
(WebCore::Node::assignedSlot):
(WebCore::Node::assignedSlotForBindings): Added.
* dom/Node.h:
* dom/NonDocumentTypeChildNode.idl:
* html/HTMLDetailsElement.h:
(HTMLDetailsElement::hasCustomFocusLogic): Added. Don't treat details element as a "focus scope".
* html/HTMLSummaryElement.h:
(HTMLSummaryElement::hasCustomFocusLogic): Ditto for summary element.
* page/FocusController.cpp:
(WebCore::hasCustomFocusLogic): Moved.
(WebCore::isFocusScopeOwner): Added. Returns true on a shadow host without a custom focus logic or
on a slot inside a shadow tree whose shadow host doesn't have a custom focus logic.
(WebCore::FocusNavigationScope::firstChildInScope): Now takes a reference. Call isFocusScopeOwner
to check for both slots and shadow roots instead of just the latter. This fixes a subtle bug that
focus may never get out of textarea in some cases due to its failure to check hasCustomFocusLogic.
(WebCore::FocusNavigationScope::lastChildInScope): Ditto.
(WebCore::FocusNavigationScope::parentInScope): Made this a member function since it needs to check
against m_slotElement inside the focus scope of a slot.
(WebCore::FocusNavigationScope::nextSiblingInScope): Added. Finds the next assigned node in a slot
in the focus scope defined for a slot. Just calls nextSibling() in the focus scope for shadow tree
and document.
(WebCore::FocusNavigationScope::previousSiblingInScope): Ditto for finding the previous sibling.
(WebCore::FocusNavigationScope::firstNodeInScope): Added. This function replaces rootNode() which
doesn't exist for the focus scope of a slot element.
(WebCore::FocusNavigationScope::lastNodeInScope): Ditto for the last node.
(WebCore::FocusNavigationScope::nextInScope):
(WebCore::FocusNavigationScope::previousInScope):
(WebCore::FocusNavigationScope::FocusNavigationScope): Added a variant that takes HTMLSlotElement.
(WebCore::FocusNavigationScope::owner): Added the support for slot elements.
(WebCore::FocusNavigationScope::scopeOf): Ditto.
(WebCore::FocusNavigationScope::scopeOwnedByScopeOwner): Ditto.
(WebCore::isFocusableElementOrScopeOwner): Added the support for slot elements and renamed from
isFocusableOrHasShadowTreeWithoutCustomFocusLogic.
(WebCore::isNonFocusableScopeOwner): Ditto. Renamed from isNonFocusableShadowHost.
(WebCore::isFocusableScopeOwner): Ditto. Renamed from isFocusableShadowHost.
(WebCore::shadowAdjustedTabIndex): Added the support for slot elements.
(WebCore::FocusController::findFocusableElementAcrossFocusScope):
(WebCore::FocusController::nextFocusableElementWithinScope):
(WebCore::FocusController::previousFocusableElementWithinScope):
(WebCore::FocusController::findElementWithExactTabIndex):
(WebCore::nextElementWithGreaterTabIndex): Call firstNodeInScope() instead of rootNode() here since
there is no root node for the focus scope defined for a slot element.
(WebCore::previousElementWithLowerTabIndex): Ditto for scope.lastNodeInScope().
(WebCore::FocusController::nextFocusableElementOrScopeOwner):
(WebCore::FocusController::previousFocusableElementOrScopeOwner):
(WebCore::parentInScope): Deleted.
(WebCore::FocusNavigationScope::rootNode): Deleted.
(WebCore::FocusNavigationScope::scopeOwnedByShadowHost): Deleted.
(WebCore::isNonFocusableShadowHost): Deleted.
(WebCore::isFocusableShadowHost): Deleted.
(WebCore::isFocusableOrHasShadowTreeWithoutCustomFocusLogic): Deleted.

LayoutTests:

Added regression tests for moving focus by tab and shift+tab across
user-defined shadow trees with slots and details element.

* fast/shadow-dom/focus-across-details-element-expected.txt: Added.
* fast/shadow-dom/focus-across-details-element.html: Added.
* fast/shadow-dom/focus-navigation-across-slots-expected.txt: Added.
* fast/shadow-dom/focus-navigation-across-slots.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@200964 268f45cc-cd09-0410-ab3c-d52691b4dbfc
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=151379

Reviewed by Antti Koivisto.

Source/WebCore:

Implemented the sequential focus navigation ordering as discussed on
WICG/webcomponents#375

New behavior treats each shadow root and slot as a "focus scope". The focus navigation ordering
is defined within each "focus scope" using tabindex, treating any "focus scope owner"
(e.g. shadow host or a slot) as if it was having tabindex=0 if it wasn't itself focusable.

This patch modifies FocusNavigationScope to support a focus scope defined for a slot element in
addition to the one defined for a shadow tree and a document as previously supported.

Tests: fast/shadow-dom/focus-across-details-element.html
       fast/shadow-dom/focus-navigation-across-slots.html

* dom/Node.cpp:
(WebCore::parentShadowRoot): Extracted from assignedSlot.
(WebCore::Node::assignedSlot):
(WebCore::Node::assignedSlotForBindings): Added.
* dom/Node.h:
* dom/NonDocumentTypeChildNode.idl:
* html/HTMLDetailsElement.h:
(HTMLDetailsElement::hasCustomFocusLogic): Added. Don't treat details element as a "focus scope".
* html/HTMLSummaryElement.h:
(HTMLSummaryElement::hasCustomFocusLogic): Ditto for summary element.
* page/FocusController.cpp:
(WebCore::hasCustomFocusLogic): Moved.
(WebCore::isFocusScopeOwner): Added. Returns true on a shadow host without a custom focus logic or
on a slot inside a shadow tree whose shadow host doesn't have a custom focus logic.
(WebCore::FocusNavigationScope::firstChildInScope): Now takes a reference. Call isFocusScopeOwner
to check for both slots and shadow roots instead of just the latter. This fixes a subtle bug that
focus may never get out of textarea in some cases due to its failure to check hasCustomFocusLogic.
(WebCore::FocusNavigationScope::lastChildInScope): Ditto.
(WebCore::FocusNavigationScope::parentInScope): Made this a member function since it needs to check
against m_slotElement inside the focus scope of a slot.
(WebCore::FocusNavigationScope::nextSiblingInScope): Added. Finds the next assigned node in a slot
in the focus scope defined for a slot. Just calls nextSibling() in the focus scope for shadow tree
and document.
(WebCore::FocusNavigationScope::previousSiblingInScope): Ditto for finding the previous sibling.
(WebCore::FocusNavigationScope::firstNodeInScope): Added. This function replaces rootNode() which
doesn't exist for the focus scope of a slot element.
(WebCore::FocusNavigationScope::lastNodeInScope): Ditto for the last node.
(WebCore::FocusNavigationScope::nextInScope):
(WebCore::FocusNavigationScope::previousInScope):
(WebCore::FocusNavigationScope::FocusNavigationScope): Added a variant that takes HTMLSlotElement.
(WebCore::FocusNavigationScope::owner): Added the support for slot elements.
(WebCore::FocusNavigationScope::scopeOf): Ditto.
(WebCore::FocusNavigationScope::scopeOwnedByScopeOwner): Ditto.
(WebCore::isFocusableElementOrScopeOwner): Added the support for slot elements and renamed from
isFocusableOrHasShadowTreeWithoutCustomFocusLogic.
(WebCore::isNonFocusableScopeOwner): Ditto. Renamed from isNonFocusableShadowHost.
(WebCore::isFocusableScopeOwner): Ditto. Renamed from isFocusableShadowHost.
(WebCore::shadowAdjustedTabIndex): Added the support for slot elements.
(WebCore::FocusController::findFocusableElementAcrossFocusScope):
(WebCore::FocusController::nextFocusableElementWithinScope):
(WebCore::FocusController::previousFocusableElementWithinScope):
(WebCore::FocusController::findElementWithExactTabIndex):
(WebCore::nextElementWithGreaterTabIndex): Call firstNodeInScope() instead of rootNode() here since
there is no root node for the focus scope defined for a slot element.
(WebCore::previousElementWithLowerTabIndex): Ditto for scope.lastNodeInScope().
(WebCore::FocusController::nextFocusableElementOrScopeOwner):
(WebCore::FocusController::previousFocusableElementOrScopeOwner):
(WebCore::parentInScope): Deleted.
(WebCore::FocusNavigationScope::rootNode): Deleted.
(WebCore::FocusNavigationScope::scopeOwnedByShadowHost): Deleted.
(WebCore::isNonFocusableShadowHost): Deleted.
(WebCore::isFocusableShadowHost): Deleted.
(WebCore::isFocusableOrHasShadowTreeWithoutCustomFocusLogic): Deleted.

LayoutTests:

Added regression tests for moving focus by tab and shift+tab across
user-defined shadow trees with slots and details element.

* fast/shadow-dom/focus-across-details-element-expected.txt: Added.
* fast/shadow-dom/focus-across-details-element.html: Added.
* fast/shadow-dom/focus-navigation-across-slots-expected.txt: Added.
* fast/shadow-dom/focus-navigation-across-slots.html: Added.


Canonical link: https://commits.webkit.org/175885@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@200964 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants