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 behavior of delegatesFocus:true will be confusing for users #554

Closed
ebidel opened this issue Aug 24, 2016 · 50 comments
Closed

:focus behavior of delegatesFocus:true will be confusing for users #554

ebidel opened this issue Aug 24, 2016 · 50 comments

Comments

@ebidel
Copy link

ebidel commented Aug 24, 2016

From here:

When any element in HOST's shadow tree has focus, :focus pseudo-class applies to HOST in addition to the focused element itself.

If :focus pseudo-class applies to HOST, and HOST is in a shadow root of another shadow host HOST2 which also delegates focus, :focus pseudo-class applies to HOST2 as well.

In practice, this behavior is very confusing. I'm not sure we have an example like it currently in the platform.

Consider http://jsbin.com/kuqepubihi/edit?html,output (Try in Chrome Canary for Custom element support). Tabbing into the <x-element> component puts the default outline ring around three items!

screen shot 2016-08-24 at 9 43 30 am

@ebidel
Copy link
Author

ebidel commented Aug 24, 2016

I'd like to better understand this feature and how we expect folks to use it. Is there a discussion somewhere? As it currently stands, I'm not sure advocating it to developers will be a good idea :\

cc @robdodson @alice

@alice
Copy link
Member

alice commented Aug 24, 2016

One idea I had previously was to only match :focus on focusable elements. Consider the infamous date input, which has a focus indicator on the focusable host, and also on the individual components of the date as appropriate.

@robdodson
Copy link

Related is the fact that clicking on an element in the Shadow DOM will find the nearest focusable element and apply focus to it.

So if I have

#shadow-root
  <p>Hello World</p>
  <input type="text">

And I click the p it will focus the input. That seems really strange.

@hayatoito
Copy link
Contributor

cc: @TakayoshiKochi , @sorvell, @sjmiles

Yeah, it looks very weird.

I remember that we had an internal meeting about how :focus should be with Polymer folks. This behavior is the conclusion of the meeting. I could not find any meeting notes which can be shareable, but I can say no one noticed this issue at that point.

I remember that the motivation of this feature is that users want to use :focus for a component if the component's shadow tree has a real focused element. But no one thought of such multiple :focus being applied. :(

Is there any good idea to fix this?

@alice
Copy link
Member

alice commented Aug 25, 2016

What about my idea to only match :focus if an element is itself focusable?

@TakayoshiKochi
Copy link
Member

TakayoshiKochi commented Aug 25, 2016

The original idea was :focus to be able to construct <input type="date">.

If an author uses custom <date-input>, then it can decorate the element date-input:focus { ... }
when any of the element inside <date-input> has focus.

And for <date-input> component author, within the component it can style its inner element
like span#time-field:focus { ... }.

So conceptually, the outermost element should be styled as if it were a regular HTML element,
regardless of the internal state (which specific element in the component is focused) via :focus.

The element with delegatesFocus=true shadow DOM is focusable, although .focus() or
tabbing behavior is customized.

I think the problem here is UA stylesheet's :focus { outline: ... } is applied to inner focusable
elements. So currently the advice would be to have a custom :focus { outline: none } or
something inside Shadow DOM's style.

@TakayoshiKochi
Copy link
Member

Here are some references:

@TakayoshiKochi
Copy link
Member

Re #554 (comment)

"nearest focusable element" is a bit inaccurate. If delegatesFocus=true, any click event
inside the component will delegate the focus to its shadow root, then the shadow root
forwards the focus to its first focusable element.

@hayatoito
Copy link
Contributor

Do you mean we should mark this issue WONTFIX unless we have a better proposal?

I appreciate a better proposal, if someone has, because "default outline ring around multiple items" is a very bad user experience. Given the current spec, it is not easy to avoid that. I am afraid that a workaround, overwriting UA's stylesheet by author stylesheet, does not always work.

@alice
Copy link
Member

alice commented Aug 26, 2016

I don't follow - is there an objection to my proposal?

@hayatoito
Copy link
Contributor

One idea I had previously was to only match :focus on focusable elements. Consider the infamous date input, which has a focus indicator on the focusable host, and also on the individual components of the date as appropriate.

Could you elaborate this? I am afraid that I do not understand the point fully. :(
Custom elements can be focusable elements.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 26, 2016

We might want to update the spec somehow:

http://w3c.github.io/webcomponents/spec/shadow/#focus

4. When any element in HOST's shadow tree has focus, :focus pseudo-class applies to HOST in addition to the focused element itself.
5. If :focus pseudo-class applies to HOST, and HOST is in a shadow root of another shadow host HOST2 which also delegates focus, :focus pseudo-class applies to HOST2 as well.

Does "removing 4 and 5 from the spec" sound good?
That looks the opposite of the conclusion of #126. We would lose the ability to create a custom element which can react ":focus".

I am not sure what is the best for us. :(

@hayatoito
Copy link
Contributor

cc: @rniwa, @annevk

@alice
Copy link
Member

alice commented Aug 26, 2016

Yes, custom elements may be focusable or not. But in my opinion, if the element is not itself focusable, it makes no sense for it to match :focus.

Two examples:

  • <input type=date>, re-created identically but as a custom element, named something like <date-input>. This would be focusable, and have focusable elements in its shadow DOM. So, the host would match :focus, as would whichever descendant inside of the shadow root which currently had focus (e.g. day, month or year). You can sensibly have two levels of focus indicator in this case.
  • <gh-sidebar>, as a composition of all the elements in the github sidebar (Labels, Milestone, etc.) which can be easily dropped into a page. This includes focusable elements, but is not itself focusable. It seems to me to make sense that this element should not match :focus when one of its descendants is focused, because it is not focusable itself.

@hayatoito
Copy link
Contributor

Yeah, we have two use cases, 1) <date-input> and 2) <x-sidebar>. It looks that the current spec honers only the use case of 1) <date-input>.

The problem is that there is no hint which tells us 1) or 2). I supposed that delegatesFocus=flag flag would be a hint, but it looks delegatesFocus=true may be used for both use cases, if my understanding is correct.

@alice
Copy link
Member

alice commented Aug 26, 2016

Supposing a shadow host didn't have any focusable ancestors, how would we determine whether it was a tab stop? Surely the same signal can be used to disambiguate (1) and (2)?

@hayatoito
Copy link
Contributor

Per http://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

Unless its shadow root’s delegatesFocus flag is set, append ELEMENT to MERGED-NAVIGATION-ORDER.

A delegatesFocus flag is used as a signal. If its shadow root's delegates flag is not set, it is top-stop-ed.

@alice
Copy link
Member

alice commented Aug 26, 2016

But that's for elements already in NAVIGATION-ORDER. How do they get in that set in the first place?

@hayatoito
Copy link
Contributor

hayatoito commented Aug 26, 2016

Ah, good point! I believe "element's tabindex should be set" in the first place.

cc: @TakayoshiKochi

@alice
Copy link
Member

alice commented Aug 26, 2016

Right, so unless tabindex is set, it's not focusable (unless setForceFocusable() is a thing). So would it make sense to say unless an element is focusable, it won't match :focus?

@hayatoito
Copy link
Contributor

Awesome. Your proposal sounds reasonable to me. I support it!

We should update 4 and 5 in the spec so that it also considers tabindex-sh.

@TakayoshiKochi , could you work on updating the spec?

If someone has a concern for this change, please let us know that.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 26, 2016

I would like to clarify expected results for each case:

<date-input tabindex=1>
  ::shadow-root (delegatesFocus=true)
</date-input>
<date-input>
  ::shadow-root (delegatesFocus=true)
</date-input>
<date-input tabindex=-1>
  ::shadow-root (delegatesFocus=true)
</date-input>
<date-input tabindex=1>
  ::shadow-root (delegatesFocus=false)
</date-input>
<date-input>
  ::shadow-root (delegatesFocus=false)
</date-input>
<date-input tabindex=-1>
  ::shadow-root (delegatesFocus=false)
</date-input>

Suppose that each shadow root has a focusable element, and it is focused,
what are expected results of "date-input:focus" for each case?

@alice
Copy link
Member

alice commented Aug 26, 2016

I would expect that if tabindex is absent, :focus would not match, regardless of the presence or value of delegatesFocus.

@hayatoito
Copy link
Contributor

Thanks.
Hmm. It might be difficult to support that because we lose the ability to create a custom element, like a <x-input>, which behaves like a built-in <input> element.

