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

Handle shadow host with delegatesFocus=true in focusing steps (click focus + focus()) #4796

Merged
merged 8 commits into from Oct 16, 2019

Conversation

@rakina
Copy link
Contributor

rakina commented Jul 24, 2019

  • At least two implementers are interested (and none opposed):
    • Blink
    • Gecko & Safari are reviewing the other PRs, so marking as interested
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: TODO
    • Firefox: TODO
    • Safari: TODO

Another part of #2013 and whatwg/dom#768. Defines the behavior of focus() and the focusing steps in general.
Note that this is a change made on top of #4735, which defines flattened tabindex-ordered focus navigation scopes, etc. needed for this change.

Another note, the behavior in Chrome for focus() when the flattened tabindex-ordered focus navigation scope is empty (there's nothing to focus on) is kinda weird - it focuses on the host instead of not focusing on anything (crbug filed). I'm not sure if we want this behavior or not in the specs (the current PR aborts the focusing steps instead).


/browsing-the-web.html ( diff )
/infrastructure.html ( diff )
/interaction.html ( diff )
/references.html ( diff )

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Jul 24, 2019

cc people involved in previous PRs @rniwa @annevk @domenic

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic domenic force-pushed the rakina:focusmethod branch from 66f6b5e to 99c8181 Jul 30, 2019
Copy link
Member

domenic left a comment

I've squashed this to just two commits, 1e1697d which contains all of PR #4735, and 99c8181 which contains the new content for this pull request. I'm sorry GitHub doesn't have a better way to do dependent pull requests :-(.

Note that this handles the focusing steps generally, and so impacts things like:

  • autofocus
  • accesskey
  • fragment navigation

I think it does not impact sequential focus navigation, even though that algorithm calls the focusing steps, because the previous PR omits the host from the sequential focus navigation order.

This changes LGTM, although I will mark it "do not merge yet" since it's dependent on #4735 landing first.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 30, 2019

Also let's call special attention to @rakina's note in the OP:

Another note, the behavior in Chrome for focus() when the flattened tabindex-ordered focus navigation scope is empty (there's nothing to focus on) is kinda weird - it focuses on the host instead of not focusing on anything (crbug filed). I'm not sure if we want this behavior or not in the specs (the current PR aborts the focusing steps instead).

It sounds like Chrome would prefer to abort focusing in such cases (e.g. delegatesFocus on a shadow host that has no children). Does that make sense to other browsers? We can certainly iterate on it later, similar to #4731 (comment) for :focus, but it's also nice to get folks impressions.

@domenic domenic self-assigned this Jul 30, 2019
source Show resolved Hide resolved
@rniwa

This comment has been minimized.

Copy link
Collaborator

rniwa commented Aug 17, 2019

Another note, the behavior in Chrome for focus() when the flattened tabindex-ordered focus navigation scope is empty (there's nothing to focus on) is kinda weird - it focuses on the host instead of not focusing on anything (crbug filed). I'm not sure if we want this behavior or not in the specs (the current PR aborts the focusing steps instead).

It sounds like Chrome would prefer to abort focusing in such cases (e.g. delegatesFocus on a shadow host that has no children). Does that make sense to other browsers? We can certainly iterate on it later, similar to #4731 (comment) for :focus, but it's also nice to get folks impressions.

We should do whatever matches when focusing an element that's not focusable, which is basically aborting.

@annevk annevk requested a review from smaug---- Aug 26, 2019
@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Aug 27, 2019

Another note, the behavior in Chrome for focus() when the flattened tabindex-ordered focus navigation scope is empty (there's nothing to focus on) is kinda weird - it focuses on the host instead of not focusing on anything (crbug filed). I'm not sure if we want this behavior or not in the specs (the current PR aborts the focusing steps instead).

It sounds like Chrome would prefer to abort focusing in such cases (e.g. delegatesFocus on a shadow host that has no children). Does that make sense to other browsers? We can certainly iterate on it later, similar to #4731 (comment) for :focus, but it's also nice to get folks impressions.

We should do whatever matches when focusing an element that's not focusable, which is basically aborting.

SGTM! This matches the spec PR then.

Copy link
Collaborator

smaug---- left a comment

This isn't really reviewable since the patch has somehow #4735 in it.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Sep 2, 2019

This isn't really reviewable since the patch has somehow #4735 in it.

Here's the changes on top of that PR: 99c8181

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Sep 20, 2019

Per w3c/webcomponents#830 (comment), this needs an update to use the flat tree order and also use the "click focusable" definition from #4768. I'll update this next week - in the meantime, @smaug---- and @rniwa can you review and maybe give approvals on #4735 and https://github.com/whatwg/html/pull/4768/files/4e2a998af84570fda3ff90d45a1882408c9f1cbb..75b444334fe8a77d6e26ee8b4c160ea6cebf3d93?

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Sep 25, 2019

Rebased and updated the PR to use first in flat tree order (+ click focusable for focus). Note that sequential navigation focus delegation already works fine because we already skip hosts with delegatesFocus=true in sequential navigation.

@rakina rakina changed the title Handle shadow host with delegatesFocus=true in Element.focus()/focusing steps Handle shadow host with delegatesFocus=true in focusing steps (click focus + focus()) Sep 25, 2019
Copy link
Member

domenic left a comment

I pushed a commit with some editorial changes (some to preexisting text, but I was in the area...), and also added a note explaining what you said about sequential focus navigation. This LGTM now.

@annevk, would you be able to do a final pass here? In particular this is the first use of flat tree ordering in the spec, so a second pair of eyes on that part would be helpful.

@domenic domenic dismissed smaug----’s stale review Sep 26, 2019

Wasn't a review

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Sep 26, 2019

There's a couple things I don't quite understand:

  1. Why do we still reference the box tree for some of this? Wouldn't the box tree also cross shadow tree boundaries, including those without delegate focus? I suspect we should update those to say flat tree as well with the appropriate checks (if flat tree allows for those checks, haven't checked).
  2. How does this deal with recursion? If a shadow tree contains another shadow tree without delegate focus, it seems that only looking at the flat tree would blow straight past that.
@emilio

This comment has been minimized.

Copy link
Contributor

emilio commented Sep 26, 2019

Yeah, using the flat tree directly to dig down in the tree just ignores any other potential shadow tree which may have delegateFocus = false, I think... I think looking at flattened tree ancestors may be ok here, but downwards you get into funny behavior.

What Gecko implements for focus navigation in shadow trees (and as far as I can tell, that also matches Blink and WebKit) is mostly http://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation, why do we need to come up with yet a different algorithm for this case?

The code for focusing an element via focus() is https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/dom/base/nsFocusManager.cpp#1140

The code for click focus handling is here: https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/dom/events/EventStateManager.cpp#3035

And effectively just checks whether the event hasn't been prevented, and looks at focusability of the event target or any flattened tree ancestor.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Sep 26, 2019

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Sep 27, 2019

Why do we still reference the box tree for some of this? Wouldn't the box tree also cross shadow tree boundaries, including those without delegate focus? I suspect we should update those to say flat tree as well with the appropriate checks (if flat tree allows for those checks, haven't checked).

I don't think I understand this, which part are you referring to?

How does this deal with recursion? If a shadow tree contains another shadow tree without delegate focus, it seems that only looking at the flat tree would blow straight past that.

I think this is OK. "delegates focus" doesn't mean "you now can focus on things inside my shadow tree", it only means "don't focus on the host". So even if a child shadow tree has delegatesFocus=false it doesn't mean "don't put focus on things inside", so there should be no problem of just using the flat tree order? As discussed in w3c/webcomponents#830, this behavior makes the focus delegation predictable and interoperable.

Copy link
Member

domenic left a comment

Per discussion in web-platform-tests/wpt#18035 (comment) we need to not use activation behavior here, but instead something like "when a user activates them". For now I think the best cross-reference for "activates" is just an <a href="#activation">. Eventually we may get better explanation for the end-to-end process of users activating things, by integrating with https://w3c.github.io/uievents/#event-flow-activation, but for now that seems good enough.

We may want to add an explicit <p class="note"> stating that focusing is not activation behavior, i.e. dispatching synthetic click events or calling click() will not cause focus.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Sep 27, 2019

Per discussion in web-platform-tests/wpt#18035 (comment) we need to not use activation behavior here, but instead something like "when a user activates them". For now I think the best cross-reference for "activates" is just an <a href="#activation">. Eventually we may get better explanation for the end-to-end process of users activating things, by integrating with https://w3c.github.io/uievents/#event-flow-activation, but for now that seems good enough.

We may want to add an explicit <p class="note"> stating that focusing is not activation behavior, i.e. dispatching synthetic click events or calling click() will not cause focus.

Added, thanks!

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Sep 27, 2019

@rakina ah, thanks, and there's no chance of a descendant host getting focus while it should not?

As for "box tree", it's mentioned twice in HTML's focus sections and it seems somewhat redundant with "flat tree".

@rakina rakina force-pushed the rakina:focusmethod branch from 4bd9f11 to f90ea02 Oct 1, 2019
@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Oct 1, 2019

@rniwa
rniwa approved these changes Oct 12, 2019
Copy link
Collaborator

rniwa left a comment

r=me

@rniwa

This comment has been minimized.

Copy link
Collaborator

rniwa commented Oct 12, 2019

Great. Thanks for all your hard work, @rakina. I'm really excited that this feature is now fully spec'ed and implementable in WebKit. As a mater of fact, I've uploaded the patch to implement this new behavior in https://bugs.webkit.org/show_bug.cgi?id=166484.

Hopefully @smaug---- can do another pass of the PR and we can call it good.

caiolima pushed a commit to caiolima/webkit that referenced this pull request Oct 14, 2019
https://bugs.webkit.org/show_bug.cgi?id=166484
<rdar://problem/29816058>

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

Import W3C tests from web-platform-tests/wpt@a8a89f2.

* web-platform-tests/resources/testdriver-vendor.js:
* web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-click-method-expected.txt: Added.
* web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-click-method.html: Added.
* web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-tabindex-varies-expected.txt: Added.
* web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-tabindex-varies.html: Added.
* web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-tabindex-zero-expected.txt: Added.
* web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-tabindex-zero.html: Added.
* web-platform-tests/shadow-dom/focus/focus-method-delegatesFocus-expected.txt: Added.
* web-platform-tests/shadow-dom/focus/focus-method-delegatesFocus.html: Added.
* web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-negative-delegatesFocus-expected.txt: Added.
* web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-negative-delegatesFocus.html: Added.
* web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-varying-delegatesFocus-expected.txt: Added.
* web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-varying-delegatesFocus.html: Added.
* web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-zero-delegatesFocus-expected.txt: Added.
* web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-zero-delegatesFocus.html: Added.

Source/WebCore:

Implement delegatesFocus as specified in whatwg/html#4796

When the shadow root of an element has delegates focus flag set, focusing on the shadow host would automatically
"delegates" focus to the first focusable element in the shadow tree instead.

The first focusable element is determined as the first element that is programatically focusable or mouse focusable
in the flat tree (composed tree in WebKit's terminology) in the case of the element getting focused via DOM API,
Element.prototype.focus, by via mouse down. In the case of sequential focus navigation (via tab key), it's the
first keyboard focusable element in the tabIndex order.

Tests: imported/w3c/web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-click-method.html
       imported/w3c/web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-tabindex-varies.html
       imported/w3c/web-platform-tests/shadow-dom/focus/click-focus-delegatesFocus-tabindex-zero.html
       imported/w3c/web-platform-tests/shadow-dom/focus/focus-method-delegatesFocus.html
       imported/w3c/web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-negative-delegatesFocus.html
       imported/w3c/web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-varying-delegatesFocus.html
       imported/w3c/web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-zero-delegatesFocus.html

* dom/Element.cpp:
(WebCore::Element::isKeyboardFocusable const): A shadow host with the delegates focus flag is not considered as
keyboard focusable. The rest is taken care of by the existing logic in FocusController.
(WebCore::isProgramaticallyFocusable): Extracted from Element::focus.
(WebCore::findFirstProgramaticallyFocusableElementInComposedTree): Added.
(WebCore::Element::focus): Added the support for delegatesFocus.
* dom/Element.h:
(WebCore::ShadowRootInit::delegatesFocus): Added.
* dom/Element.idl: Ditto.
* dom/ShadowRoot.cpp:
(WebCore::ShadowRoot::ShadowRoot): Added delegatesFocus to the constructor.
* dom/ShadowRoot.h:
* page/EventHandler.cpp:
(WebCore::findFirstMouseFocusableElementInComposedTree): Added.
(WebCore::EventHandler::dispatchMouseEvent): Added the support for delegatesFocus. Uses the first mouse focusable
element in the flat tree (composed tree) order.
* page/FocusController.cpp:
(WebCore::FocusController::findFirstFocusableElementInShadowRoot):
* page/FocusController.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251043 268f45cc-cd09-0410-ab3c-d52691b4dbfc
domenic added 2 commits Oct 16, 2019
@domenic domenic merged commit 133607f into whatwg:master Oct 16, 2019
2 checks passed
2 checks passed
Participation rakina participates on behalf of Google LLC
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.