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

Tests for SVG tabindex/focus/blur #10149

Merged
merged 8 commits into from Dec 6, 2018

Conversation

@dstorey
Copy link
Contributor

dstorey commented Mar 23, 2018

SVG defines focus/blur/tabindex as behaving the same as HTML.

This takes some tests from HTML and adapts to the SVG template used in the SVG folder. I included them in the interact folder as that page (scripting) is the one that defines which elements are focusable in SVG.

Apologies if I got the folder structure wrong. I believe I suppose to put the tests in the in the scripted folder.

Note: SVG say that the following are focusable by default:

I only included a test for the root-most SVG element, as SVG further defines that zoom and pan controls only work on the root most SVG element, so it will pass with or without that control.

I also assumed that video/audio with user controls meant the controls attribute.

These tests match the behavior of the HTML equivalents. Edge fails the tabindex tests when checking the content attribute as there is a non-tabindex related bug with attribute reflection in SVG where the canonical case is lowercase. Otherwise those tests would pass.

I couldn't find any HTML tests specific to blur() so I reused the composed test. blur() passes in Edge, but doesn't support event.composed.

I didn't add tests for dataset (which is in the same section of the spec) as there are already tests in HTML that detect dataset on SVG

dstorey added some commits Mar 14, 2018

Add test for default tabIndex value
Adapted from html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-default-value.html
Add focus management test
Adapted from HTML version at /html/editing/focus/focus-management/focus-events.html
Add focus composed test
Based on HTML's composed.window.js
@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 23, 2018

Build ERRORED

Started: 2018-03-24 01:20:42
Finished: 2018-03-24 01:28:59

Failing Jobs

  • chrome:dev

View more information about this build on:

@dstorey

This comment has been minimized.

Copy link
Contributor Author

dstorey commented Mar 23, 2018

Hmmm works on my machine :D

@AmeliaBR

This comment has been minimized.

Copy link
Contributor

AmeliaBR commented Mar 23, 2018

Thanks for this, David! (When I discovered Edge's tabIndex vs tabindex bug last week, my first thought was "this is why we need tests!!!")

I agree it makes sense to put these in the interact folder, since that's where the behavior is defined, even if the attribute is defined in structure. Also, non-interactive user agents can skip over these tests.

ForzoomAndPan, the distinction from a svg:root was I think to cover the case of a zoomable inline SVG. But doesn't really matter, since none of the major browsers support SVG zoom and pan, other than by browser zoom and scrollbars on a document window. The spec intentionally says "if it has controls" for that reason, rather than talking about the attribute values.

Some thoughts:

  • There should be tests that nested svg don't get the focusable-by-default behavior that root SVG do.

  • Add tests for normally-rendered elements (e.g., a[href] and path[tabindex=0]) that are not rendered because of being inside defs, symbol, pattern, etc. (At least some browsers let them get focus, last I checked!) Also for elements that have display: none (pretty sure browsers are better with those) and shape elements that are not rendered because of invalid geometry properties (haven't tested).

  • Conversely, add a test that visibility: hidden doesn't cancel focusability (as defined in https://svgwg.org/svg2-draft/render.html#VisibilityControl ) (this will probably fail & so should probably be marked as risk, but it's an accessibility thing related to the fact that these elements can still be sensitive to pointer events in SVG).

  • Is there a way to distinguish the "shoulds" tests (e.g., focusable support) from the "musts"?

  • You might want to separate out tests relating to HTML embedded elements into a separate file, so it can clearly be dependent on underlying support of those elements in SVG contexts.

  • Is there a purpose to svg/interact/scripted/resources/blank.htm ?


Additional tests still required for full coverage of this section (not necessarily in this PR, just keeping note):

  • tests for correct tab order calculation (e.g., when an element has a positive tabindex value)

  • inline svg tests (check default focusable state of non-root svg element, make sure tabindex still works, make sure that the tab order is correctly calculated on a document-wide basis)

  • Also, might want to add a test of HTML focusable elements inside <foreignObject>.

  • make sure scroll-into-view on focus works in both SVG docs and inline SVG in HTML

  • check that 'click' events are fired for keyboard activation of a focused element

  • tests for focus handling of elements inside use-element shadow DOM

  • tests of :focus styles (default styles are applied, and custom styles can be used to override)

@dstorey

This comment has been minimized.

Copy link
Contributor Author

dstorey commented Mar 23, 2018

Yeah, I think some of these would probably be a separate PR as they're not covered by the HTML versions either (presumably :focus is in a CSS test for example), and perhaps the existing tests in this PR would be enough to get the HTML PR merged for adding the mixin we require to move these definitions into HTML.

I'll look into an additional PR though that adds things that don't need to be in here. the foreignObject/use maybe should be in this one. I guess I could just have a duplicate test in the flags test inside a use element for that one. I've not used use for a long time so will have to look it up again :)

(Edge is fixing our reflect bugs. Now our tree work is done we're working on improving how we deal with attributes)

@dstorey

This comment has been minimized.

Copy link
Contributor Author

dstorey commented Mar 23, 2018

Re: should vs must: I don't think so. Edge passes them at least :D My understanding from HTML is most of the focusable by default elements are "should" anyway (or at least there is language about being up to the user agent/platform conventions, so it is probably fine to include them.

re: blank.html. The text mentioned valid (nested) browsing context, so I added a document. I guess I could add a data URI or such too but I just copied what was used in other tests and didn't want to deal with encoding quotes and angle brackets. I wasn't 100% sure if the definition meant if it is just an iframe with no html/svg document that it shouldn't be focusable.

Re: controls: do you mean zoomAndPan or video/audio, as it isn't 100% clean that the latter means the controls attribute?

Re: non root SVG: I thought I added a basic test for this but may be mistaken. I'll look into it.

annevk added a commit to whatwg/html that referenced this pull request Mar 23, 2018

Add HTMLOrSVGElement interface mixin
This exposes dataset, tabIndex, focus(), and blur() on SVG elements.

It also does away with NoncedElement.

Tests: web-platform-tests/wpt#10110 and web-platform-tests/wpt#10149 (dataset tests for SVG already exist).

Fixes #3471 and fixes w3c/svgwg#60.
@AmeliaBR

This comment has been minimized.

Copy link
Contributor

AmeliaBR commented Mar 23, 2018

Re: should vs must

Ah, I was thinking of the CSS test metadata. I'd like to get to the point of adding similar metadata for all SVG tests (plus SVG-specific metadata, like which processing modes the test is relevant to), but that will wait until we have an agreed-upon metadata system.

Re: blank.html

Ah, ok. Makes sense.

Re: controls: do you mean zoomAndPan or video/audio, as it isn't 100% clean that the latter means the controls attribute?

I meant zoomAndPan, but the general wording is used for both, and for the same reasons. For audio/video controls, the user agent is supposed to add controls even without the attribute if scripting is disabled, and may add controls regardless (in practice, that is usually with a context menu option), so again the focusability rule just focused on whether the controls are there.

But if the default behavior in HTML is to not make the element focusable just for context menu controls, then we can match that. Or leave out the no-attribute test & only test that the elements are focusable if the author has specifically requested user-agent controls.

Re: non root SVG: I thought I added a basic test for this but may be mistaken. I'll look into it.

You had a test of tabindex working on a non-root SVG, I suggested adding a test of the default behavior being non-focusable.

@dstorey

This comment has been minimized.

Copy link
Contributor Author

dstorey commented Mar 23, 2018

Makes sense. I'll remove the test that tests audio/video fail if no controls as you mention as there is no way of detecting if the UI adds their own except with [controls].

Respond to feedback
Remove test for audio/video without controls and add non root svg

yutakahirano added a commit to yutakahirano/html that referenced this pull request Apr 26, 2018

Add HTMLOrSVGElement interface mixin
This exposes dataset, tabIndex, focus(), and blur() on SVG elements.

It also does away with NoncedElement.

Tests: web-platform-tests/wpt#10110 and web-platform-tests/wpt#10149 (dataset tests for SVG already exist).

Fixes whatwg#3471 and fixes w3c/svgwg#60.
@svgeesus

This comment has been minimized.

Copy link
Contributor

svgeesus commented Sep 6, 2018

Hmm looks like the error is caused by WPT moving to a new repo.

@wpt-pr-bot wpt-pr-bot removed the request for review from boggydigital Sep 6, 2018

@svgeesus

This comment has been minimized.

Copy link
Contributor

svgeesus commented Sep 23, 2018

@AmeliaBR I would like to get these merged, because this PR is languishing and also because it may be easier to merge and then do a separate PR for any fixes, rather than ask @dstorey to fix them. I'm therefore going to give an approving review.

@svgeesus
Copy link
Contributor

svgeesus left a comment

Yes, let's fix up any details after merge.

@svgeesus

This comment has been minimized.

Copy link
Contributor

svgeesus commented Sep 23, 2018

@foolip I think this old PR needs admin to override the Travis CI failure caused by the repo having moved.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Sep 27, 2018

@svgeesus, I've merged in origin/master into the review branch, which has re-triggered Travis. Just make sure to squash this when merging as the history will be very silly otherwise.

@svgeesus

This comment has been minimized.

Copy link
Contributor

svgeesus commented Nov 29, 2018

Still getting Travis CI failure from chrome:dev stability failure, (but not getting the "repo moved" failure, thanks!)

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Dec 5, 2018

@svgeesus Travis last ran 2 months ago. Those checks are no longer blocking. I restarted but it's not picking up changes to the Travis config that have happened on master it seems. I'll just merge in origin/master again.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Dec 6, 2018

Alright, everything is fine now :)

@foolip foolip merged commit 4bdeca6 into web-platform-tests:master Dec 6, 2018

2 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Dec 6, 2018

Bah! I picked the "rebase and merge" option, but that failed. When I hit the "try again" button eventually it was merged, but squashed. So there's now just a single commit in history, where it's not as clear which comments refer to which test files. Sorry :(

alice added a commit to alice/html that referenced this pull request Jan 8, 2019

Add HTMLOrSVGElement interface mixin
This exposes dataset, tabIndex, focus(), and blur() on SVG elements.

It also does away with NoncedElement.

Tests: web-platform-tests/wpt#10110 and web-platform-tests/wpt#10149 (dataset tests for SVG already exist).

Fixes whatwg#3471 and fixes w3c/svgwg#60.
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.