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

fix(link-name): pass landmark content as link text #2617

Merged
merged 3 commits into from
Nov 9, 2020

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Nov 2, 2020

To figure this one out I tested the following code:

<label for="bar" id="foo">
	<a href="/" id="baz">
		<div role="alert" aria-label="Goodbye">Hello</div>
		<div role="textbox" aria-label="mars">earth</div>
		<div role="img">people</div>
	</a>
</label>
<input type="button" aria-labelledby="foo" id="bar" />

The surprising thing is that AT gave different names than the browsers do. The button works fairly consistently, but most AT seem to have something in place to deal with this a[href] > article:not([aria-label]) scenario. Here are my test results:

(either aria-labelledby or explicit label)

Browser (+AT) Link Button
Chrome name Goodbye mars Goodbye mars
NVDA + Chrome Hello mars Goodbye mars
JAWS + Chrome Hello slash Goodbye mars
Firefox name Goodbye mars earth Goodbye mars earth
NVDA + Firefox Hello mars Goodbye mars
Safari name Goodbye mars people Goodbye mars
VO + Safari Goodbye earth mars Goodbye mars
JAWS + IE Hello earth people Goodbye mars

Versions: Firefox 82, Chrome 86, Safari 14, IE11, VO (MacOS) Catalina, JAWS 2020, NVDA 2020.3

Results are completely all over the place, so to avoid false positives what I've done is chosen the most optimistic scenario, which is that when looking for name from content of an element that isn't a label, all descendants should be considered, even elements that are named from author, except if the element that is named from author has an accessible name already through some other means.

This PR does not guarantee the outcome will work everywhere. Instead it avoids the false positive. I'm opening a ticket to look at this more closely when we're not days away from a code freeze.

Closes issue: #1461

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

if (
alreadyProcessed(virtualNode, context) ||
!namedFromContents(virtualNode, { strict })
virtualNode.props.nodeType !== 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even when subtreeDescendant is true, we need to return an empty string for things that aren't nodes.

// Chrome 72: This is This is a label of
// Firefox 62: This is ARIA Label
// Safari 12.0: This is This is a label of
// Chrome 86: This is This is a label of
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we marking what the browser's accessibility tree says or what screen readers with those browsers say?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the browser's name in the acc tree.

straker
straker previously requested changes Nov 3, 2020
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

We should add the example code you used as a test case for this change so we can make sure future changes affect it.

@WilcoFiers WilcoFiers merged commit e77992e into develop Nov 9, 2020
@WilcoFiers WilcoFiers deleted the link-landmark-text branch November 9, 2020 15:03
@goodwitch
Copy link
Contributor

Hey @WilcoFiers is this really a common situation. I was just chatting about this with Michael Fairchild...and he wonders how common that issue is?

It seems like having a textbox wrapped by a link would be super rare (and also invalid if this were entirely HTML, but the mix of html/aria means it’s probably technically valid code and would pass 4.1.1)

What am I getting at? If screenreader or browser devs see this code reported in an issue, they might say “yeah, that’s not valid and such an edge case, I’ll put this at the lowest possible priority to fix”.

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

Successfully merging this pull request may close these issues.

3 participants