<input> element is focusable without explicitly specified tabindex. We would like to create a <x-input> (delegatesFocus=true) element which rects ":focus" without explicitly specified tabindex attribute.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 26, 2016

Ah, good point! I believe "element's tabindex should be set" in the first place.

Hmm. per the spec.

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.

I am not sure what "tabindex value" means here if a custom element does not have a tabindex attribute. It is considered zero, or negative?

@TakayoshiKochi
Copy link
Member

TakayoshiKochi commented Aug 26, 2016

I am not sure what "tabindex value" means here if a custom element does not have a tabindex. It is considered zero?

If the element has an explicit attribute, use its value, otherwise it has internal value (accessible
via JS .tabIndex property) which has 0 or -1, per its default focusable behavior.

At this point we don't allow shadow root on any default-focusable elements (e.g. <a> or <button>),
so it may not be a problem, but once we support custom element type extension (aka is=),
<button is="my-button"> is a custom element and can attach shadow root.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 26, 2016

What is the default value of an internal value for case 2 and 5?

@TakayoshiKochi
Copy link
Member

TakayoshiKochi commented Aug 26, 2016

For those with explicit tabindex attribute, the value is reflected.
For case 2, .tabIndex = 0, and for case 5, .tabIndex = -1.

(for 2, its shadow root's delegatesFocus = true makes the host focusable, thus internally
it is treated like tabindex=0)

@hayatoito
Copy link
Contributor

If tabindex value for case 5 is -1, I am afraid that the focusable elements in the shadow tree would not be merged to "document sequential focus navigation order". That looks a bug... :(

@hayatoito
Copy link
Contributor

hayatoito commented Aug 26, 2016

We can fix that as another spec issue. :) That should be an orthogonal issue, I think.

@TakayoshiKochi
Copy link
Member

We already have overloaded meaning for "tabindex" attribute (determine tab order, and making the element focusable), I'm reluctant to add another meaning of "making it match for :focus when
delegatesFocus=true shadow root is attached and inner element has focus".

IMHO we can do either

  • Preserve the current behavior as is
  • Remove the recursive :focus matching completely, and make the :focus only match to the element which has real focus

In the latter case, you lose the ability to make <input type="date">-compatible <date-input>, and
the component user loses the ability to style by :focus { ... } equally for native elements and custom
elements. On the other hand you can make sure only one element in a shadow-including document
matches :focus rule.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 26, 2016

Could someone summarize the proposals?
In each case 1 - 6, suppose that a focusable element in a shadow tree is focused,

Proposal A) keep the status quo.

  • date-input:focus is applied for case 1, 2, 3

Proposal B)

  • date-input:focus is not applied for any case.

Proposal C) (@alice ?)

  • date-input:focus is applied for case 1, 4

Is there any other proposal?

@alice
Copy link
Member

alice commented Aug 26, 2016

I don't think this actually overrides the meaning of tabindex. tabindex still makes things focusable, and this is keyed off of focusability, not tabindex. If setForceFocusable() made something focusable, that would also cause something to match :focus. So I don't think this is a valid concern.

@alice
Copy link
Member

alice commented Aug 26, 2016

Regarding the examples, my proposal would have date-input:focus match for 1, 3, 4 and 6. Edit: I was wrong about 1 and 3.

Focusable is not the same as being in the tab order. If you write <p tabindex=-1>Some text</p> and then click "some text", you will see a focus ring appear, but the <p> will not be in the focus order. Similarly, in the (6) case, calling .focus() or clicking on the <date-input> should cause it to become focused.

My read on delegatesFocus is that for a shadow host which would otherwise be focusable (e.g. <date-input tabindex=0>), it causes the host to become unfocusable, but for the elements in its shadow tree to become part of the composed focus order.

I agree it would be great to have a mechanism for making a custom element focusable without an explicit tabindex (and I have given that feedback multiple times), but I think that's orthogonal to this issue.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 29, 2016

Thanks. I guess there are a lot of different opinions on this issue. We should be careful.

My understanding of this issue is that:

  • In case of "delegatesFocus=false", which has been the default behavior for a long time,
    we do not have any special treatment for a shadow host, in terms of ":focus". It's just a parent (ancestor) tag. Nothing special.

    That means we should have the same behavior to what we have when we expand a shadowhost-shadowtree to the flat tree.

    Suppose if we have the following markup,

    <date-input>
      <input>
    </date-input>

    In this case, even if a child <input> is focused, <date-input> is never affected, in terms of ":focus".
    That has been the default behavior.

  • Because the previous behavior is not useful sometimes, we introduced a new flag, "delegatesFocus=true", so that we do not break existing behavior. In case of "delegatesFocus=true", a shadow host should be special, like an encapsulated component, in term of ":focus".

    That is all I know. I hope other folks are adding more comments on this issue.

@TakayoshiKochi
Copy link
Member

TakayoshiKochi commented Aug 29, 2016

@hayatoito thanks for summarizing the issue.

delagatesFocus=true is special in that it makes its shadow host be in the focus navigation order, without stopping at the host itself but relays the focus into its shadow - and gets back the focus status from its shadow to match :focus - that's where "delegate" comes from.

I am wondering @alice 's <gh-navbar> element ever needs delegatesFocus=true, as it does not have to trap focus as a component. If it did, to control order of focus navigation (e.g. top navigation -> comment thread -> side navbar), it would have a tabindex attribute, but then you would have focus ring around it (if you propose :focus should match delegatesFocus=true shadow hosts with tabindex attribute)?

Probably for such an element, you can have :host(:focus) { outline: none } from its shadow root to specifically cancel drawing focus ring on the host (in contrast to what I commented in #554 (comment) to cancel focus ring inside shadow root).

@alice
Copy link
Member

alice commented Aug 29, 2016

Ah right, thanks for clarifying @TakayoshiKochi! I agree, <gh-navbar> shouldn't need delegatesFocus = true since it should never itself be focusable.

So I guess I don't completely understand what should happen if a non-focusable element (like in @ebidel's example) has a shadow root attached with delegatesFocus = true.

It seems like, as specced, delegatesFocus = true implicitly makes the host quasi-focusable - i.e., matches :focus, has special focusing behaviour when one of its descendants is clicked. I think this is the piece that I hadn't completely understood - my expectation was that delegatesFocus would have no effect unless the host element was explicitly focusable.

I think with the understanding that delegatesFocus = true has this special "quasi-focusable" implication, the current behaviour does make sense. However, if this is surprising to others, might we want to revisit that implication?

@TakayoshiKochi
Copy link
Member

@alice - thanks for the comment, I think we are now on the same page.

In my mental model, a shadow host with delegatesFocus=true is explicitly declared focusable, as a whole component. You can focus() on the element, you can click on any part of the element to focus the element, and once it gets focus it matches the :focus selector. The difference is that it delegates focus to shadow root.

As you pointed out, delegatesFocus = true makes the shadow host focusable but the behavior is somewhat different from regular focusable element. This is new, so it can be possible that developers don't expect the behavior.

Once you are educated, are you happy and can you live with the current delegatesFocus=true behavior? If so, I think this is a problem of explaining the new behavior to more general public (than spec readers / browser implementers).

@alice
Copy link
Member

alice commented Aug 30, 2016

Yup, the current behaviour now makes sense to me.

@ebidel What do you think? Could we come up with a more real-world example for the primer to explain this behaviour in a less confusing way?

@ebidel
Copy link
Author

ebidel commented Aug 30, 2016

I think a better example would be great, especially one that still maintains how to explain all the nuanced behavior. Open to ideas if anyone has any.

@alice
Copy link
Member

alice commented Aug 30, 2016

@ebidel <date-input>?

@hayatoito
Copy link
Contributor

Can we expect that a workaround , "disable UA's :focus style for builtin-elements by an author style", always work? It it works, I am okay to close this issue.

@TakayoshiKochi
Copy link
Member

UA stylesheet's :focus { ... } can be overridable in either way:

From outside: your-element:focus { outline: none }
From inside shadow: :host(:focus) { outline: none }

(UA stylesheet is the least precedence in cascading order)
So either a component user or a component author can specify the expected behavior for the component outline style. When a component specifies the style for its host's outline, a user can still override from outside.

outline property doesn't inherit, which is good because these rules above can specifically target an element, and it does not affect other descendants accidentally.

I think it's okay to close the issue.

@hayatoito
Copy link
Contributor

hayatoito commented Sep 1, 2016

Is it guaranteed that UA stylesheet does not use !important rule for ":focus" in all browsers? That can not be overridden.

@hayatoito hayatoito reopened this Sep 1, 2016
@TakayoshiKochi
Copy link
Member

Hmm, if UA stylesheet has !important, author !important cannot override according to cascading order, but isn't it a UA bug (or policy, maybe)?
If UA stylesheet had :focus { ... !important}, an author could not style like e.g. input:focus { ... !important } regardless of Shadow DOM or delegatesFocus flag.

What is the point of guaranteeing it is overridable even for !important UA rules?

@hayatoito
Copy link
Contributor

We rely on the UA stylesheet not being overridable for guarantees that are necessary for security and correctness.

@TakayoshiKochi
Copy link
Member

So we cannot close this issue until we have some way to override the focus ring behavior when UA uses :focus { ... !important } ?

I'm not sure whether such use cases happen, and focus ring decoration shouldn't imply any security, so maybe for accessibility correctness?

I'm not sure for such cases actually happen, but considering from different angle that the default :focus behavior for delegatesFocus=true is surprising to authors/users is the original concern, we may change the default behavior.

Here are options I have, in the order of my preference.

  1. Add a flag (e.g. add matchesPseudoFocus=true to shadow root option, defaulting false and decouple the meaning from delegatesFocus=true) to control the :focus matching behavior.
  2. Abandon :focus matching for shadow hosts with delegatesFocus=true so that the problem should not happen.
  3. Having a normative sentence in the spec so that UA stylesheet should not use :focus { outline: ... !important } to guarantee it is overridable.

As we have a use case for matching :focus (<date-input>), I would like to avoid 2, and 3 seems somewhat weird.

@alice @ebidel @robdodson @rniwa @annevk any thoughts?

@hayatoito
Copy link
Contributor

So we cannot close this issue until we have some way to override the focus ring behavior when UA uses :focus { ... !important } ?

That is not my point. We should not provide any way to override UA's !important rule. That's the bottom line.

The point is that we should make it clear that there is a case where web developers can not control fully, before closing this issue with the wrong assumption.

I prefer the status quo as is, instead of any of 1, 2 or 3.

@TakayoshiKochi
Copy link
Member

Ah, okay, thanks for the clarification. Scratch my few comments above.

So does the conclusion below look good?


When you specify delegatesFocus=true on a shadow root, its shadow host matches :focus pseudo class when any shadow-including descendant of the shadow root has focus.
This means you can see multiple focus rings, if some style rule gives outline for `:focus'.

This can be avoided by giving an explicit style rule like:

From outside shadow: your-element:focus { outline: none }
From inside shadow: :host(:focus) { outline: none }

When both a component and its user specify the style, their precedence will be determined by shadow cascading rule.

CSS outline property doesn't inherit, which is good because those rules do not affect other descendants accidentally.

Note: whether the style rule like above applies or not follows the rule of cascading, and for a rule in UA stylesheet with !important, for example, can have higher precedence and you may not be able to override.

@hayatoito
Copy link
Contributor

Let's close this status quo.

devtools-bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this issue Apr 27, 2020
This CL disables the focus delegation when creating
a shadow root for devices list to work around an
issue with focus being set on a wrong element when
clicking some other element [1].

[1] WICG/webcomponents#554 (comment)

Bug: 1075414
Change-Id: If5aad7f01a7c13cf095a73c12432661a44e4c771
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2165788
Reviewed-by: Peter Marshall <[email protected]>
Reviewed-by: Mathias Bynens <[email protected]>
Commit-Queue: Alex Rudenko <[email protected]>
babot pushed a commit to binaryage/dirac that referenced this issue Apr 28, 2020
This CL disables the focus delegation when creating
a shadow root for devices list to work around an
issue with focus being set on a wrong element when
clicking some other element [1].

[1] WICG/webcomponents#554 (comment)

Bug: 1075414
Change-Id: If5aad7f01a7c13cf095a73c12432661a44e4c771
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2165788
Reviewed-by: Peter Marshall <[email protected]>
Reviewed-by: Mathias Bynens <[email protected]>
Commit-Queue: Alex Rudenko <[email protected]>
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

5 participants