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 delegation from shadow host should not behave like sequential focus navigation #830

Closed
rakina opened this issue Aug 28, 2019 · 49 comments

Comments

@rakina
Copy link
Member

rakina commented Aug 28, 2019

A brief summary of focus to help understand the issue:

  • Focusing on an element can be done through three methods: sequential, click, and programmatical. They differ in a number of ways, but for this issue we will only need to know the differences related to the tabindex value.
    • Sequential focus navigation (Moving focus with tab key) skips elements with negative tabindex value
    • Sequential focus navigation also prioritizes elements whose tabindex value is non-zero, in increasing order. So it will move the focus to all element with tabindex value of 1, and then all element with tabindex value of 2, ... and so on, and finally move the focus to elements with tabindex value of 0 last.
    • Click focus (clicking on an element) and programmatic focus (calling focus() on an element) will focus on elements even if their tabindex value is negative
  • When attaching a shadow root to a host, we can set its delegateFocus value to true, so that the host will delegate focus.
  • When a host that delegats focus is the target of focus (with any method) it will not focus on the host, but instead move/delegate the focus to one of the element in its shadow root's tree/scope.

While we were trying to move the delegatesFocus spec to the HTML spec on whatwg/html#4796, @domenic pointed out that the current behavior (which I think is only implemented in Chrome?) is quite weird: it uses sequential focus navigation even when the host gets focused with clicking on it or calling the focus() method on it.

Currently the delegation process finds the new focus target with sequential focus navigation, this means in this case:

<div #host>
 #shadowRoot delegatesFocus=true
  <div tabindex=-1>clickable</div>
  <div tabindex=0>sequentially focusable</div>

If we click/call focus() on the host, it will focus the "sequentially focusable" div instead. This is a bit weird because it's inconsistent with the method that initially makes us target the host in the first place.
Proposal to change the behavior:
If we click/call focus() on the host, we should be as if we actually click/call focus() on the first element that is click/programmatically focusable, even if it's not sequentially focusable - in this case the "clickable" div. Note that if we are focusing on the host through tabbing to it (sequential focus navigation), then we will use sequential focus navigation and skip elements with negative tabindex values.

Another point - the current behavior in Chrome also cares about tabindex value priority mentioned earlier. For example:

<div #host>
 #shadowRoot delegatesFocus=true
  <div tabindex=0>A</div>
  <div tabindex=1>B</div>
</div>

If we focus on #host, the "B" div will get focus.
Proposal to change the behavior:
Since we want to change the behavior of focus delegation to not be related to sequential focus navigation, we should probably remove the tabindex priority thing as well in this case. So we should always delegate focus to the first focusable area in DOM/composed-tree order - in this case the "A" div.

Since the sequential delegation behavior is already implemented in Chrome, there is a compatibility risk for changing to the proposed behavior. I predict this to be small and worth it for the change, but would love to hear from people who actually use this on whether this change makes sense and whether it's hard to move from the old behavior to the new one.

cc @rniwa @annevk @smaug---- and possible users of delegatesFocus that might be impacted @justinfagnani @muan?

@annevk
Copy link
Collaborator

annevk commented Aug 28, 2019

It seems reasonable to me to change the default. I think ideally the delegation is exposed to script (either by being able to set an element or some kind of callback).

(Out of curiosity, what happens when the host is focusable itself?)

@rniwa
Copy link
Collaborator

rniwa commented Aug 28, 2019

Since the sequential delegation behavior is already implemented in Chrome, there is a compatibility risk for changing to the proposed behavior. I predict this to be small and worth it for the change, but would love to hear from people who actually use this on whether this change makes sense and whether it's hard to move from the old behavior to the new one.

This indeed seems like a serious compat risk for Chrome although the new proposed behavior is a lot simpler to understand & implement so I support the proposed change.

@domenic
Copy link
Collaborator

domenic commented Aug 28, 2019

@annevk to be clear, changing the default isn't quite what we raised this issue for. The question is just what the default behavior should be.

In Chrome's implementation click/programmatic-focusing a delegatesFocus host chooses a shadow-child to delegate to based on the shadow-children's sequential-focusability. This is strange. The proposed change is to choose a shadow-child based on the shadow-children's click/programmatic focusability.

There might be a separate discussion about giving more explicit control over focus delegation, instead of "pick the appropriate child that is focusable", but I think that gets complicated for different reasons...

@domenic
Copy link
Collaborator

domenic commented Aug 28, 2019

One thing I just clarified for myself:

If we click/call focus() on the host,

This predicate, and its consequences, only hold if you click on the host directly. If you click on one of the children, then that child will get focus directly, regardless of ordering. That is, consider an element implemented like Chromium's <input type=date>. We are discussing what happens if you click on the border of the element, or on the empty space. If you click on one of the three "shadow child textboxes", then that textbox directly will get focus.

(This is true with both the Chromium behavior and the proposed change.)

@domenic
Copy link
Collaborator

domenic commented Aug 28, 2019

Let me try to come up with a semi-realistic example. This is fairly hard to do, because we need to find situations where:

  • Using delegatesFocus makes sense
  • Click/programmatic/sequential focusability differ for the shadow children

You can easily come up with contrived examples, like the OP, by using tabindex="" to cause differing click/programmatic/sequential focusability. But a realistic example is trickier. This is the best I've got.

Consider a control that is a kind of "color well", with a button that pops open a color picker, plus a textbox for direct control entry. We want this control to delegate focus, similar to <input type=date>: it is itself focusable, but also you can focus the individual pieces of it, at least in browsers where they are focusable. So your flat tree looks like:

<span tabindex="0">Before</span>
<color-picker>
  <!!shadow-root delegatesFocus=true!!>
    <button style="background: #0033FF;"></button>
    #<input type="text" value="0033FF">
  </!!shadow-root!!>
</color-picker>
<span tabindex="0">After</span>

On Chrome with the current Chromium behavior:

  • Tabbing through this markup will focus Before -> button -> input -> After
    • The button is the first sequentially-focusable shadow child
  • Clicking on the color-picker (e.g. on the # character) will focus the button
    • The button is the first sequentially-focusable shadow child
  • Calling .focus() on the color-picker will focus the button
    • The button is the first sequentially-focusable shadow child

On Safari (where buttons are only programmatically focusable), with the current Chromium behavior:

  • Tabbing through this markup will focus Before -> input -> After
    • The input is the first sequentially-focusable shadow child
  • Clicking on the color-picker (e.g. on the # character) will focus the input
    • The input is the first sequentially-focusable shadow child
  • Calling .focus() on the color-picker will focus the input
    • The input is the first sequentially-focusable shadow child

On Chrome with the proposed behavior: no change, but the reasoning changes...

  • Tabbing through this markup will focus Before -> button -> input -> After
    • The button is the first sequentially-focusable shadow child
  • Clicking on the color-picker (e.g. on the # character) will focus the button
    • The button is the first click-focusable shadow child
  • Calling .focus() on the color-picker will focus the button
    • The button is the first programatically-focusable shadow child

On Safari with the proposed behavior:

  • Tabbing through this markup will focus Before -> input -> After
    • The input is the first sequentially-focusable shadow child
  • Clicking on the color-picker (e.g. on the # character) will focus the input
    • The input is the first click-focusable shadow child
  • Calling .focus() on the color-picker will focus the button, the first programmatically-focusable
    • The button is the first programmatically-focusable shadow child

Make sense?

@justinfagnani
Copy link
Contributor

justinfagnani commented Aug 28, 2019

I haven't used delegatesFocus yet. Maybe @azakus, who's working on the material design components, can comment from actual usage.

But reading the details (thanks for the very clear explanation @rakina), it doesn't sound to me like any choice is going to be all that great. We have to choose either the first sequential or first clickable focusable element - and in either case we're only inferring what the component author wants in the face of no explicit focus event handler. I honestly find either choice strange in its own way. tabindex=-1 could be interpreted to mean "only focus this element explicitly" and yet the proposed change would add a new want to focus it without actually clicking it or calling focus().

The thing I find particularly strange about the proposed change is that we'd be picking the first click-focusable element, ie sequentially, where order doesn't matter otherwise. At least with Chrome's current behavior the author already expects that tabindex ordering and tree-order matter.

All that said, I don't feel particularly strongly here.

@domenic
Copy link
Collaborator

domenic commented Aug 28, 2019

The thing I find particularly strange about the proposed change is that we'd be picking the first click-focusable element, ie sequentially., where order doesn't matter otherwise. At least with Chrome's current behavior the author already expects that tabindex ordering and tree-order matter.

Right, after talking with @s4y I realized that's probably the justification for Chrome's current behavior. That is, Chrome's current behavior allows you to get some control over where focus gets delegated to in all cases, whereas the proposed behavior only gives you such control for sequential focus.

The weird thing is, the way that Chrome's proposed behavior gives you such control is by co-opting the tabindex="" attribute, which previously was about controlling sequential focus, to also control click and programmatic focus.

This gets back to @annevk's point about giving better control, and my comment about it being complicated. It feels the most powerful thing we could do would be three separate APIs, for controlling delegation for each of click, programmatic, and sequential focus, instead of assuming you want the same delegation in all cases.

But that's tricky in at least two ways:

@justinfagnani
Copy link
Contributor

Do we not already have an API with the focus and focusin events?

  this.addEventListener('focus', (e) => {
    e.preventDefault();
    this.shadowRoot.querySelector('#default-input').focus();
  });

@travisleithead
Copy link
Member

CC @atanassov and @boggydigital

@rniwa
Copy link
Collaborator

rniwa commented Aug 29, 2019

  • We don't really have that control for the main document, so giving that control for shadow children seems strange.
  • Any explicit focus control starts to collide with user agents' desires to provide platform-aligned defaults

Yeah, both of these are highly problematic. What if we let delegatesFocus take an ID as in: delegatesFocus: "foo" and pick the first element with that ID? ID is unique per shadow tree so there is no ambiguity. But this probably needs to be used only in the case of an explicit focus call or when the user clicks somewhere on a custom element but not elements within its shadow tree themselves.

In the case when the user uses sequential focus navigation, the UA should probably decide where the focus should go based on what it decides to be focusable within the shadow tree. Otherwise, we'd end up with a bunch of components that try to implement Windows behavior to delegate the focus to hyperlinks and bunch others that try to match macOS to skip hyperlinks and only delegate to editable text fields.

@ekashida
Copy link

I've been spending some time on a delegatesFocus polyfill at Salesforce and I support the proposed behavior. I think it makes sense that click and programmatic focus behavior are not influenced by the sequential focus navigation order because the default user experience should be consistent. It's considered a bad practice to go crazy with tabindex values because that can potentially ruin the user experience and I would prefer that the default behavior is not tied to that.

In terms of control, components could do the following for programmatic:

class Foo extends HTMLElement {
  focus() {
    this.shadowRoot.querySelector('input').focus(); 
  }
}

This is what our internal components are required to do so that we don't break them once the expected behavior is specified and our framework is ready to implement it.

I'm not sure if anything can be done for clicks using existing APIs but I wonder if the component really needs control over that?

@justinfagnani
Copy link
Contributor

I think it makes sense that click and programmatic focus behavior are not influenced by the sequential focus navigation order because the default user experience should be consistent.

What does "consistent" mean here?

The more I think about it the more I think that the current Chrome behavior makes sense. Via tabindex the developer has at least hinted at which element come "first" in some logical ordering, and that seems the most likely to be the one that should be selected absent any other signals.

@caridy
Copy link

caridy commented Aug 30, 2019

@justinfagnani I think "consistent" means that the user can reasoning about how the UI respond to interactions for the most part. And yes, the developer can control some aspects of it, e.g.: programmatic focus, but the rest should behave as expected.

@ekashida has been our champion when it comes to focus related bug, and it is important to also notice that focus is the biggest honeypot for Lightning Web Components bugs, it has been like that for a long time, probably because we have a lot of complex forms and inputable components.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 30, 2019

I was notified, asked to give a comment in this discussion, so let me do it. :)

I remember that I and @TakayoshiKochi designed and implemented the current behavior.
So I can share some thought here why we choose the current behavior. I hope that might be useful for further discussion!

  • There is a use case where web components authors want to control which element in a shadow tree should get focused when a host is clicked; Web component authors don't like if the first clickable element in a shadow tree is always focused.
  • If an end-user clicks a host, and then hits TAB keys to traverse elements in the component , an end user expects that they can traverse every traversal elements in the component. If we introduce inconsistency here, we break this expectation. e.g. Suppose that there are 10 elements which can be traversed in the component. If we introduce an inconsistency here, in the worst case, when an end user clicks the host, and hits TAB key, then an end user would escape out of the component without traversing 9 elements.

I can guess that the current behavior is not perfect, and there is a situation that the current behavior is not desired, however, I think the current behavior should make sense in most use cases with consistent behaviors between click and sequential navigation.

If we want to address a use case where this default consistent behavior is not desired, I think we should add another API (or any workaround) so that web component authors explicitly use it; web components authors should be aware that they might break an end user's expectation by using it.

@rniwa
Copy link
Collaborator

rniwa commented Aug 30, 2019

The disconnect here is that it's highly desirable for JS API element.focus() to have interoperable behavior across browsers yet what's being proposed and what Chrome implements can't be.

Also, I'm not certain that using the keyboard-based sequential focus navigation order for when a host element is clicked makes much sense. If the user is clicking somewhere a more sensible thing for the component might be to select the nearest element there is to where the user clicked so perhaps some kind of an event-based focus delegation is needed anyway. In the case where keyboard is used to move focus, however, it obviously makes sense that tabIndex order is used.

@ekashida
Copy link

@justinfagnani - As @caridy mentioned, what I meant by "consistent" is that the resulting outcome is easier to reason about for the end-user as they interact with different components over time. Thinking more about it though--as @rniwa mentioned--the most sensible thing to happen for the click case would probably be for the nearest focusable element to be focused, and from that perspective, any other option seems kind of arbitrary.

My only issue with focusing on the first sequentially focusable element is that we would be ignoring all click-focusable elements for clicks, and all programmatically-focusable elements for programmatic, and that seems very strange.

@ekashida
Copy link

@hayatoito - Could you elaborate on the "inconsistency" that you mentioned in your second point?

@hayatoito
Copy link
Contributor

hayatoito commented Aug 30, 2019

The most sensible thing to happen for the click case would probably be for the nearest focusable element to be focused,

I think the first element is not always nearest element from an end user's perspective.
An end user may click around "bottom-right corner" of a host element, and HitTest would pick up the host element, if there is no overlapping element in a shadow tree for the clicked point.

@rniwa
Copy link
Collaborator

rniwa commented Aug 30, 2019

My only issue with focusing on the first sequentially focusable element is that we would be ignoring all click-focusable elements for clicks, and all programmatically-focusable elements for programmatic, and that seems very strange.

Right. I don’t think the current behavior of Chrome makes much sense in this regard.

@hayatoito
Copy link
Contributor

hayatoito commented Aug 30, 2019

It's still unclear to me why the first clickable element (in a node tree) is better.

Could someone explain what would happen if the first clickable element is assigned to a slot?

For example, given the following tree of trees:

host-1 (delegatesFocus=true)
└──/shadowroot
    └── host-2 (non-focusable element)
        ├──/shadowroot
        │   ├── slot name=slot1
        │   └── slot name=slot2
        ├── clickable-element2 slot=slot2
        └── clickable-element1 slot=slot1

Then, the flat tree would be:

host-1
└── host-2
    ├── clickable-element1 slot=slot1
    └── clickable-element2 slot=slot2

If an end user clicks host-1, an end user's expectation is that clickable-element1 gets focused, I think.
The current behavior focuses clickable-element1, AFAIR. The current behavior tries to focus the first element in the flat tree as much as possible, as long as all traversal elements have the same tabindex value.

If we always choose the first clickable element in the shadow tree, ignoring slot assigments at all, we will focus clickable-element2. Is that acceptable behavior?

I think the current behavior is not perfect, but it works in most cases, handling this case too, without any further tricks.

Comments are welcome!

@rakina
Copy link
Member Author

rakina commented Aug 30, 2019

If we always choose the first clickable element in the shadow tree, ignoring slot assigments at all, we will focus clickable-element2. Is that acceptable behavior?

FWIW we can definitely use the flat tree even if we're not using the "sequential focus navigation" delegation anymore.

Yeah, both of these are highly problematic. What if we let delegatesFocus take an ID as in: delegatesFocus: "foo" and pick the first element with that ID? ID is unique per shadow tree so there is no ambiguity. But this probably needs to be used only in the case of an explicit focus call or when the user clicks somewhere on a custom element but not elements within its shadow tree themselves.

This might have some problems with slotted elements. What if we use the actual elements instead? So like host.setFocusDelegationTarget(el)? The default behavior if the focus delegation target is not set would be to focus the first focusable thing in flat-tree order, but if it's set it will always focus on el.

@rniwa
Copy link
Collaborator

rniwa commented Aug 30, 2019

Yeah, both of these are highly problematic. What if we let delegatesFocus take an ID as in: delegatesFocus: "foo" and pick the first element with that ID? ID is unique per shadow tree so there is no ambiguity. But this probably needs to be used only in the case of an explicit focus call or when the user clicks somewhere on a custom element but not elements within its shadow tree themselves.

This might have some problems with slotted elements. What if we use the actual elements instead? So like host.setFocusDelegationTarget(el)? The default behavior if the focus delegation target is not set would be to focus the first focusable thing in flat-tree order, but if it's set it will always focus on el.

Indeed, an explicit setter might better although it brings its own can of worms by allowing random node outside of shadow host's flat tree to be specified. However, that might not matter. We can reject whatever node which is not a focusable element in the flat tree at the time of focusing, and treat as if it's not specified in which case we can fallback to the default (maybe the first focusable element).

Now that I've been thinking about this issue for a while, I'm pretty convinced that mouse based focusing & keyboard based focusing need to be treated differently. In the case of keyboard based sequential focus navigation, we should simply find the first element with the tabIndex order and focus that for the reasons @hayatoito stated above. So regardless of whether we add this setter or not, the keyboard-based sequential focus navigation should continue to use tabIndex order to pick which element gets the delegation.

In the case of mouse based focusing, however, it's unclear to me if a component can specify which element to be focused priori. As I stated above, some components may need to make a decision knowing where the user had clicked to trigger such focusing. I think a component can do this by listening to focus event on the host element and re-focusing to whichever element they want to delegate focus.

As I've noted multiple times above, we don't want element.focus() to depend upon tabIndex order since what is considered keyboard focusable is implementation dependent and we want JS API to be as interoperable as possible. This would mean that the main point of host.setFocusDelegationTarget is to specify what gets focused when the script manually calls element.focus. Note this too needs to have a fallback option when the element the script had set isn't in the flat tree of the shadow host. So we'd have to decide what would happen in that case as well (probably the first clickable element in the flat tree).

@domenic
Copy link
Collaborator

domenic commented Aug 30, 2019

So I feel like we have something close to consensus on the following:

  • Sequential focus: use the tabIndex order
  • .focus(): use the first focusable area in flat tree order by default

We are still discussing the desired behavior for clicking, e.g. using flat-tree-first focusable element vs. tabIndex-order-first element vs. closest-to-cursor focusable element. However, getting this exactly nailed down in the spec seems less urgent, because there will always be user agent discretion involved. We could even leave it explicitly up to the UA in the spec.

We also are discussing potential additions:

  • .focus(): changing the default element focused
  • Clicking: changing the default element focused

Although, it's also been noted that there are alternative ways of accomplishing this, e.g. overriding .focus(), or listening for the "focus" event. (My personal favorite would be to add a field to the focus Event object that tells you how the element was focused.)


If the above is accurate, I propose:

  • The spec (i.e. the PR in Handle shadow host with delegatesFocus=true in focusing steps (click focus + focus()) whatwg/html#4796) be modified in the way @rakina proposes in the OP.
    • Variant: leave click focusing behavior up to the UA, so it can choose between things like closest to cursor, etc. based on what it thinks is useful.
  • Chrome's shipping plan:
    • Work to ship .focus() using the first element in flat tree order. Report back to standards forums if this turns out to be web-incompatible. This is important because it is a potential interop issue.
    • Less urgently, work to align with the spec on click focusing. (This is not an interop issue and UA discretion on determining what is click focusable means we technically can be following the spec either way.)
  • Use this repo (probably this thread) to continue discussing use cases for the "potential additions" listed.

Does this sound good? Thoughts on OP vs. the "leave click focusing up to the UA" variant?

@smaug----
Copy link

That proposal doesn't look quite right. We should define the order in which UA should try to find the focusable element on clicking. Then leave it up to the UA to say which elements are focusable that way. (Firefox on MacOS should have quite similar behavior to Safari)

@rniwa
Copy link
Collaborator

rniwa commented Sep 2, 2019

  • Sequential focus: use the tabIndex order
  • .focus(): use the first focusable area in flat tree order by default

Yeah, it seems like we've reached a consensus on these two issues as far as I could tell.

I really don't get the urgency of merging this feature into the HTML spec though. All the involved parties are paying attention to this issue and related PRs so it's not like having this in the HTML specification would speed up any process. If anything, it would create an illusion of the feature being mature to web developers, which is problematic.

We also are discussing potential additions:

  • .focus(): changing the default element focused
  • Clicking: changing the default element focused

Although, it's also been noted that there are alternative ways of accomplishing this, e.g. overriding .focus(), or listening for the focus event. (My personal favorite would be to add a field to the focus Event object that tells you how the element was focused.)

Adding an option to focus do a sequential focus navigation is problematic because that would result in a different element being focused by different UAs. That would lead to many web developers writing code that relies on Chrome's behavior on Windows for example and can cause compatibility issues.

Work to ship .focus() using the first element in flat tree order. Report back to standards forums if this turns out to be web-incompatible. This is important because it is a potential interop issue.

FWIW, if this turns out be an compatibility issue, we should probably rename delegatesFocus to something else instead of spec'ing Chrome's behavior because implementing Chrome's behavior for focus() is problematic independent of whether it's Web compatible or not for the reasons I've stated above.

That proposal doesn't look quite right. We should define the order in which UA should try to find the focusable element on clicking. Then leave it up to the UA to say which elements are focusable that way. (Firefox on MacOS should have quite similar behavior to Safari)

Could you elaborate what you mean by this? Are you suggesting that we should define an order of focusable areas and then UA would pick the first element in which it deems focusable under that ordering? That does sound like a reasonable approach.

@domenic
Copy link
Collaborator

domenic commented Sep 3, 2019

I really don't get the urgency of merging this feature into the HTML spec though. All the involved parties are paying attention to this issue and related PRs so it's not like having this in the HTML specification would speed up any process. If anything, it would create an illusion of the feature being mature to web developers, which is problematic.

I mean, this feature has been in a semi-specced state for literally years (in the shadow DOM spec). It has fairly high usage. It'd be nice to have a real spec, instead of telling people to look at a bunch of outstanding pull requests, or maybe the shadow DOM spec.

@rniwa
Copy link
Collaborator

rniwa commented Sep 3, 2019

I really don't get the urgency of merging this feature into the HTML spec though. All the involved parties are paying attention to this issue and related PRs so it's not like having this in the HTML specification would speed up any process. If anything, it would create an illusion of the feature being mature to web developers, which is problematic.

I mean, this feature has been in a semi-specced state for literally years (in the shadow DOM spec). It has fairly high usage. It'd be nice to have a real spec, instead of telling people to look at a bunch of outstanding pull requests, or maybe the shadow DOM spec.

The reality of the situation is that delegatesFocus is currently a proprietary API of Blink. I don't think anyone should be encouraging developers to start using it outside the confined realm of Blink only content (e.g. Chrome App store).

I find it rather frustrating that this feature got dropped on floor when people started merging shadow DOM & custom elements spec into the HTML & DOM if you think it's so important; specially because I had complained about how the merge would make reviewing & implementing these features impossible on my end.

Regardless, we're making pretty good progress here. Let's focus on the technical discussion and settle on what to do on click. That's the only ting left to agree on, and I bet we can get there in a matter of days & weeks, and not months.

@rniwa
Copy link
Collaborator

rniwa commented Sep 5, 2019

Hm... if I'm not mistaken, it doesn't seem like the current HTML specification defines how clicking on an element ends up putting the focus on the element at all... If that's the case (someone should confirm it), it's kind of moot point as to what delegatesFocus would do to it since it's entirely unspecified at the moment (obviously we should fix that but that is probably best done as a separate project).

@rakina
Copy link
Member Author

rakina commented Sep 5, 2019

Hm... if I'm not mistaken, it doesn't seem like the current HTML specification defines how clicking on an element ends up putting the focus on the element at all... If that's the case (someone should confirm it), it's kind of moot point as to what delegatesFocus would do to it since it's entirely unspecified at the moment (obviously we should fix that but that is probably best done as a separate project).

I recently worked on an update around that part of the spec: whatwg/html#4768
See the definition of "click focusable" here: https://whatpr.org/html/4768/2a397f6...75b4443/interaction.html#click-focusable and how the focusing steps is triggered. Do you think this solves that concern?

@rniwa
Copy link
Collaborator

rniwa commented Sep 5, 2019

I recently worked on an update around that part of the spec: whatwg/html#4768
See the definition of "click focusable" here: https://whatpr.org/html/4768/2a397f6...75b4443/interaction.html#click-focusable and how the focusing steps is triggered. Do you think this solves that concern?

Indeed, that PR adds the much needed definition. It's really unfortunate that all these PRs are tied up with delegatesFocus. Otherwise, we could have been able to merge them all and have one tiny PR which just defines delegatesFocus.

@rakina
Copy link
Member Author

rakina commented Sep 5, 2019

Indeed, that PR adds the much needed definition. It's really unfortunate that all these PRs are tied up with delegatesFocus. Otherwise, we could have been able to merge them all and have one tiny PR which just defines delegatesFocus.

Actually both whatwg/html#4735 and whatwg/html#4768 are not really related to delegatesFocus. They only mention delegatesFocus https://github.com/whatwg/html/pull/4735/files#diff-36cd38f49b9afa08222c0dc9ebfe35ebR73369 in one line and I guess I can take that out (though it only says we skip hosts with delegatesFocus when doing sequential focus navigations, which I think is non-controversial for everybody?).

If you and @smaug---- approve those, then we can merge those two PRs - and after those two are merged the delegatesFocus changes are actually quite small.

@rakina
Copy link
Member Author

rakina commented Sep 9, 2019

Regardless, we're making pretty good progress here. Let's focus on the technical discussion and settle on what to do on click. That's the only ting left to agree on, and I bet we can get there in a matter of days & weeks, and not months.

Continuing the discussion on this,

Could you elaborate what you mean by this? Are you suggesting that we should define an order of focusable areas and then UA would pick the first element in which it deems focusable under that ordering? That does sound like a reasonable approach.

I think this approach is good. The set of things would be all the focusable elements in the host's shadow tree (and slotted ones too). Maybe the ordering will be flat-tree order, like the one suggested in the original post? Then the UA can use their own mechanisms (like skipping options). What do you think?

FYI this is kinda like what <input type="date"> is doing, it delegates to the first focusable area when clicked.

@rniwa
Copy link
Collaborator

rniwa commented Sep 9, 2019

Could you elaborate what you mean by this? Are you suggesting that we should define an order of focusable areas and then UA would pick the first element in which it deems focusable under that ordering? That does sound like a reasonable approach.

I think this approach is good. The set of things would be all the focusable elements in the host's shadow tree (and slotted ones too). Maybe the ordering will be flat-tree order, like the one suggested in the original post? Then the UA can use their own mechanisms (like skipping options). What do you think?

But defining the order here doesn't help if UA could skip any element because we're picking exactly one element here. Let's say we have elements: e_1, e_2, etc... e_n inside a shadow tree. If UA wanted to focus an element e_c because it happens to be the first element in flat tree order, or because it's the first in tab index order, then UA can just skip e_1 through e_(c-1) and always pick e_c. Put it another way, for any sequence sequence S_1 and S_2 each consisting of all elements of a set {e_1, e_2, ... e_n}, you can pick k ∈ ℤ: 1 ≤ k ≤ n such that k-th element in S_1 is the first element in S_2. So defining the order then letting UA skip any element would not result in any more specific definition than saying it's entirely UA dependent.

@domenic
Copy link
Collaborator

domenic commented Sep 9, 2019

I agree, which is why I was a bit surprised by folks pushing to standardize a specific order. Given that different browsers explicitly want to have different choices for what elements are click-focusable, I don't see any way of avoiding the result being UA-dependent. So shall we just go with that?

@rniwa
Copy link
Collaborator

rniwa commented Sep 9, 2019

I agree, which is why I was a bit surprised by folks pushing to standardize a specific order. Given that different browsers explicitly want to have different choices for what elements are click-focusable, I don't see any way of avoiding the result being UA-dependent. So shall we just go with that?

Well, there is a subtle difference between allowing UA to skip any element and skip only ones that are not mouse focusable. If we defined the order and defined so that UA can only skip non-mouse focusable element, then the first element is more or less fixed. e.g. UA can't use tab index order as opposed to flat tree order since the first mouse focusable element wouldn't be the first element in the tab index order.

@domenic
Copy link
Collaborator

domenic commented Sep 9, 2019

I think the intention of @rakina's OP was to only skip ones that are not click-focusable. However, I'm not sure how different that is in practice, because the UA gets to decide what is click-focusable, so in essence it seems like the UA just gets whatever freedom it wants.

@rakina
Copy link
Member Author

rakina commented Sep 10, 2019

Well, there is a subtle difference between allowing UA to skip any element and skip only ones that are not mouse focusable. If we defined the order and defined so that UA can only skip non-mouse focusable element, then the first element is more or less fixed. e.g. UA can't use tab index order as opposed to flat tree order since the first mouse focusable element wouldn't be the first element in the tab index order.

Alright. So, for clicking on a host that delegates focus. let's use "Focus on the first element in flat-tree order that is click focusable." with the definition of "click focusable" from https://whatpr.org/html/4768/2a397f6...75b4443/interaction.html#click-focusable. This ensures UA can only skip the ones it considers to be not click focusable, making it consistent with its other click behaviors.

@rniwa
Copy link
Collaborator

rniwa commented Sep 10, 2019

That sounds good. Hopefully that's what @smaug---- had in mind as well in #830 (comment).

aarongable pushed a commit to chromium/chromium that referenced this issue Sep 10, 2019
The focus delegation behavior might change as proposed in
WICG/webcomponents#830.

This CL adds use counter for when a host with delegatesFocus is focused,
and when the result of the focus delegation is different than the
proposed change, to see the possible impact of the change.

Bug: 692678
Change-Id: I90a0a10e1e5adbd3fe018ea772e1d560fedec9a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1794945
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695100}
@smaug----
Copy link

That sounds reasonable

@mfreed7

This comment has been minimized.

@justinfagnani
Copy link
Contributor

@mfreed7 that's indeed the default behavior. This is only for when the delegatesFocus option has been passed to attachShadow().

@mfreed7
Copy link

mfreed7 commented Sep 16, 2019

Sorry - please ignore my comment.

@rniwa
Copy link
Collaborator

rniwa commented Sep 18, 2019

Sounds like there was a consensus to move forward with what's being proposed thus far:

  • Programmatic focus would focus the first element in the flat tree that's programmatically focusable.
  • Mouse focus would focus the first element in the flat tree that's mouse focusable.
  • Keyboard focus would focus the first element in tab index order that's keyboard focusable.

@rniwa
Copy link
Collaborator

rniwa commented Oct 8, 2019

Are relevant PRs being updated with the latest discussion? It's getting hard to keep track of what's being incorporated and what's not.

@rakina
Copy link
Member Author

rakina commented Oct 8, 2019

Are relevant PRs being updated with the latest discussion? It's getting hard to keep track of what's being incorporated and what's not.

Yes (mostly), also there's a list on whatwg/html#2013 (comment).
There are 2 issues with no PRs yet :

I think they should be added later on after whatwg/html#4796 is merged (which is ready for merging now I think)

@rniwa
Copy link
Collaborator

rniwa commented Oct 8, 2019

Hm... it doesn't seem like web-platform-tests/wpt#18035 contain tests for tabIndex order being ignored for focus() and click(). Could you point me to a test case which would fail if we had used tabIndex order for those?

@rakina
Copy link
Member Author

rakina commented Oct 8, 2019

Hm... it doesn't seem like web-platform-tests/wpt#18035 contain tests for tabIndex order being ignored for focus() and click(). Could you point me to a test case which would fail if we had used tabIndex order for those?

The test for click with varying tabindex is https://github.com/web-platform-tests/wpt/blob/d50f1a32f2c84c8466e739f40cd3d9fb70668c5d/shadow-dom/focus/click-focus-delegatesFocus-tabindex-varies.html

I had some tests for focus() where we still ensure it focuses things with tabindex=-1, also added the tabindex=0 vs tabindex=1 test just now https://github.com/web-platform-tests/wpt/blob/d50f1a32f2c84c8466e739f40cd3d9fb70668c5d/shadow-dom/focus/focus-method-delegatesFocus.html

@rniwa
Copy link
Collaborator

rniwa commented Oct 11, 2019

Okay, two of your tests have bugs as I pointed out in web-platform-tests/wpt#18035 but everything else looks good. Will upload a patch to implement this in WebKit once those tests are fixed.

@domenic
Copy link
Collaborator

domenic commented Oct 16, 2019

Fixed in whatwg/html#4796!

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