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(region): Decorative images ignored by region rule #4412

Merged
merged 11 commits into from
May 3, 2024

Conversation

gaiety-deque
Copy link
Contributor

Issue was around decorative images, specifically 1 pixel wide/tall marketing tracking images, that get added outside of regions failing the region rule. An easy and fairly robust solution the issue opener agreed with was ignoring images with alt='' and so that's what this PR implements.

Dev notes:

  • Added isPresentationGraphic/1 check which currently only handles alt='' and ignores them for the region rule
  • role='presentation' test added as well, but this already worked previously prior to this code change
  • svg I believe is handled differently already, there's a test called treats svg elements as regions so the new function doesn't check for svg's

fix: #4145

added isPresentationGraphic check which currently only handles alt='' and ignores them for the region rule
svg I believe is handled differently already
role='presentation' test added as well, use-case already handled

fix: #4145
my IDE was adding tabs ew, replaced with spaces

Refs: #4145
@gaiety-deque gaiety-deque marked this pull request as ready for review April 16, 2024 18:19
@gaiety-deque gaiety-deque requested a review from a team as a code owner April 16, 2024 18:19
if (
getRole(virtualNode) === 'button' ||
isRegion(virtualNode, options) ||
['iframe', 'frame'].includes(virtualNode.props.nodeName) ||
(dom.isSkipLink(virtualNode.actualNode) &&
dom.getElementByReference(virtualNode.actualNode, 'href')) ||
!dom.isVisibleToScreenReaders(node)
!dom.isVisibleToScreenReaders(node) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This brings up an interesting question. Should a presentational img using alt="" be considered not visible to screen readers? @WilcoFiers is the better change to update isVisibleToScreenReaders to return false for an img with alt=""? Right now isVisibleToScreenReaders returns true for an img with empty alt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know I was wondering that, it was a path I started diving down and then uncertainty got the best of me. It's a function leveraged in a lot of places.

If we think that's the right path I'm happy to try it out and see if any other test failures come up along the way that make us second guess it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair question, but no I don't think so. Conceptually, there's a difference between a hidden element and an element that's ignored. It matters for example on <img role="button" alt="">. That should fail for having an inappropriate role, whereas <img role="button" aria-hidden="true" alt=""> should be incomplete.

Hellothuy1

This comment was marked as spam.

if (
getRole(virtualNode) === 'button' ||
isRegion(virtualNode, options) ||
['iframe', 'frame'].includes(virtualNode.props.nodeName) ||
(dom.isSkipLink(virtualNode.actualNode) &&
dom.getElementByReference(virtualNode.actualNode, 'href')) ||
!dom.isVisibleToScreenReaders(node)
!dom.isVisibleToScreenReaders(node) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair question, but no I don't think so. Conceptually, there's a difference between a hidden element and an element that's ignored. It matters for example on <img role="button" alt="">. That should fail for having an inappropriate role, whereas <img role="button" aria-hidden="true" alt=""> should be incomplete.

@@ -86,6 +91,10 @@ function findRegionlessElms(virtualNode, options) {
}
}

function isPresentationGraphic(virtualNode) {
return virtualNode.props.nodeName === 'img' && virtualNode.attr('alt') === '';
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a little incomplete. There are other graphics, and there are more ways for an image to be decorative. We have methods we can use to figure this stuff out:

Suggested change
return virtualNode.props.nodeName === 'img' && virtualNode.attr('alt') === '';
return aria.isVisualContent(virtualNode.actualNode) && text.accessibleTextVirtual(virtualNode) === '';

Copy link
Contributor Author

@gaiety-deque gaiety-deque Apr 18, 2024

Choose a reason for hiding this comment

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

Hmm I agree, alas I'm a bit unsure which existing methods are the most appropriate.

Your suggestion is close, however accessibleTextVirtual never actually checks for an alt attribute in its computational steps. Edit: nevermind, it does

The closest I've found is hasAltEvaluate in shared but that isn't actually used anywhere and only returns a boolean. Edit: Nevermind I misunderstood what this was for

However, I think findTextMethods should work as it uses nativeTextMethods which checks for alt text - but that gives me an empty string even without an alt attribute on the image. Edit: Nevermind again

Think I'll post asking for pairing assistance tomorrow to learn how to fish for the right answer on this one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WilcoFiers I paired with Steve and we generally agree that it'd be nice to make this check more broad, however isVisualContent is perhaps too broad for what it checks for. We currently seem to lack a shared utility for checking if something is a graphic (as far as we could tell) so one path forward may be writing one.

We would both love to circle back with you on Monday @WilcoFiers about what we'd like to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a call we discussed we wanted to check for the tags img, svg, object and canvas and to then use accessibleTextVirtual but that function does not work for our needs, as it seems to return an empty string regardless of if an img tag has no alt attribute (like the original issue) or an empty alt attribute.

My findings copied from Slack below:


It (accessibleTextVirtual) seems to be written deeply to rely on empty strings all over the place as far as I can tell.

  • accessibleTextVirtual's main reduce initializes the value as ''
  • ^ it also tries to sanitize the value, which if the value is falsey at all returns an empty string
  • ^ the reduce returns immediately if the value is anything that isn't an empty string
  • nativeTextAlternative's main reduce also initializes the value as ''
  • ^'s uses several functions in its reduce including findTextMethods finds and uses native-text-methods.js's attrText function which returns the attribute or an empty string meaning it'll never return null

I tried adjusting all of these places but it got pretty hairy and makes me think this was all set up intentionally the way it was, and it may be the wrong thing to use.


So this PR is stuck assuming on this comment thread currently without a clear path forward. I believe Steve and Wilco are busy so, hi @dbjorge any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the real question is the following:

What we need to determine is if the region rule should ignore both an image with an empty alt='' attribute and if an image is entirely lacking an alt attribute

Copy link
Contributor

@dbjorge dbjorge Apr 30, 2024

Choose a reason for hiding this comment

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

My thoughts on the problem

What we need to determine is if the region rule should ignore both an image with an empty alt='' attribute and if an image is entirely lacking an alt attribute

I think this ought to come down to "is the image in question going to be exposed to ATs or not"; I think the result of that is that region should ignore most cases with empty alt and should not ignore cases with no alt, but note that there are some edge cases where the alt attribute isn't the whole story.

I agree with the other comment chain that it's confusing that the use of isVisibleToScreenReaders doesn't already cover "is the image going to be exposed to ATs or not". Even if we do decide to keep isVisibleToScreenReaders as-is, I think it'd be a good idea for us to at least update the comment on isVisibleToScreenReaders to explain this gotcha.

The difficulty with updating isVisibleToScreenReaders for this case is that many places that use it (including-but-not-limited-to its own recursive implementation and also region-evaluate here) do so with the understanding that if it returns false for a given element, it implies that the element and all its descendants are going to be invisible to screen readers.

However, that's not the case here!

The mechanism by which imgs with alt="" get hidden from ATs is that the implicit aria role for such an element is role="none"/role="presentation" rather than role="img"1,2.

role="none"/role="presentation" semantics are different from isVisibleToScreenReaders' semantics in that they don't hide the element's descendants from the accessibility tree; they only hide the semantics of the element itself. A basic <img> tag with a resolved role of none will be omitted from the accessibility tree because its semantics as an image covers the only information about it that would appear in the tree. But in something like <h1 role="none">content</h1>, for example, the content text still appears in the accessibility tree (just as a plain text node, not treated as a heading).

What I suggest to resolve it

I lean towards updating findRegionlessElms to calculate an isShallowlyHidden condition for candidate elements, something like this:

  const role = getRole(virtualNode, { noPresentational: true });
  const isPresentational = role === null
  // The element itself is not visible to screen readers, but its descendants might be
  const isShallowlyHidden = isPresentational && !hasChildTextNodes(elm)

...and then update the conditional that checks for dom.hasContent(node, true) (L76 of the original) to treat !isShallowlyHidden as an additional requirement to return [virtualNode];.

You could make a reasonable argument that the better fix would be to add presentational-role as an additional any check on the region rule. I think that (+ a change to that check's messages to special case the messaging for the alt="" case) would likely be a better result message in this specific motivating repro case, but I recommend against it because of the way the region rule currently tries to only emit failures for the topmost ancestor in a tree of regionless elements; it'll give a misleading "how to fix" recommendation for region violations on elements with text descendants, where a presentational role wouldn't be enough to hide that element's descendant tree as a whole.


Footnotes

1: This has a few caveats:

  • If the author assigns a role attribute explicitly, it will take precedence over implicit role selection
  • If the element is focusable or has a global aria attribute, its implicit role will be treated as role="img" even with alt=""
    • this also overrules an explicit role="none"/role="presentation" attribute

2: axe-core has a few places where this logic comes up, including the caveats from note 1; see presentational-role-evalutate.js and implicit-html-roles.js's img entry

Copy link
Contributor Author

@gaiety-deque gaiety-deque Apr 30, 2024

Choose a reason for hiding this comment

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

Possibly addressed in 524f0be

I played with adding some manual checks for the caveats of global aria attributes and focusable, but it didn't seem to need any additional code. Writing the tests leads me to believe the caveats are handled as expected.

@dequelabs dequelabs deleted a comment from Hellothuy1 Apr 18, 2024
@gaiety-deque gaiety-deque force-pushed the region-rule-ignores-decorative-image branch from 524f0be to f914665 Compare April 30, 2024 19:52
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Can you add an integration test as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an integration test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a few more tests here about other elements:

  • <canvas role="none"> passes
  • <object aria-label="foo"> fails
  • <svg role="none" aria-label="foo"> fails
  • <div role="none">foo</div> fails
    • <div role="none"></div> passes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the other tests as both unit and integration tests

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Almost. Just accept the suggestion and we're done.

test/integration/full/region/region-pass.html Outdated Show resolved Hide resolved
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
@gaiety-deque gaiety-deque merged commit f8c918d into develop May 3, 2024
21 checks passed
@gaiety-deque gaiety-deque deleted the region-rule-ignores-decorative-image branch May 3, 2024 15:23
WilcoFiers added a commit that referenced this pull request May 6, 2024
###
[4.9.1](v4.9.0...v4.9.1)
(2024-05-06)

### Bug Fixes

- Prevent errors when loading axe in a page with prototype.js
- **aria-allowed-attr:** allow meter role allowed aria-\* attributes on
meter element
([#4435](#4435))
([7ac6392](7ac6392))
- **aria-allowed-role:** add gridcell, separator, slider and treeitem to
allowed roles of button element
([#4398](#4398))
([4788bf8](4788bf8))
- **aria-roles:** correct abstract roles (types) for
aria-roles([#4421](#4421))
- **aria-valid-attr-value:** aria-controls & aria-haspopup incomplete
([#4418](#4418))
- fix building axe-core translation files with region locales
([#4396](#4396))
([5c318f3](5c318f3)),
closes [#4388](#4388)
- **invalidrole:** allow upper and mixed case role names
([#4358](#4358))
([105016c](105016c)),
closes [#2695](#2695)
- **isVisibleOnScreen:** account for position: absolute elements inside
overflow container
([#4405](#4405))
([2940f6e](2940f6e)),
closes [#4016](#4016)
- **label-content-name-mismatch:** better dismiss and wysiwyg symbolic
text characters
([#4402](#4402))
- **region:** Decorative images ignored by region rule
([#4412](#4412))
- **target-size:** ignore descendant elements in shadow dom
([#4410](#4410))
([6091367](6091367))
- **target-size:** pass for element that has nearby elements that are
obscured ([#4422](#4422))
([3a90bb7](3a90bb7)),
closes [#4387](#4387)


This PR was opened by a robot 🤖 🎉 (And updated by @WilcoFiers
)
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.

Decorative images should be ignored by the region rule
6 participants