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 accname tests for “Hidden not referenced” step #40074

Merged

Conversation

adampage
Copy link
Member

@adampage
Copy link
Member Author

Just getting my feet wet here. Could I trouble you for some initial thoughts, @cookiecrook?

These failures related to content-visibility: hidden and aria-hidden="true" make me wonder if I’m misunderstanding the accname spec, or if additional language is needed to handle elements that meet the spec’s definition of “hidden” but that also remain focusable? (related: w3c/aria#1856)

@adampage adampage requested a review from cookiecrook May 18, 2023 05:59
@cookiecrook
Copy link
Contributor

[error]We stopped hearing from agent Hosted Agent. Verify the agent machine is running and has a healthy network connection. Anything that terminates an agent process, starves it for CPU, or blocks its network access can cause this error. For more information, see: https://go.microsoft.com/fwlink/?linkid=846610
Agent: Hosted Agent
Started: Today at 10:31 PM

Well, at least that explains why there are no results for WebKit.

accname/name/comp_hidden_not_referenced.html Outdated Show resolved Hide resolved
accname/name/comp_hidden_not_referenced.html Outdated Show resolved Hide resolved
accname/name/comp_hidden_not_referenced.html Outdated Show resolved Hide resolved
@cookiecrook
Copy link
Contributor

cookiecrook commented Jul 7, 2023

I thought of another example: the collapsed (hidden) contents of summay/details, when a role is applied that gets namefrom contents.

<summary role="comment" data-expectedlabel="visible contents" data-testname="namefrom:contents(comment role) on summary w/ hidden details" class="ex">
  visible contents
  <details>hidden contents</details>
</summary>

@adampage adampage force-pushed the accname-comp-hidden-not-referenced branch from 7675245 to 6770885 Compare August 16, 2023 23:01
@adampage
Copy link
Member Author

I thought of another example: the collapsed (hidden) contents of summay/details, when a role is applied that gets namefrom contents.

<summary role="comment" data-expectedlabel="visible contents" data-testname="namefrom:contents(comment role) on summary w/ hidden details" class="ex">
  visible contents
  <details>hidden contents</details>
</summary>

My brain broke on this one. 😅 First of all, <details> should enclose <summary> here, right? Not the other way around? And then the intention is to test the accessible name of the nested summary when it’s given an explicit role allowing name form contents?

@adampage
Copy link
Member Author

adampage commented Aug 17, 2023

Hey @cookiecrook, thanks again for your patience, for your feedback, and for all the test case suggestions. With the exception of the <summary role="comment"> one, I believe I’m all caught up, so I went ahead and resolved your previous comments.

In the case of aria-hidden="false" elements nested within aria-hidden="true", it seems neither Chrome nor Safari succesfully un-hide the nested content, which appears to put them in conformance with both ARIA 1.2 and the current editor’s draft, but I also see there’s still ongoing conversation in ARIA #1256. In any case, I left these WPT tests aspirationally expecting that aria-hidden="true" will in fact re-expose that content to the accessibility tree.

One weird behavior I noticed is that one of the two failing cases — #8 — actually does appear to kind of work in reality, at least when testing in macOS with VoiceOver (for both Chrome and Safari). When I move the virtual cursor to the rendered heading, I do get the expected announcement of “visible to all users, un-hidden for all users”. In Chrome, the devtools accessiblity tree shows both intended StaticText nodes as exposed, but then it also shows the parent heading as having an accessible name matching just the first node:

image

So that makes me suspicious that perhaps there’s a bug/feature in VoiceOver where it announces the full contents of the heading on focus of the virtual cursor, rather than the computed accessible name? As more interesting evidence, the rotor identifies the heading by the computed accessible name:

image

I’ll plan to do a little more testing of that in Windows to see how JAWS and NVDA behave.

@adampage adampage marked this pull request as ready for review August 17, 2023 03:38
@cookiecrook
Copy link
Contributor

First of all, <details> should enclose <summary> here, right? Not the other way around? And then the intention is to test the accessible name of the nested summary when it’s given an explicit role allowing name form contents?

Yes, sorry. I think this markup is right now, but I'm no longer sure of the expectation.

<details role="comment" data-expectedlabel="visible contents" data-testname="namefrom:contents(comment role) on summary w/ hidden details" class="ex">
  <summary> visible contents </summary>
  hidden contents
</details>

Hmm… I'm no longer sure the above is expected, because I think it'd be reasonable for implementations to not provide a backing accessibility node for <details> itself when only the <summary> is displayed. Probably just add a todo comment.

@cookiecrook
Copy link
Contributor

I left these WPT tests aspirationally expecting that aria-hidden="true" will in fact re-expose that content to the accessibility tree.

Since it's still ambiguous, we should probably comment those tests, with a link to the blocking spec issue.

@cookiecrook
Copy link
Contributor

that makes me suspicious that perhaps there’s a bug/feature in VoiceOver

Do you have a permanent link to that test case? I can write up a bug and start to determine if it's an engine problem or a VO problem.

Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Tests look good overall... Thanks for all your hard work on this one!

Minor nits below about testname and label names for clarity and permalink longevity.

accname/name/comp_hidden_not_referenced.html Show resolved Hide resolved
accname/name/comp_hidden_not_referenced.html Outdated Show resolved Hide resolved
accname/name/comp_hidden_not_referenced.html Outdated Show resolved Hide resolved
accname/name/comp_hidden_not_referenced.html Outdated Show resolved Hide resolved
@adampage
Copy link
Member Author

adampage commented Oct 2, 2023

Do you have a permanent link to that test case? I can write up a bug and start to determine if it's an engine problem or a VO problem.

@cookiecrook well, my premise with this test case was that an element with visibility:visible should be expected to override an ancestor’s visibility:hidden and surgically re-expose its own contents to the accessibility tree. But now I’m failing to find any spec’d documentation in either ARIA or CSS to support that expectation.

Our 1.3 editors draft doesn’t seem to make any exceptions for descendants of an element with visibility:hidden:

Elements, including their descendent elements, that have host language semantics specifying that the element is not displayed, such as CSS display:none, visibility:hidden, or the HTML hidden attribute.

But as far as interop goes, it does seem like there’s a test case here worth pursuing, since Firefox passes the test case (as written) while Chrome fails and Safari fails.

A strict reading of that language in the ARIA 1.3 spec suggests that I should keep the test case but change the expected label to “visible to all users,”, then file a Mozilla bug?

@adampage
Copy link
Member Author

adampage commented Oct 2, 2023

@cookiecrook besides this last comment, I believe I’ve addressed all your feedback with my latest batch of commits. Please let me know what you think?

Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Approving either way, but with a minor preference for removing the commented code.

Comment on lines 49 to 51
<!-- TODO: Restore the following test once expectation for `aria-hidden="false"`
<!-- behavior is documented. See: -->
<!-- https://github.com/w3c/aria/issues/1256 -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you worked hard on these, but I think it would be better to check in the patch without the commented code, and only add specific examples once the new issue came to resolution... Otherwise someone might see 1256 is resolved (in the future) and uncomment these even if they don't align with the resolution… Or also bad, these could just sit around commented indefinitely.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just keep your test names, since they should be clear enough to show what needs to be tested? such as:

<!-- 
  Todo: once https://github.com/w3c/aria/issues/1256 resolved.

  - button labelled by an element that is aria-hidden=true which contains a nested child that is aria-hidden=false 
  - test name 2
  - test name 3
  - etc.
-->

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for working so hard on these!

Copy link
Contributor

@cookiecrook cookiecrook Oct 5, 2023

Choose a reason for hiding this comment

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

Acknowledging that August James asked you to comment them, and October James asked you to remove them, so I won't stop you from squash merging as-is... Take your pick. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha ha, no trouble at all, @cookiecrook — happy for the opportunity to contribute at all. I went ahead and removed the test cases that are blocked by aria#1256 but left a comment identifying each one by name. 👍🏻

Based on our quick conversation in the WPT meeting, I also left the expectation as is for the ...visibility:hidden with nested content that is visibility:visible test case, so Firefox is currently the only pass for that one.

@adampage adampage merged commit 4e061fd into web-platform-tests:master Oct 6, 2023
15 of 18 checks passed
cookiecrook pushed a commit to cookiecrook/wpt that referenced this pull request Oct 11, 2023
…s#40074)

* add tests

* replace all tests

* remove numbers from test names

* disambiguate “hidden” terminology in test names

* add todo comment for future details test

* comment out aria-hidden=false test cases

* finesse test name

* lint fix

* remove test cases blocked by aria web-platform-tests#1256
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this pull request Dec 11, 2023
…s#40074)

* add tests

* replace all tests

* remove numbers from test names

* disambiguate “hidden” terminology in test names

* add todo comment for future details test

* comment out aria-hidden=false test cases

* finesse test name

* lint fix

* remove test cases blocked by aria web-platform-tests#1256
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AccName: tests/todos in accname/name/comp_hidden_not_referenced
4 participants