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

Add delegatesFocus init to attachShadow and flag to ShadowRoot #768

Merged
merged 4 commits into from Sep 24, 2019

Conversation

@rakina
Copy link
Contributor

rakina commented Jun 25, 2019

Fixes #367 (The DOM part of whatwg/html#2013)

This adds delegatesFocus to ShadowRootInit, used in attachShadow.
Also adds a "delegates focus flag" on shadow roots, that later can be referenced by the focus algorithms in the HTML specs.

Related PRs:


Preview | Diff

@rniwa

This comment has been minimized.

Copy link

rniwa commented Jun 25, 2019

Looks sane to me. This only defines a hook for the HTML specification to use it?

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Jun 25, 2019

Looks sane to me. This only defines a hook for the HTML specification to use it?

Yep, since the focus algorithms are defined there. I'm currently working on the HTML changes.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Jun 25, 2019

Thanks for the approval! (also can someone with merge access merge this?)

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jun 25, 2019

As the editor, @annevk is the one that should merge; I'm mostly doing a drive-by approval. We may also want to delay merging until the corresponding HTML-side change is ready, to ensure they integrate well.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Jun 25, 2019

As the editor, @annevk is the one that should merge; I'm mostly doing a drive-by approval. We may also want to delay merging until the corresponding HTML-side change is ready, to ensure they integrate well.

I see. Will link to the HTML spec PRs here when it's ready then. Thanks!

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jun 25, 2019

Since this is a new field, should we use a boolean instead (basically, remove the word flag and instead of set/unset we use set to false/true)? And yeah, it'd be great to land this together with the HTML change and tests (if any new tests are needed).

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jun 26, 2019

Another thing I realized is that we could clean this up further by having a default value in the dictionary. I suspect we might need to leave the default value outside the dictionary as well for shadow roots that are not allocated via the method (i.e., user agent shadow roots) though? Either way, if the dictionary has a default value the constructor step can simply assign that value to the internal slot.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Jun 26, 2019

Another thing I realized is that we could clean this up further by having a default value in the dictionary. I suspect we might need to leave the default value outside the dictionary as well for shadow roots that are not allocated via the method (i.e., user agent shadow roots) though? Either way, if the dictionary has a default value the constructor step can simply assign that value to the internal slot.

Yeah you're right! Changed :)

@annevk
annevk approved these changes Jun 26, 2019
Copy link
Member

annevk left a comment

Thanks! I'll leave this open per earlier discussion.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 12, 2019

