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

Disabling tabindex across potentially many shadow roots #520

Closed
robdodson opened this issue Jun 9, 2016 · 14 comments
Closed

Disabling tabindex across potentially many shadow roots #520

robdodson opened this issue Jun 9, 2016 · 14 comments

Comments

@robdodson
Copy link

I'm working on an offscreen slide-in navigation panel which may contain several focusable elements and needs to animate at 60fps. To achieve the 60fps animation, the drawer has been promoted to a layer using will-change and transformed offscreen. Unfortunately this means all of the focusable children are still interactive.

I could set the drawer to visibility: hidden when it's offscreen (thus fixing the focus issue) but then I'd lose my fancy layer and sacrifice some of my animation perf. On a very large drawer (like the kind on m.facebook.com) this repainting could be very expensive.

An alternative is to do something like:

document.querySelectorAll('a[href], area[href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), button:not([disabled]), iframe, object, embed, [tabindex="0"], [contenteditable]');

And loop over all potentially focusable items, setting each to tabindex=-1 when the drawer is offscreen. This may be the faster of the two approaches but it gets difficult if any of those children contain Shadow DOM. Now I need to recurse through each shadow root and again look for either focusable children or more shadow roots.

So there's a few things at play here: how can I remove an entire shadow tree from the tab order? And, if there was such a mechanism, why not let me do it for regular trees as well so I don't need the crazy querySelector string?

cc @alice @paullewis @paulirish

@rniwa
Copy link
Collaborator

rniwa commented Jun 9, 2016

Did you really intend to file this on the issue tracker for W3C web components specifications?

The problem described here seems to be:

  • A bug in browser that focusable element makes animation slow even if it's not visible in the viewport
  • Something a library / framework should workaround for a specific browser.

@robdodson
Copy link
Author

@rniwa yeah @dglazkov asked me to file it here as part of it relates to Shadow DOM.

A bug in browser that focusable element makes animation slow even if it's not visible in the viewport

To clarify, the issue is there's no easy way to remove an entire shadow tree of elements from the tab order. It's my understanding (though I may be mistaken) that setting tabindex=-1 on the host would not remove the children from the tab order.

The animation use case is an example to demonstrate when someone would encounter this issue.

@rniwa
Copy link
Collaborator

rniwa commented Jun 10, 2016

Now that I think about this, I don't know why we decided tabindex=-1 on a shadow host should behave like tabindex=0.

@hayatoito @annevk

@hayatoito
Copy link
Contributor

hayatoito commented Jun 10, 2016

To clarify, the issue is there's no easy way to remove an entire shadow tree of elements from the tab order. It's my understanding (though I may be mistaken) that setting tabindex=-1 on the host would not remove the children from the tab order.

Yeah, for v0, it was what the spec said, however, we have changed the spec for v1 so that we can skip.

@TakayoshiKochi
I remember that we have decided to skip entire shadow subtrees if shadow host has tabindex=-1 for v1.
Could you check the spec?

@TakayoshiKochi
Copy link
Member

I'm not sure what " tabindex=-1 on a shadow host should behave like tabindex=0 " means.

Without delegatesFocus=true, a shadow host with tabindex=-1, the host and all its children
will be skipped for tab focus navigation.
It is still clickable/focusable (in that sense only, it is same as tabindex=0).

@TakayoshiKochi
Copy link
Member

TakayoshiKochi commented Jun 10, 2016

I don't understand why interactive elements slows animation.
And putting tabindex=-1 on a element, makes the element skipped for tab navigation, but
confusingly, it is still focusable. I'm not sure why you thought putting it would contribute to
smoother animation.

(edit)
so the problem is not the animation performance but if TAB key is hit, some element in the animating layer is visited, - a good way to disable all the focusable elements at once, hopefully. Is my understanding correct?

@hayatoito
Copy link
Contributor

hayatoito commented Jun 10, 2016

Given,

http://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation
Step 3. 1.2.1

if ELEMENT is focusable, a shadow host, or a slot element, append ELEMENT to NAVIGATION-ORDER.

and

http://w3c.github.io/webcomponents/spec/shadow/#dfn-focus-navigation-order-merging-algorithm
Step 4. 2.2:

Apply the focus navigation order merging algorithm with the focus navigation order owned by its shadow root as input, then append the result to MERGED-NAVIGATION-ORDER.

To me, it looks that we do not skip a shadow tree . Step 4 2.2 is always applied, even if the shadow host's tabindex=-1, isn't it?
If so, we should fix the spec.

@hayatoito
Copy link
Contributor

hayatoito commented Jun 10, 2016

I have found that Step3. 1.3 says:

Reorder NAVIGATION-ORDER according to the tabindex value attached to each node. In this step, an element whose tabindex value is negative must not be appended to NAVIGATION-ORDER.

Thus, the spec looks correct to me.
However, you can say "an element whose tabindex value is negative must not be appended to NAVIGATION-ORDER." in 1.2.1, instead of 1.3. That's the reason I was confused. :)

@hayatoito
Copy link
Contributor

hayatoito commented Jun 10, 2016

According to the current spec, a focusable element in a shadow tree whose host's tabindex = -1 is skipped from a sequential focus navigation (by pressing tab-key).

However, it does not mention anything that such a focusable element in a shadow tree should be considered 'non-focusable' elements. That's out of the scope in the current spec.

To support that, I guess we need another new feature (new attribute?).

@robdodson
Copy link
Author

a good way to disable all the focusable elements at once, hopefully. Is my understanding correct?

@TakayoshiKochi yes. I was not saying that focusable elements affect performance of animations. I was saying that the only ways I currently know of to disable focus on all elements are either:

  1. Make the element visibility: hidden or display: none. This does not work in my case because I'm trying to animate a layer so it would negatively impact performance to remove that layer from the render tree.
  2. Set tabindex=-1 on all children of the element. This should hopefully not cause repainting but is hard to do because (as I understood it) if any of those children contained shadow dom I would need to pierce their shadow boundaries and set all of their focusable children to tabindex=-1

If setting tabindex=-1 on a shadow host in shadow dom v1 will remove its children from the tab order then I think I'm happy :)

@hayatoito
Copy link
Contributor

hayatoito commented Jun 10, 2016

If setting tabindex=-1 on a shadow host in shadow dom v1 will remove its children from the tab order then I think I'm happy :)

Yes, that should be. At least, that's our intention in v1. They are removed from the tab order, but they are still focusable, I think. (Please correct me if I am wrong). Is that okay?

Regarding Blink's implementation, let me test the current behavior soon.

@annevk
Copy link
Collaborator

annevk commented Jun 10, 2016

@robdodson did you see whatwg/html#897? This seems similar to that proposal (and that one is not shadow tree specific).

@LJWatson
Copy link

@hayatoito
"Yes, that should be. At least, that's our intention in v1. They are removed from the tab order, but they are still focusable, I think. (Please correct
me if I am wrong). Is that okay?"

This mirrors the behaviour for tabindex="-1" in the HTML5.1 spec. It would make sense for tabindex to be consistent with that I think.

@robdodson
Copy link
Author

@hayatoito that's good to hear thank you :)

@annevk yep I'm on that thread. I think what I'm referring to is a little different, it's closer to the proposed inert attribute but that discussion isn't specific to web components so we can close this issue. I just wanted to make sure Shadow DOM and tabindex played nice together :)

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

6 participants