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

Remove Duplicate Level One Heading #376

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove Duplicate Level One Heading #376

wants to merge 3 commits into from

Conversation

jkva
Copy link
Contributor

@jkva jkva commented Dec 8, 2021

This PR addresses the following item from PAC's "ARIA-AT App Screen Reader Accessibility Observations" document:

There are two level 1 headings for each testing task, making it potentially unclear where the main content of interest begins. There should only ever be one level 1 heading on a page.


Question: since the h1 and h2 on this page are now quite similar:

  • h1 "Testing task: 1. Open a Modal Dialog"
  • h2 (previously h1) "Testing task: Open a Modal Dialog"

Could the h2 not be done without? That way this PR can be simplified by just removing the duplicate h1 altogether and thus keeping the rest of the heading structure intact. This would then also ensure no changes need to be made to #375.

The main instructional and data entry content for the task is still in a separate section, and it looks like the content that would require a heading either way. But generally this looks like the overall page structure could be improved here.


Effective changes:

  • Move second h1 (Testing task title) to h2; and
  • any h2 to h3, h3 to h4.

@jkva jkva added the question Further information is requested label Dec 8, 2021
@jkva jkva requested a review from jscholes December 8, 2021 13:32
Copy link
Contributor

@jscholes jscholes left a comment

Choose a reason for hiding this comment

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

@jkva I agree with your assertions here, but I think restructuring the headings completely is a larger task. E.g. I'd personally have the test plan name, AT/browser info and number of completed tests earlier on the page, maybe before the test navigator or in that section? Not sure. But either way it would also require some design changes.

For now, does this also address the formatting of the remaining h1, whereby the text is missing whitespace after the colon for screen reader users?

@jkva
Copy link
Contributor Author

jkva commented Dec 9, 2021

@jscholes I've added a non breaking space between the "Testing task:" span and the rest of the h1 content. There's otherwise not really anything to do there apart from setting the aria-label on the h1 which seems overkill.

As for the heading restructuring, the PR in its current state already manages that. It just results in two headings (h1 and h2) being quite similar in text content.

@jkva jkva requested a review from jscholes December 9, 2021 12:30
Copy link
Contributor

@jscholes jscholes left a comment

Choose a reason for hiding this comment

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

Approving this, caveat emptor my comments in #379 (review).

@isaacdurazo
Copy link
Member

@jkva I noticed that adding the non-breaking-space creates an alignment issue in the UI, which pushes the "Testing task" name slightly to the right.

Before:
Screen Shot 2022-02-03 at 2 56 59 PM

After:
Screen Shot 2022-02-03 at 2 57 03 PM

Would it be possible and semantically correct to have the non-breaking-space right after the colon but within the <span> element?

@jkva
Copy link
Contributor Author

jkva commented Feb 7, 2022

@isaacdurazo good point, thank you. Addressed.

@isaacdurazo
Copy link
Member

Thanks for addressing, @jkva

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested ready-for-eng-review UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants