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

"Heading is descriptive" (b49b2e): replace "section of content" #1425

Merged
merged 17 commits into from Sep 23, 2020

Conversation

Jym77
Copy link
Collaborator

@Jym77 Jym77 commented Aug 20, 2020

Change the expectation to remove "section of content" and replace it with "the next palpable content" (according to https://www.w3.org/2020/07/23-act-r-minutes.html#item01).

Doing minimal changes to get the fix working. Notably not splitting the rule for visible/accessible headings (it feels much needed now), and not cleaning up the rest of the rule (will be done once we want to submit it to TF review).

Closes issue(s):

Need for Final Call:
This will require a 1 week Final Call (changing expectation)


Pull Request Etiquette

When creating PR:

  • Make sure you're requesting to pull a branch (right side) to the develop branch (left side).

After creating PR:

  • Add yourself (and co-authors) as "Assignees" for PR.
  • Add label to indicate if it's a Rule, Definition or Chore.
  • Link the PR to any issue it solves. This will be done automatically by referencing the issue at the top of this comment in the indicated place.
  • Optionally request feedback from anyone in particular by assigning them as "Reviewers".

When merging a PR:

  • Close any issue that the PR resolves. This will happen automatically upon merging if the PR was correctly linked to the issue, e.g. by referencing the issue at the top of this comment.

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”.
  • Make sure to also review the proposed Final Call period. In case of disagreement, the longer period wins.

@Jym77 Jym77 added Rule Update Use this label for an existing rule that is being updated reviewers wanted labels Aug 20, 2020
@Jym77 Jym77 self-assigned this Aug 20, 2020
Comment on lines 36 to 37
- [visible][], if the target is [visible][]; and
- [included in the accessibility tree][], if the target is [included in the accessibility tree][].
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This strongly suggest that this rule should be split in two, one for each case…

Copy link
Collaborator

@EmmaJP EmmaJP Aug 26, 2020

Choose a reason for hiding this comment

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

Not if those 'and' conditional are intentional. I read this as:

If the target is visible and included in the accessibility tree, then it should describe the first palpable content that is visible and included in the accessibility tree that comes after the target in the flat tree.

However, what about non-visible headings describing visible content, where both are included in the accessibility tree? This is not an unusual use case, eg. a news front page with a hidden heading 'Top headlines'. The heading isn't needed visually as that is usually apparent from other cues.

That also makes me question the phrasing of the applicability. Should it not be ' visible and/or included in the accessibility tree'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it is both visible and included in the accessibility tree, then it is "visible or included in the accessibility tree" (the or is inclusive by default). And then it matches both the expectations and need to describe both visible and accessible content (which may be the same or different palpable content).

kasperisager
kasperisager previously approved these changes Aug 20, 2020
Copy link
Contributor

@kasperisager kasperisager left a comment

Choose a reason for hiding this comment

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

I like it! 👏

@Jym77 Jym77 marked this pull request as ready for review August 24, 2020 11:38
@Jym77 Jym77 dismissed kasperisager’s stale review August 24, 2020 11:39

Rest of the changes done

@Jym77 Jym77 changed the title [WiP] "Heading is descriptive" (b49b2e): replace "section of content" "Heading is descriptive" (b49b2e): replace "section of content" Aug 24, 2020

**Note:** Headings do not need to be lengthy. A word, or even a single character, may suffice.

## Assumptions

This rule assumes that the language of each test target can be correctly determined (either programmatically or by analyzing the content), and sufficiently understood.
- This rule assumes that the language of each test target can be correctly determined (either programmatically or by analyzing the content), and sufficiently understood.
- This rule assumes that the [flat tree][] order is close to the reading order, as elements are rendered on the page. Due to positioning, it is possible to render a document in a order that greatly differ from the tree order, in which case the content which is visually associated with a heading might not be the content following it in tree order and this rule might fail while [Success Criterion 2.4.6 Headings and Label][sc246] is still satisfied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the rule essentially assuming conformance to 1.3.2 (https://www.w3.org/TR/WCAG/#meaningful-sequence)? In that case, it seems like an even more reasonable assumption to make.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really, because the programmatically determined order could be different from the DOM order (and match the visual order). For example by tweaking the accessibility tree with aria-owns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍 I also just looked into CSS Grid and, interestingly enough, grid layouts that re-arrange the logical order of content are non-conforming: https://drafts.csswg.org/css-grid/#order-accessibility

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not to mention the fun that can ensue when flexbox gets involved!

_rules/heading-descriptive-b49b2e.md Outdated Show resolved Hide resolved
EmmaJP
EmmaJP previously requested changes Aug 26, 2020
Copy link
Collaborator

@EmmaJP EmmaJP left a comment

Choose a reason for hiding this comment

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

I think some fine tuning is still needed with the phrasing of the applicability and expectation sections. Other than that, I think this is really starting to shape up well.

@Jym77 Jym77 dismissed stale reviews from WilcoFiers and EmmaJP September 3, 2020 12:33

Split expectation in two

Jym77 and others added 4 commits September 16, 2020 16:05
Co-authored-by: Aron Janecki <aronjanecki@gmail.com>
Co-authored-by: Aron Janecki <aronjanecki@gmail.com>
@Jym77
Copy link
Collaborator Author

Jym77 commented Sep 16, 2020

Final Call ends on September 23rd.

@Jym77 Jym77 added Review Call 1 week Call for review for small changes and removed reviewers wanted labels Sep 16, 2020
@Jym77
Copy link
Collaborator Author

Jym77 commented Sep 23, 2020

Final Call has ended. Merging.

@Jym77 Jym77 merged commit 453ef16 into develop Sep 23, 2020
@Jym77 Jym77 deleted the heading-descriptive-fix branch September 23, 2020 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Call 1 week Call for review for small changes 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.

"Heading is descriptive" (b49b2e) What do heading need to describe?
5 participants