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: Add manual accessibility tests for fieldset #12691

Merged
merged 10 commits into from
Mar 3, 2019

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Aug 27, 2018

No description provided.

============================

These tests are intended to test the accessibility of the fieldset and legend elements. They do not
yet have pass conditions or a clear way to run them. This is expected to be addressed later.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes them rather hard to review, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. :/ I don't know yet what expected behavior should be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see w3c/html-aam#145

@zcorpan
Copy link
Member Author

zcorpan commented Aug 29, 2018

If https://github.com/WICG/aom/blob/gh-pages/explainer.md#full-introspection-of-an-accessibility-tree---computedaccessiblenode is implemented, it could be used by these tests as normal testharness.js tests.

@zcorpan
Copy link
Member Author

zcorpan commented Sep 2, 2018

I see accname/ has some tests for accessible name (but not for fieldset), and although the tests are marked as manual, they have some script to formalize the expected result. I'm not familiar with that. @halindrome @joanmarie should these tests be in accname/ with that convention? How are those tests run?

@zcorpan
Copy link
Member Author

zcorpan commented Sep 7, 2018

@halindrome @joanmarie ping

@joanmarie
Copy link
Contributor

The tests are run via tools created by the ARIA working group which we hope to integrate into WPT, but which are not yet ready for general consumption due to bugs, lack of documentation, etc.

As for where the tests should live, we should get @stevefaulkner and @scottaohara to chime in. The HTML AAM has normative content regarding Accessible Name calculation specific to HTML and which overrides what is in the Acc Name spec.

@zcorpan
Copy link
Member Author

zcorpan commented Sep 12, 2018

OK, thank you. Maybe we can leave these tests as manual until those tools are ready?

@zcorpan
Copy link
Member Author

zcorpan commented Sep 17, 2018

@mcking65 are you able to take a look at this?

Copy link
Contributor

@asurkov asurkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall it looks good, sorry it took that long

@@ -0,0 +1,7 @@
<!doctype html>
<title>fieldset accessibility test: ARIA</title>
<div role=group aria-labelledby=legend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears people tends to put attribute values into quotes in this repo, see for example [1]. I'd say it makes sense to keep that style here too, unless you have a reason for this.

[1] https://github.com/web-platform-tests/wpt/blob/master/html/semantics/edits/the-del-element/del_effect.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a style guide, and unquoted attributes certainly are used elsewhere in wpt.

<div id=legend>Foo</div>
<input>
</div>
<p>Expected accessible name: "Foo"
Copy link
Contributor

@asurkov asurkov Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to be explicit here and point out which accessible object you refer to. For example,

Expected accessible name for div@role=group: Also you may want to give @id to div@role=group and use it instead, like

Expected accessible name for @id=fieldset:

I'm sure HTML allow to not close < p > element, but I sort of like to keep explicit

. I'm not sure though what policies of this repo are, so I leave it to you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also curious why the test are restricted to fieldset accessible name only. Should it have accessible role as well?

</style>
<fieldset>
<div>
<legend>Foo</legend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to HTML spec [1], legend has to be a first child of fieldset. So you're right the browsers shouldn't pick up accessible name from a legend, which is not a direct child. I'd be nice to have a comment in the test or extend <title> to reflect this.

[1] https://html.spec.whatwg.org/multipage/form-elements.html#the-legend-element

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an authoring conformance requirement. The UA conformance requirement is

https://w3c.github.io/html-aam/#fieldset-element-accessible-name-computation

step 2

<!doctype html>
<title>fieldset accessibility test: legend child display: none</title>
<style>
legend > span { display: none; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the test is not legend specific, but it's ok to have it if you want

<!doctype html>
<title>fieldset accessibility test: legend role=group aria-labelledby=fieldset</title>
<fieldset id=fieldset>
<legend role=group aria-labelledby=fieldset>Foo</legend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a tricky one, the output looks reasonable, but I didn't run it through the spec name computation alg

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the spec side is pretty messy. I filed an issue but can't find it now. Basically I think accname should have hooks for html-aam to use when defining fieldset/legend.

<title>fieldset accessibility test: multiple legends</title>
<fieldset>
<legend>Foo</legend>
<legend>Bar</legend>
Copy link
Contributor

@asurkov asurkov Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a good one, might be worth to have a test where legend is not a first child

<legend slot="my-text">Foo</legend>
</my-fieldset>

<p>Expected accessible name: ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm, why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessible name computation works on the DOM tree, not the flat tree.

<!doctype html>
<title>fieldset accessibility test: legend visibility: hidden</title>
<style>
legend { visibility: hidden; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be worth to have display:none/visibility:hidden placed on fieldset element. Also might worth testing visibility:collapsed and visibility:visible

@zcorpan
Copy link
Member Author

zcorpan commented Oct 18, 2018

I can't find the spec for accessible name and descriptions anymore... w3c/accname#34

@zcorpan
Copy link
Member Author

zcorpan commented Oct 18, 2018

OK addressed comments, thank you @asurkov

@zcorpan
Copy link
Member Author

zcorpan commented Mar 3, 2019

Rebased on latest master. Should be good to merge if bots are green.

@sideshowbarker sideshowbarker merged commit 7b032f0 into master Mar 3, 2019
@sideshowbarker sideshowbarker deleted the zcorpan/fieldset-accessibility branch March 3, 2019 22:01
@zcorpan
Copy link
Member Author

zcorpan commented Mar 3, 2019

Thanks Mike!

@sideshowbarker
Copy link
Contributor

Cheers 🍫

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants