Skip to content

Conversation

annethyme
Copy link
Collaborator

@annethyme annethyme commented Mar 19, 2019

@rvantonisse wrote in #424:

During several audits with this rule, “Heading is Descriptive”, we encountered some situations in which we were forced by the rule to fail the SC.

The stituations were:

  • Empty heading with a section of the content.
  • Heading without section of the content
  • Heading that describes only a part of the section of the content.

We believe these situations should not fail the WCAG SC 2.4.6 and therefore the rule needs some improvements.

Closes issue: #424

Also updating the rule to:

  • in the Applicability use the replacements terms for the "non-empty" definition introduced in this pull request: https://github.com/auto-wcag/auto-wcag/pull/430/files
  • in the Expectation evaluate both the accessible name and text nodes of the heading, if the two are different. This was not clear from the rule beforehand.

How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.

@annethyme annethyme self-assigned this Mar 19, 2019
@annethyme annethyme added March 31 deadline Rule Use this label for a new rule that does not exist already Rule Update Use this label for an existing rule that is being updated labels Mar 19, 2019
Copy link
Collaborator

@ShadowBB ShadowBB left a comment

Choose a reason for hiding this comment

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

Hey Anne,

I am unsure why we should evaluate both the Accessible name and the Text Nodes. If they are different, should not only the Accessible name be evaluated (seeing as it replaces the text nodes?)

@annethyme
Copy link
Collaborator Author

I have misplaced my test asset, "oranges.png". I don't know how to get it into a folder... Can @JKODU help?

@annethyme annethyme requested review from Brynanders and jeeyyy March 20, 2019 08:38
@annethyme
Copy link
Collaborator Author

@ShadowBB,
As I see it, this success criterion is for sighted users as well as screen reader users, so both user groups deserve a descriptive heading.

@annethyme annethyme requested a review from ShadowBB March 20, 2019 08:39
### Expectation 1

