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

HTML: Sequential focus navigation with shadow dom #17523

Merged
merged 4 commits into from Aug 15, 2019

Conversation

@rakina
Copy link
Contributor

rakina commented Jun 26, 2019

See whatwg/html#4735 for the spec change.

I used the term "flat tree order" in some variable names and comments, but it isn't defined in the spec so it might be confusing. However I'm not sure what else I can use since "composed tree" is also not in the spec, and "shadow-including tree order" is a little bit different because of slots.

Copy link
Member

domenic left a comment

Some style nits, but this overall looks good. We may also want to move into shadow-dom/

It would be good to get a second implementers' eye on the setup + expected orders (which are the most important parts of the tests). @rniwa maybe :)

// <div #belowSlot tabindex=-1>
// <div #belowHost tabindex=0>
async_test(async (t) => {

This comment has been minimized.

Copy link
@domenic

domenic Jun 27, 2019

Member

This could be promise_test( and then you wouldn't need t.step or t.done()

This comment has been minimized.

Copy link
@rakina

rakina Jun 27, 2019

Author Contributor

Done.

setTabIndex([aboveHost, host, belowHost], 0);
resetFocus();
const expectedOrder = [aboveHost, host, belowHost];
for (let el of expectedOrder) {

This comment has been minimized.

Copy link
@domenic

domenic Jun 27, 2019

Member

Can this be a utility function too? It seems like the loop is the same in all tests.

await assertFocusOrder([aboveHost, host, belowHost]);

This comment has been minimized.

Copy link
@rakina

rakina Jun 27, 2019

Author Contributor

Done.

}
}
function navigateFocusForward() {

This comment has been minimized.

Copy link
@domenic

domenic Jun 27, 2019

Member

Can you just use the shared one and then say const focused = shadowRoot.activeElement || document.activeElement afterward?

This comment has been minimized.

Copy link
@rakina

rakina Jun 27, 2019

Author Contributor

Done.

@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Jun 27, 2019

cc @annevk and/or @smaug---- for Firefox?

@@ -69,3 +70,14 @@ function navigateFocusForward() {
return test_driver.send_keys(document.body, "\ue004");
}

function assertFocusOrder(expectedOrder) {

This comment has been minimized.

Copy link
@domenic

domenic Jul 2, 2019

Member

The return new Promise isn't needed here. Just make the whole function async, and then put the loop body inside the function:

async function assertFocusOrder(expectedOrder) {
  let shadowRoot = document.getElementById("host").shadowRoot;
  for (let el of expectedOrder) {
    await navigateFocusForward();
    let focused = shadowRoot.activeElement ? shadowRoot.activeElement : document.activeElement;
    assert_equals(focused, el);
  }
}

This comment has been minimized.

Copy link
@rakina

rakina Jul 9, 2019

Author Contributor

Oh awesome, thanks :D

@rniwa

This comment has been minimized.

Copy link
Contributor

rniwa commented Aug 14, 2019

All but one test (focus-tabindex-order-shadow-zero-delegatesFocus.html) with delegatesFocus set to true pass in WebKit so these tests look good to me. Perhaps we can land all tests which doesn't involve delegatesFocus right now so that we can test coverage on things we ought to have interoperability.

Will ad the delegatesFocus test in the delegatesFocus PR instead (#18035)
@rakina

This comment has been minimized.

Copy link
Contributor Author

rakina commented Aug 15, 2019

All but one test (focus-tabindex-order-shadow-zero-delegatesFocus.html) with delegatesFocus set to true pass in WebKit so these tests look good to me. Perhaps we can land all tests which doesn't involve delegatesFocus right now so that we can test coverage on things we ought to have interoperability.

Yay, thanks for trying it out. I've removed the delegatesFocus test, will move that to #18035.

@rniwa
rniwa approved these changes Aug 15, 2019
@rniwa rniwa merged commit 888da5c into web-platform-tests:master Aug 15, 2019
9 of 10 checks passed
9 of 10 checks passed
wpt.fyi - firefox[experimental] Firefox results
Details
Azure Pipelines Build #20190815.26 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - safari[experimental] Safari results
Details
natechapin added a commit to natechapin/wpt that referenced this pull request Aug 23, 2019
…#17523)

* HTML: Sequential focus navigation with shadow dom

The delegatesFocus test will be in the delegatesFocus PR (web-platform-tests#18035)
domenic added a commit to rakina/html that referenced this pull request Sep 24, 2019
This defines an explicit list for the document's "sequential focus
navigation order", whose contents are defined to include elements in
shadow trees. Previously the contents of the sequential focus navigation
order were defined mostly implicitly, in the tabindex section.

This also expands the ordering requirements for sequential focus
navigation order to account for shadow trees and slotted elements.

Finally, this has a minor connection to delegatesFocus, in that it
excludes elements that are shadow hosts with delegatesFocus = true from
being focusable areas.

Part of whatwg#2013.

Tests: web-platform-tests/wpt#17523
domenic added a commit to whatwg/html that referenced this pull request Sep 24, 2019
This defines an explicit list for the document's "sequential focus
navigation order", whose contents are defined to include elements in
shadow trees. Previously the contents of the sequential focus navigation
order were defined mostly implicitly, in the tabindex section.

This also expands the ordering requirements for sequential focus
navigation order to account for shadow trees and slotted elements.

Finally, this has a minor connection to delegatesFocus, in that it
excludes elements that are shadow hosts with delegatesFocus = true from
being focusable areas.

Part of #2013.

Tests: web-platform-tests/wpt#17523
zcorpan added a commit to whatwg/html that referenced this pull request Nov 6, 2019
This defines an explicit list for the document's "sequential focus
navigation order", whose contents are defined to include elements in
shadow trees. Previously the contents of the sequential focus navigation
order were defined mostly implicitly, in the tabindex section.

This also expands the ordering requirements for sequential focus
navigation order to account for shadow trees and slotted elements.

Finally, this has a minor connection to delegatesFocus, in that it
excludes elements that are shadow hosts with delegatesFocus = true from
being focusable areas.

Part of #2013.

Tests: web-platform-tests/wpt#17523
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.