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

Move table of contents sidebar to right of body content, fix title, and change role to navigation #24

Closed
7 of 8 tasks
s3ththompson opened this issue Apr 7, 2022 · 27 comments

Comments

@s3ththompson
Copy link
Member

s3ththompson commented Apr 7, 2022

There is currently a table of contents sidebar on the Pattern, Fundamental, Example, and Index pages.

The sidebar should be moved to the right side of the content (maintaining its current location in the DOM) to match the redesigned supplemental material template: example here, source here.

For the Example page, the miscellaneous links under the table of contents (in yellow box) should be removed. "Report Issue" and "Related Issues" are superseded by the "Help us improve this page" section. The "Browser and Assistive Technology Support" link is no longer necessary.

To-do list for this issue

The following to-do items are all described in more detail in comments below.

  • Remove "Report Issue" and "Related Issues" links from yellow box in page contents section. They will be added to the the new page footer as part of Create page footers for example pages #32 .
  • Remove the related links link from the yellow box in the page contents section; it will be added to footer as part of issue Create page footers for example pages #32.
  • Remove the "Browser and Assistive Technology Support" link from the yellow box in page contents section; it is not necessary because it is included in the disclosure that follows the H1.
  • Change DOM location of page contents to be before and outside main
  • Set role of page contents region to "navigation" (this should be done in template)
  • Set name of navigation region to "Page Contents" (e.g., aria-label="Page Contents")
  • Change title from "Table of Contents" to "Page Contents"
  • Ensure title remains H2 (best to enforce in template)

Edited: "Related Issues" should be moved to footer instead of "Help us improve this page", see new issue linked above

@s3ththompson
Copy link
Member Author

s3ththompson commented Apr 7, 2022

cc @mcking65 @a11ydoer did we decided what would happen to the parent "Design Pattern" link on the Example page? If we remove the miscellaneous links from the sidebar we need to either find a new home for it or remove it.

The parent design pattern is already linked to from the body content on the Example page, so that might be sufficient.

@shawna-slh
Copy link
Contributor

RE: parent design pattern:

Would users find it helpful to have the parent Design Pattern name/title and link fairly prominent?

just fyi, for cognitive supplemental guidance and for Understanding WCAG, we have the parent in the nav pager, e.g, "Objective: Help Users Focus":

Since APG won't have the pager, we could put the parent Design Pattern in a box at the beginning of the content, similar to what we're doing with Techniques, Understanding, and Rules — e.g., "Rules Mapping":

@mcking65 mcking65 changed the title Move table of contents sidebar to right of body content Move table of contents sidebar to right of body content, fix label, and change region type May 2, 2022
@mcking65
Copy link
Contributor

mcking65 commented May 2, 2022

@shawna-slh wrote:

Since APG won't have the pager, we could put the parent Design Pattern in a box at the beginning of the content, similar to what we're doing with Techniques, Understanding, and Rules — e.g., "Rules Mapping":

Since we consistently have the design pattern link in the first sentence of the main content, this could end up being pretty noisy. We might want to consider another approach later, e.g., a breadcrumb nav.

For now, let's just drop the extra design pattern link.

Shawn, BTW, the "rules mapping" is inside of a complementary region inside of main. Complementary should never be inside of main. In general, it is best to avoid putting any regions inside of main. Occasional exceptions could be a search or a nav that is related to paging inside of main.

@mcking65
Copy link
Contributor

mcking65 commented May 2, 2022

Change region role from complementary to navigation

Currently, the page table of contents links are inside of a region with role complementary. The role should be navigation, not complementary. I didn't inspect, but I am assuming this might be because an aside element was used and the role was not explicity set.

@mcking65
Copy link
Contributor

mcking65 commented May 2, 2022

Name the page contents navigation region

Currently, the rregion containing the table of contents is not named. In addition to changing the role to navigation as mentioned above, the name should be "Page Contents". Then, it will be announced as "Page Contents Navigation Region" or equivalent by most screen readers.