@annevk I think we have enough HTML spec changes that are ready to merge (whatwg/html#4731 and whatwg/html#4735 almost) that we should merge this. Any objections?

We'll hold off on filing implementer bugs for delegatesFocus until all related PRs merge.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Jul 12, 2019

I don't think it makes senes to merge these changes unless there is some progress being made in at least one more implementation.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 12, 2019

@rniwa, per the working mode, the requirement is not two implementations, but two implementations that support the feature. Among other benefits, this allows people to implement off of a complete spec, instead of looking at multiple different branches and pull requests.

Previously you have expressed support for delegatesFocus, and I am pretty sure (but don't recall specific instances) that the same holds for Mozilla. Do you now feel that maybe WebKit might switch from supportive to opposed?

@rniwa

This comment has been minimized.

Copy link

rniwa commented Jul 12, 2019

@rniwa, per the working mode, the requirement is not two implementations, but two implementations that support the feature. Among other benefits, this allows people to implement off of a complete spec, instead of looking at multiple different branches and pull requests.

We do support the general use cases behind it. Given how complex all these changes are, I don't really understand what delegatesFocus truly does or what kind of code changes we'd end up having to make, or even the proposed change is implementable in WebKit. Given that, I don't think I can say we support the PR as is until the actual implementation effort starts happening in WebKit.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 12, 2019

OK. Please let us know when you have time to review them; it would be much appreciated.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 14, 2019

What's the behavior of the focus ring for a shadow host with delegatesFocus? It's not drawn?

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 14, 2019

Alright, I've left all the relevant comments after implementing what's being currently proposed & spec'ed in https://webkit.org/b/166484. The remaining issues / questions I have are:

  • What happens to focus rings?
  • WPT test in web-platform-tests/wpt#18035 seems to have bugs.
  • What does the first focusable area mean when focusing a shadow host with delegatesFocus set to true? Does it consider any elements that are focusable during sequential navigation? e.g. anchors vs. buttons.
@domenic

This comment has been minimized.

Copy link
Member

domenic commented Aug 28, 2019

@rakina did you see #768 (comment) ? I'll try to answer as best I can.

What happens to focus rings?

These seem to be largely user-agent defined, according to the CSSWG. https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo. Any changes would probably be done there, but I'm unsure what changes we would make, given how much leeway that document gives user agents. Perhaps we could add another bullet point to the non-normative suggestions?

@rakina can probably explain what Chrome does, perhaps as a starting point for such a non-normative suggestion.

WPT test in web-platform-tests/wpt#18035 seems to have bugs.

Looks like this got stalled a bit, I'll poke it.

What does the first focusable area mean when focusing a shadow host with delegatesFocus set to true? Does it consider any elements that are focusable during sequential navigation? e.g. anchors vs. buttons.

This is the subject of great discussion in whatwg/html#4796 (comment) and w3c/webcomponents#830 . Complicated :(. As I stated in the PR it might be best to merge with a big red box pointing to the open issue.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 29, 2019

These seem to be largely user-agent defined, according to the CSSWG. https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo. Any changes would probably be done there, but I'm unsure what changes we would make, given how much leeway that document gives user agents. Perhaps we could add another bullet point to the non-normative suggestions?

I think we need to at least decide what happens if UA can decide to make it obvious that a shadow host has the focus even if the focus had been delegated to its shadow tree's content. Or would we follow the normal convention so that the focus ring is only drawn in the inner most element with the actual focus?

What does the first focusable area mean when focusing a shadow host with delegatesFocus set to true? Does it consider any elements that are focusable during sequential navigation? e.g. anchors vs. buttons.

This is the subject of great discussion in whatwg/html#4796 (comment) and w3c/webcomponents#830 . Complicated :(. As I stated in the PR it might be best to merge with a big red box pointing to the open issue.

I don't think we should merge this or that PR until we figure out which element gets the focus delegated given that's this feature is all about. As things stand, I can't really implement / land this feature in WebKit because the behavior is not well defined.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Aug 29, 2019

Oops, I missed this thread. Thanks @domenic!

These seem to be largely user-agent defined, according to the CSSWG. https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo. Any changes would probably be done there, but I'm unsure what changes we would make, given how much leeway that document gives user agents. Perhaps we could add another bullet point to the non-normative suggestions?

The default focus ring is drawn for all the activeElement in every shadow tree/document. If a shadow host that delegates focus has a focused element within its shadow tree, the focus ring is drawn for both the focused element in the shadow tree and the host.

I'm not really sure I understand :focus-visible, but I think in Blink it's like the above with the added requirement of being focused sequentially or through focus() (in other words, not through click), or if it's editable/can have user keyboard input/etc.

Looks like this got stalled a bit, I'll poke it.

I'll change shadow-utils a bit directly on Blink and update the tests once we figure out what's the desired behavior for focus delegation is.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 29, 2019

The default focus ring is drawn for all the activeElement in every shadow tree/document. If a shadow host that delegates focus has a focused element within its shadow tree, the focus ring is drawn for both the focused element in the shadow tree and the host.

It seems like that should be spec'ed somewhere then. It doesn't look like the current CSS selectors spec allows that for now.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Aug 29, 2019

It's a little unclear to me why speccing the focus ring is required. It is not web-observable (right?). I thought it was just part of how browsers present their UI.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 29, 2019

It's a little unclear to me why speccing the focus ring is required. It is not web-observable (right?). I thought it was just part of how browsers present their UI.

I mean, it's definitely not a blocker but we should update the spec because while it doesn't specify when UA MUST show a focus ring but it does specify it MUST NOT implicitly.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Aug 30, 2019

It seems like that should be spec'ed somewhere then. It doesn't look like the current CSS selectors spec allows that for now.

Oh sorry I was mistaken in my post earlier: we don't draw the focus rings for every activeElement. Instead I think we draw the focus ring for everything that matches the :focus selector (so hosts with delegatesFocus gets the focus ring if there's anything focused in its shadow scope), which is already updated in whatwg/html#4731. Is there any other place that needs an update?

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 30, 2019

Instead I think we draw the focus ring for everything that matches the :focus selector (so hosts with delegatesFocus gets the focus ring if there's anything focused in its shadow scope), which is already updated in whatwg/html#4731.

Ah, okay. The focus ring issue is taken care of then. It might be worth adding a non-normative note about that.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Sep 24, 2019

Given the consensus we reached at TPAC, let's start merging things as they become ready. This one is the foundation of all the others, and is solid, so I'll start here!

@domenic domenic merged commit 4cf85ef into whatwg:master Sep 24, 2019
1 check passed
1 check passed
Participation rakina participates on behalf of Google LLC
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.