Each target element describes the topic or purpose of its [section of the content](#section-of-content).
The [visible](#visible) [content](https://www.w3.org/TR/WCAG21/#dfn-content) of each target element describes the topic or purpose of the entirety or a part of its [section of the content](#section-of-content), or the `heading` has no section of the content.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The visible content of each target element describes the topic or purpose of the entirety or a part of its section of the content, or the heading has no section of the content.

change to...

The visible content of each target element describes the topic or purpose of its section of the content in part or in it's entirety, or the heading has no section of the content.


### Expectation 2

The [accessible name](#accessible-name) of each target element describes the topic or purpose of the entirety or a part of its [section of the content](#section-of-content), or the `heading` has no section of the content.
Copy link
Collaborator

Choose a reason for hiding this comment

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

See previous comment, this doesn't sound right... "...describes the topic or purpose of the entirety or a part of its section of the content."


## Assumptions
_There are currently no assumptions._
This rule assumes that while having a heading that has no section of content might be a WCAG violation under other success criteria (e.g. 1.3.1 Info and Relationships, if the heading markup has been misused for purely presentational purposes), this is allowed under success criterion 2.4.6 Headings and Labels: Headings and labels describe topic or purpose.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Heading marked up with `h` element that describes the topic or purpose of a part of its section of the content.

```html
<h1 class="target">Oranges</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the target class for? It seems redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No clue, it comes from the test case I copied+modified. Many of the test cases have class="target" on them. I will remove it.


#### Passed example 10

Visible heading is made up by an image of text, that also has an accessible name. Both visible content and accessible name is descriptive.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The visible heading is made up of an image of text, that also has an accessible name. Both the visible content and the accessible name describe the content.

#### Failed example 5
Empty heading marked up with `h` element.
Heading marked up with `h` element where the visible text nodes describe the content, but the accessible name doesn't.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I should be flagging this stuff but it seems like you are using a kind of short form of English
Heading marked up with h element...
as opposed to
A heading is marked up with an h element...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was there in the rule when I started editing it. I'll change it if the native English speaker says so :)

@annethyme annethyme dismissed Brynanders’s stale review March 21, 2019 09:25

Thanks for all the great feedback! I have worked in the changes now.

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.

Thanks @annethyme for the great work done with this rule. I'm not really sure I should be approving this one since I'm one of the authors, although it has changed a lot since the initial input. Please dismiss my review if I'm not supposed to review.

On a final note, I agree with @Brynanders suggestion for shortening that note. His proposal is much easier to comprehend. I haven't gone through the newest ACT rules format, but if there is a place to justify why something that passed needs further testing this would be the note to include there.

DagfinnRomen
DagfinnRomen previously approved these changes Mar 25, 2019
Copy link
Collaborator

@DagfinnRomen DagfinnRomen left a comment

Choose a reason for hiding this comment

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

@annethyme - Good work on updating this rule!

I am also listed as one of the authors, but will provide my review. Please dismiss if I am not supposed to review/approve.

ShadowBB
ShadowBB previously approved these changes Mar 26, 2019
Copy link
Collaborator

@ShadowBB ShadowBB left a comment

Choose a reason for hiding this comment

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

Awesome work! I cannot find anything I would change.

Maybe we want to add notes to the passed examples that would fail WCAG (just not this rule) so people don't use them as examples for their websites.

@annethyme annethyme changed the title Rule update: SC2-4-6-descriptive-headings [WIP] Rule update: SC2-4-6-descriptive-headings Jul 18, 2019
@annethyme annethyme changed the title [WIP] Rule update: SC2-4-6-descriptive-headings [WIP] Rule update: Heading is descriptive (b49b2e) Jul 24, 2019
@annethyme
Copy link
Collaborator Author

annethyme commented Aug 6, 2019

Suggestion for splitting up this rule in two:

Suggested rule 1: Visible heading is descriptive

Applicability

This rule applies to any visible element with the semantic role of heading.

Expectation

The visible heading of each target element describes the topic or purpose of its visible section of the content in part or in its entirety, or the heading has no visible section of the content.

Suggested rule 2: Accessible name of heading is descriptive

Applicability

This rule applies to any element with the semantic role of heading that is included in the accessibility tree, has an accessible name that is not only whitespace, and where the:

Expectation

The accessible name of each target element describes the topic or purpose of its section of the content that is included in the accessibility tree in part or its entirety, or the heading has no section of the content that is included in the accessibility tree.

@annethyme annethyme removed the On hold label Aug 7, 2019
@annethyme
Copy link
Collaborator Author

@kasperisager is on board with the suggested split up, so @annethyme will split up the rule to get it ready for review soon again.

@CLAassistant
Copy link

CLAassistant commented Aug 13, 2019

CLA assistant check
All committers have signed the CLA.

- [ARIA12: Using role=heading to identify headings](https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA12)
- [HTML 5.2 Standard - Heading content](https://www.w3.org/TR/html52/dom.html#heading-content)

## Test Cases - UPDATE IN PROGRESS
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@annethyme, make sure to update this

## Applicability

This rule applies to any element with the [semantic role](#semantic-role) of `heading` that is [included in the accessibility tree](#included-in-the-accessibility-tree), has an [accessible name](#accessible-name) that is not only [whitespace](#whitespace), and where:
- the [accessible name](#accessible-name) is different than the [visible](#visible) heading, OR
Copy link
Collaborator Author

@annethyme annethyme Aug 15, 2019

Choose a reason for hiding this comment

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

Tentative suggestion from Kasper and Anne:

## Applicability

This rule applies to any element with the [semantic role](#semantic-role) of `heading` that is [included in the accessibility tree](#included-in-the-accessibility-tree), has an [accessible name](#accessible-name) that is not only [whitespace](#whitespace), and where:
- the [accessible name](#accessible-name) does not [match](#matching-characters) the [visible](#visible) heading that is the combined [characters](#character) formed by the [visible](#visible) [descendant text content](#) and [characters](#character) in [images of text](https://www.w3.org/TR/WCAG21/#dfn-images-of-text), OR
Copy link
Collaborator Author

@annethyme annethyme Aug 16, 2019

Choose a reason for hiding this comment

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

flat tree text content: the concatenation of the data of all text node descendants of node in the flat tree, in tree order.

[visible] images of text that are descendants of the test target in the flat tree

This rule applies to any [visible](#visible) element with the [semantic role](#semantic-role) of `heading`.

This rule applies to any element with the [semantic role](#semantic-role) of heading that is either [visible](#visible) or [included in the accessibility tree](#included-in-the-accessibility-tree).
**Note:**** Headings with only [whitespace](#whitespace) are not [visible](#visible).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remove **

The [visible](#visible) heading of each target element describes the topic or purpose of its [visible](#visible) [section of the content](#section-of-content) in part or in its entirety, or the `heading` has no [visible](#visible) [section of the content](#section-of-content).

Each target element describes the topic or purpose of its [section of the content](#section-of-content).
The [visible](#visible) heading is the combined [characters](#character) formed by the [visible](#visible) [descendant text content](#) and [characters](#character) in [images of text](https://www.w3.org/TR/WCAG21/#dfn-images-of-text).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove link to character and insert note about definition missing + create issue on character definition.

@@ -0,0 +1,290 @@
---
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Split this rule out into separate PR.

@@ -0,0 +1,29 @@
---
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove definition from this PR

@Jym77 Jym77 assigned Jym77 and unassigned annethyme Aug 23, 2019
@Jym77 Jym77 added the On hold label Sep 3, 2019
@Jym77 Jym77 removed the Rule Use this label for a new rule that does not exist already label Sep 26, 2019
@Jym77 Jym77 mentioned this pull request Oct 16, 2019
@Jym77 Jym77 added the Blocked Blocked by another PR/Issue label Oct 18, 2019
@Jym77 Jym77 removed their assignment Nov 11, 2019
@kasperisager kasperisager removed their request for review April 24, 2020 09:52
@WilcoFiers
Copy link
Member

Closing due to inactivity.

@WilcoFiers WilcoFiers closed this May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Blocked Blocked by another PR/Issue Discussion On hold Rule Update Use this label for an existing rule that is being updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.