On this test rules page, the words "Page Contents" appear at the start of the region. However, on this APG page, the words "Table of Contents" appear.

APG should be made consistent with the template and use the words "Page Contents".

On the APG page, the "Page Contents" title is a H2, which is correct. On the rules page, it is not even marked up as a heading; that is a defect in the template or the rules page; the title "Page Contents" should be an H2.

Net, the markup should be:

<nav aria-labelledby="toc_title">
  <h2 id="toc_title">Page Contents</h2>
  <ul>
    <li>link 1</li>
    ...
  </ul>
</nav>

@mcking65
Copy link
Contributor

mcking65 commented May 2, 2022

Fix DOM location of page contents

On this test rules page, the page contents region is correctly loacated in the DOM just prior to and outside of the main content region.

However, on this APG page, the page contents is located inside of main.

Correct the reading order for the page contents by relocating it in the DOM so it is outside and just before main to be consistent with the template.

@mcking65 mcking65 changed the title Move table of contents sidebar to right of body content, fix label, and change region type Move table of contents sidebar to right of body content, fix title, and change role to navigation May 2, 2022
@richnoah
Copy link

richnoah commented May 2, 2022

@mcking65 you have added a deliverable to remove extra 4 links from bottom of page contents but I am not clear on what those 4 links are and in order to be able to QA the work appropriately would you please list them.

@mcking65
Copy link
Contributor

mcking65 commented May 3, 2022

@richnoah I updated the to-do list to be explicit about each of the 4 links.

@richnoah
Copy link

richnoah commented May 6, 2022

@shawna-slh there are two items on this issue that we need some assistance with in regards to the template:

  1. Setting the role of the "Page Contents" section to
  2. Documentation on how to set the "Page Contents" section outside of

cc: @mcking65 @s3ththompson

@SteveALee
Copy link
Contributor

@richnoah cc: @mcking65 @s3ththompson

Let me try to clarify the situation and your request

  1. it looks like the side page contents box is an aside when it should be navigation - great spot - that's an historical artefact
  2. we don't use complementary and as Shawn pointed out elsewhere aside may be preferred. Is there still a nesting issue. If so what exactly needs to be done.

@SteveALee
Copy link
Contributor

To enable the automatic side bar contents add the following to pages (using page specific frontmatter or global config.yml as usual)

sidebar: true

That's it.

It will:

  • add an id attribute to all H2 elements based on the text
  • populate the Document Contents box with links to the H2s

@shawna-slh
Copy link
Contributor

it looks like the side page contents box is an aside when it should be navigation - great spot - that's an historical artifact

Looks like there are a few issues with the Page Contents sidebar. I put some in a new site-wide issue on structure and semantics

I am assuming it is best for APG to use the template sidebar: true as is, and not do any overrides. Then, lets see if we can get the site-wide issues addressed ASAP.

We certainly welcome Pull Requests from Bocoup and others to get it done sooner.

@mcking65
Copy link
Contributor

mcking65 commented May 8, 2022

@SteveALee asked:

  1. we don't use complementary and as Shawn pointed out elsewhere aside may be preferred. Is there still a nesting issue. If so what exactly needs to be done.

There is not a nesting problem specifically related to this issue. I have observed unnamed "complementary" regions inside of main content in several places across WAI site. I will raise some separate issues related to that. Perhaps the aside element is being used inside of main and is getting mapped to complementary. If that is the case, that might be a browser bug. I'll have to look into the UA requirements and mappings related to that. If you are using aside inside of main, one way to work around such a bug would be to put role="none" on the aside.

@mcking65
Copy link
Contributor

@SteveALee I might have misunderstood your question about nesting.

On the other WAI minimal template pages, there is not a nesting issue for this region.

However, in APG, we have a problem that the page contents navigation region is showing up inside of main instead of just before it.

We also have the issue that it does not have an aria-label. it should have aria-label="Page Contents". Alternatively, it could have aria-labelledby referring to the H2 that is at the start of the nav element; that h2 has content of "Page Contents" so refering to it with aria-labelledby would yield the same name as using aria-label="Page Contents".

@SteveALee
Copy link
Contributor

SteveALee commented May 10, 2022

You did understand my question, thanks. I'll investigate why it would be different for APG. I suspect its due to using the tabbed navigation.

We definitely want to get ARIA usage correct for this content! :)

I had mistakenly thought the labelled-by would not be needed. Given the first rule of ARIA. After all, it's never added to the main or body to point to the H1. Or should it be? On reflexion being explicit does no harm even if markup diverges for AT. Thanks for the correction.

@mcking65 mcking65 mentioned this issue May 10, 2022
@richnoah
Copy link

@SteveALee when sidebar: true was implemented the attribute was not added to the H2 elements and thus the content was not added to the sidebar. Please advise.

@SteveALee
Copy link
Contributor

However, in APG, we have a problem that the page contents navigation region is showing up inside of main instead of just before it.

I'm confused as the sidebar is not showing what area do you mean?

when sidebar: true was implemented the attribute was not added to the H2 elements and thus the content was not added to the sidebar. Please advise.

Ah, my bad, it also needs a symblink to a plugin. I can do that for you I do not see the sidebar:true in any commit so I will create a new PR for you

@SteveALee
Copy link
Contributor

@richnoah see #72

@richnoah
Copy link

@SteveALee yes, thank you.

@mcking65
Copy link
Contributor

@SteveALee wrote:

I had mistakenly thought the labelled-by would not be needed. Given the first rule of ARIA. After all, it's never added to the main or body to point to the H1. Or should it be?

Generally, main is not named because there should never be more than one main element, the purpose of main is self-evident, and there are so many other indicators of the name of a page.

Navigation regions are fundamentally different from main even if there is only one navigation region. A name on a navigation region makes the purpose, e.g., what the user is navigating, much easier to understand.

One exception is that I often recommend naming the main region in single-page apps where the main content is a reflection of a navigation experience in one section of the page and the new content load is less evident to screen reader users. In those cases, multiple ways of ensuring the user understand which content is displayed can help the user avoid mistakes. For example, on messenger.com, we name the main "Conversation with CHAT_TITLE", e.g. "Conversation with Steve Lee". This gives one more signal of which conversation is loaded, providing one more layer of assurance that you are in the chat you think you are in.

The body element should not be named.

@mcking65
Copy link
Contributor

I have reviewed the preview of #62, and all but 2 of the items are resolved in the preview:

  1. Providing accessible name of the navigation region -- "Page Contents"
  2. Positioning it in the DOM before the main element.

@SteveALee
Copy link
Contributor

@mcking65 these both require #72 to be merged into the launch-fixes branch and all old Contents to be deleted.
It looks like @alflennik was having problems and I'll address them now. I also now see some clashes so will merge in your branch too.

@richnoah
Copy link

The aria-lablel-by is specified in the template. All other criteria for this task have been met. Moving to In Review as the template will have to address the lablel.

@mcking65
Copy link
Contributor

Verified fixed in #62 except for the accessible name as mentioned by Rich.

@SteveALee
Copy link
Contributor

@mcking65 said:

Verified fixed in #62 except for the accessible name as mentioned by Rich.

Well Rich says "The aria-lablel-by is specified in the template" which I assume means is fixed and indeed the mark up I see in the launch branch is as expected

<nav class="..." arial-label-by="sidebar-header">
          <h2 id="sidebar-header" class="box-h ">Page Contents</h2>

Am I missing something?

@SteveALee
Copy link
Contributor

In email @richnoah said

aria-label-by is governed by the template. We are not able to set the label to “Page Contents” as requested in the issue. The current aria-label-by reads “sidebar-header”

Now fixed - was a type-o in the aria-labelledby attrib referencing the h2 element

@richnoah
Copy link

@SteveALee Alex had created a PR in wai-website-data to help with the issue of H2 ids being overwritten. Would you please review and merge if looks good to you? w3c/wai-website-data#87

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

No branches or pull requests

5 participants