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

new rule: Image not in the accessibility tree is decorative #1213

Merged
merged 20 commits into from
May 14, 2020

Conversation

WilcoFiers
Copy link
Member

@WilcoFiers WilcoFiers commented Mar 6, 2020

This rule checks that visible img, svg and canvas elements that are ignored by assistive technologies are decorative.

Need for Final Call: 2 weeks

Copy link
Member

@carlosapaduarte carlosapaduarte left a comment

Choose a reason for hiding this comment

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

Nice work. Got some questions and comments for you.

_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
Comment on lines 35 to 36
- **ignored svg**: the element is an `svg` with an empty (`""`) [accessible name][] and a [semantic role][] of `graphics-document`; or
- **ignored canvas**: the element is a `canvas` with an empty (`""`) [accessible name][] and no [explicit semantic role][]; or
Copy link
Member

@carlosapaduarte carlosapaduarte Mar 11, 2020

Choose a reason for hiding this comment

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

I understand the assumption, but I can't really understand why should these be inapplicable. If they are not in the accessibility tree, shouldn't they be decorative?

Btw, I tested an svg with VoiceOver on Firefox and it was not ignored. It was ignored with VoiceOver on Chrome.

Copy link
Member Author

Choose a reason for hiding this comment

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

These SVG and canvas exceptions are for elements that are included in the accessibility tree, but that will still be ignored by assistive technologies. So, despite being in the accessibility tree, because AT ignores them, we will test that they are actually decorative.

Copy link
Member

Choose a reason for hiding this comment

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

It seems I understood the applicability differently from what you meant.
This is how I understood it: "All visible img, canvas, svg that are not in the accessibility tree. From that set, remove unloaded img, ignored svg, ignored canvas and named from author"
This is what you meant (from your reply): "All visible img, canvas, svg that are not in the accessibility tree. To that set, add unloaded img, ignored svg, ignored canvas and named from author"

If I am correct, we should update the applicability (since it is ambiguous).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree there is something funky with the applicability

I read it the same way as @carlosapaduarte, but @WilcoFiers's answer "[these] elements that are included in the accessibility tree but (…) ignored by AT" makes little sense with that interpretation: if these elements are included in the accessibility tree, then they are already inapplicable and don't need to be specifically excluded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Jup, rereading this, I see your point. Fixing it.

_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
Jym77
Jym77 previously requested changes Mar 12, 2020
Copy link
Collaborator

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

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

Some more comments on top of what @carlosapaduarte already said.


#### Inapplicable Example 4

This `svg` is ignored because it is a child of a link that has provides its [accessible name][].
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would help to say this is the named from author exception (given that the items are named, better use these names…)

_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved

#### Passed Example 4

This decorative `svg` element is ignored by assistive technologies because it has no attribute that would give it an [accessible name][].
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this rule is not to check whether element have accessible name or not? We should rephrase this description to meet the expectation and also for similar examples. Something like that:
"This svg element is not included in the accessibility tree and is a decorative because it do not contain an attribute which provide alternative text to this element."


#### Passed Example 5

This decorative `canvas` element is ignored by assistive technologies because it has no attribute that would give it an [accessible name][].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider same comment from Passed Example 4

top: -9999em;
}
</style>
<img src="/test-assets/shared/w3c-logo.png" alt="" />
Copy link
Collaborator

@adilsofficial adilsofficial Mar 12, 2020

Choose a reason for hiding this comment

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

If the image is non-decorative it should have alt attribute with text as a best practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair. I'll change the content of the image so it's actually a decorative image.


#### Inapplicable Example 4

This `svg` is ignored because it is a child of a link that has provides its [accessible name][].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This `svg` is ignored because it is a child of a link that has provides its [accessible name][].
This `svg` element is ignored because this element is a descendant of a link that provides its [accessible name][].

Copy link
Collaborator

@adilsofficial adilsofficial left a comment

Choose a reason for hiding this comment

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

Good work 👍 .

  • Examples description should be rephrase and/or improve to make them consistent with each other.
  • Add examples for exceptions.

WilcoFiers and others added 2 commits March 17, 2020 13:26
Co-Authored-By: carlosapaduarte <carlosapaduarte@gmail.com>
Co-Authored-By: Adil Hussain <42895421+adilsofficial@users.noreply.github.com>

#### Failed Example 5

This `canvas` element which has no [semantic role][] and an empty (`""`) [accessible name][] is not [purely decorative][].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean "[semantic role][] of graphics-document" here? if yes update this description and add role="graphics-document" in the code below as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, canvas doesn't have an implicit semantic role.


#### Inapplicable Example 9

This is a `div` element with a background image.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more description for this example as it does not make sense that a div element with a background image so what exactly makes this example inapplicable? Maybe with a note that this rule does not apply to background images.

WilcoFiers and others added 3 commits April 16, 2020 14:31
Co-Authored-By: Jean-Yves Moyen <jym@siteimprove.com>
Co-Authored-By: Jean-Yves Moyen <jym@siteimprove.com>
@WilcoFiers WilcoFiers dismissed stale reviews from adilsofficial and Jym77 April 17, 2020 10:31

updated

Copy link
Collaborator

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

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

This looks good.
Some minor (and optional) editorial comments.


**Exception**: Exclude any `img` element where the [current request][]'s [state][image request state] is not [completely available][]; or

**Note**: An example of an image ignored because of "named from author" is when the image is a descendant of a `button` element that uses `aria-label` for its accessible name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keeping the same typography (bold) than in the initial list to make it obvious that it is the same thing.
(i.e., consistency in typography).

(I won't insist if you feel it's not needed)

Comment on lines 41 to 42
- The element has an [ancestor][] in the [flat tree][] of an element that is [named from author][]; or
- The element is an `img` element where the [current request][]'s [state][image request state] is not [completely available][].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also name these items?

Suggested change
- The element has an [ancestor][] in the [flat tree][] of an element that is [named from author][]; or
- The element is an `img` element where the [current request][]'s [state][image request state] is not [completely available][].
- **named from author** The element has an [ancestor][] in the [flat tree][] of an element that is [named from author][]; or
- **unavailable** The element is an `img` element where the [current request][]'s [state][image request state] is not [completely available][].

_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
Copy link
Member

@carlosapaduarte carlosapaduarte left a comment

Choose a reason for hiding this comment

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

Just a single edit.
Plus there's a couple of suggestions from @Jym77 in here that you might want to commit (or not - I'll accept the PR without those).

_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@daniel-montalvo daniel-montalvo left a comment

Choose a reason for hiding this comment

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

Good work!

Please note this minor typo, but otherwise happy to approve.

_rules/image-not-in-acc-tree-is-decorative-e88epe.md Outdated Show resolved Hide resolved
Co-Authored-By: daniel-montalvo <49305434+daniel-montalvo@users.noreply.github.com>
Co-Authored-By: Jean-Yves Moyen <jym@siteimprove.com>
Co-Authored-By: carlosapaduarte <carlosapaduarte@gmail.com>
@WilcoFiers WilcoFiers dismissed carlosapaduarte’s stale review April 29, 2020 09:09

Editorial changes made

@WilcoFiers WilcoFiers added Review call 2 weeks Call for review for new rules and big changes and removed reviewers wanted labels Apr 29, 2020
Jym77
Jym77 previously requested changes Apr 29, 2020
Comment on lines 41 to 44
- The element has an [ancestor][] in the [flat tree][] that is [named from author][]; or
- The element is an `img` element where the [current request][]'s [state][image request state] is not [completely available][].

**Note**: An example of an image ignored because of "named from author" is when the image is a descendant of a `button` element that uses `aria-label` for its accessible name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If items in the list above are not named, you can't refer to"named from author" like that. Especially given that the meaning here is not the meaning in ARIA (here, "named from author" means that one ancestor is "named from author in the ARIA meaning").

@WilcoFiers WilcoFiers merged commit a42a90e into develop May 14, 2020
@WilcoFiers WilcoFiers deleted the image-decorative branch May 14, 2020 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review call 2 weeks Call for review for new rules and big changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants