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

A11y: Wrap ToC Block within nav tag #6084

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

A11y: Wrap ToC Block within nav tag #6084

wants to merge 23 commits into from

Conversation

Tishasoumya-02
Copy link
Contributor

@Tishasoumya-02 Tishasoumya-02 commented Jun 7, 2024

Fixes #6082


📚 Documentation preview 📚: https://volto--6084.org.readthedocs.build/

Copy link

netlify bot commented Jun 7, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit ed61fd1
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66ffd72f48ddc400089f653b

@ichim-david ichim-david self-requested a review June 7, 2024 19:23
Copy link
Sponsor Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@Tishasoumya-02 you know the saying:
The road to hell is paved with good intentions?

This pull request right here brings in mind to me this quote because your
intention is good but the outcome is not.
Many times an aria-label will do worse to have it than not.

Have a look at this video that I will upload, it shows some of the issues of having aria-label on the toc links

toc-entry.mp4

Have a look at this nice toc:
https://www.a11y-collective.com/blog/layout-tables-accessibility/
https://webaim.org/articles/visual/
https://www.w3.org/TR/WCAG20-TECHS/G64.html#G64-description
https://www.w3.org/TR/WCAG21/#label-in-name

if we look at w3 and other popular a11y websites, none of them employ
the technique that you propose.

As long as we ensure that we have proper listing with depth information
plus a good heading to mark table of contents we don't need to mess
with the link information.
A user using the screen reader might use the heading rotary and find
table of contents and from there it finds the list and links.
Have a look at this survey
https://webaim.org/projects/screenreadersurvey10/#finding

@Tishasoumya-02
Copy link
Contributor Author

Thanks! @ichim-david for the in-depth explanation, from the contents you shared , I agree aria-labels can mislead , but if we want the screen reader to read as '....... ToC link' so the users know it is a ToC link , can we use aria but in a more descriptive way eg- ' Table linearisation a Table of Content link' something similiar in this line.

@ichim-david
Copy link
Sponsor Member

@Tishasoumya-02 aria-labelledby gives a better solution that aria-label but even it suffers from replacing the links title from the "Link Rotary" on VoiceOver app.
I tried also aria-describedby and it says the "Table of contents" when you are tabbing around but it doesn't say it when using the Rotary system.
I don't think there is a solution for your problem that is without trade-offs and I have yet to see any accessibility firms or websites provide extra info concerning the table of contents links.
Have a look at these guys that make the axe linter that is used by everyone, even they don't have anything close to what you are proposing:
https://www.deque.com/web-accessibility-beginners-guide/#what-is-a11y

Please investigate some more on this issue and if you find something that is worth investigating do share it.
I also have one website that I want to bring to your attention when you want to use a certain aria role have a look also at the technologies support and try something that has as broad support as possible:
https://a11ysupport.io/tech/aria/aria-describedby_attribute

@Tishasoumya-02
Copy link
Contributor Author

Tishasoumya-02 commented Jun 10, 2024

Sure, Thanks @ichim-david !

@tisto
Copy link
Sponsor Member

tisto commented Jun 10, 2024

@ichim-david I think there is room for two IMO valid opinions here:

  1. One could argue that a screen reader will read "table of contents" (the title of the toc block) following a list of headlines. This will make it clear to the reader that the links are internal anchor links and not links to another page. If you have this view, we should leave things as they are. Maybe slightly improve the HTML structure.

  2. One could also argue that it is essential for a "reader" to differentiate between links to another page and links on the same page (anchors). In that case, you want to be explicit and use an additional aria-label that tells the reader that this particular link is an anchor link (here is an article where someone tries this https://amberwilson.co.uk/blog/are-your-anchor-links-accessible/). If you have this view, I think this PR is exactly what you want (and still minimal compared to other options).

I don't have a strong opinion here. I wasn't able to find any good sources on this topic. However, I am not sure if websites that write about a11y are a "good enough" source for best practices and decisions.

Maybe @polyester or our friends from redturtle (@pnicolli @cekk et al) have an opinion on the topic.

@ichim-david
Copy link
Sponsor Member

@ichim-david I think there is room for two IMO valid opinions here:

1. One could argue that a screen reader will read "table of contents" (the title of the toc block) following a list of headlines. This will make it clear to the reader that the links are internal anchor links and not links to another page. If you have this view, we should leave things as they are. Maybe slightly improve the HTML structure.

I am in favor of this option. You have yet to convince me that option 2 is viable given that even the author who investigated this technique hasn't used this technique for its own table of contents on the page itself:
https://amberwilson.co.uk/blog/are-your-anchor-links-accessible/
My vote is not to have aria-label on the links, aria-labelledby behaves better both when tabbing around and when using the rotary but I still don't like that it shows in the rotary duplication "Table of Content link" instead of the actual link text as such I wouldn't add any aria tags on the links.

All of the a11y blogs that I have read have not employed this technique and although yes, just like blogs about performance can suffer from performance issues, if this was an established pattern I think there would be more information about it and used more broadly.
If there are better techniques discovered then we can test them and see if they have more pros than cons.
Having a discussion and trying things is beneficial so this time isn't wasted but at the same time we should be careful with what aria tags we introduce as the rule is still that no aria rules are better than wrong aria rules.

@polyester
Copy link
Sponsor Member

polyester commented Jun 11, 2024

I would go with option 1 as well for now, but improve the HTML structure. Currently, the ToC block seems to produce a

<div class="table-of-contents default">
<ul> ... </ul>
 </div>

which would already be improved by having the div replaced as a
<nav aria-labelledby="table of contents" class="table-of-contents default">
instead. That's a fairly minimal, gradual approach.

@JeffersonBledsoe
Copy link
Member

JeffersonBledsoe commented Jun 13, 2024

I'd agree with using Option 1 here. Adding the navigation role as suggested above with a descriptive label along with the heading of the ToC provides enough context to the user about what the links will do. You could use an aria-label rather than labelledby on the <nav> to add more context about the ToC, but I wouldn't say this is essential and should be researched on a project-by-project basis. e.g:

<nav aria-label="Contents on this page">
<h2>Contents</h2>
<ul> ... </ul>
</nav>

GOV UK Publishing Components - Contents list is a good example of a well researched and accessible ToC

@tisto
Copy link
Sponsor Member

tisto commented Jun 15, 2024

@Tishasoumya-02 @ichim-david @polyester @JeffersonBledsoe thanks for your valuable input folks! Then let's go with option 1 and improve the HTML structure of the toc block. @sneridagh I guess this is considered to be a breaking change. Can this go into Volto 18? We are still in alpha, right?

@sneridagh
Copy link
Member

@tisto Yeah, no problem by being breaking. We will have to write the breaking notice and the upgrade guide update.

@Tishasoumya-02 could you take care in the next days?

@Tishasoumya-02
Copy link
Contributor Author

Sure @sneridagh

@ichim-david
Copy link
Sponsor Member

@sneridagh @Tishasoumya-02 I don't think we can use any of the changes from this pull request. it would make sense in my opinion to open a new pull request with changes to the Table of contents block.

It's still not clear to me though what is going to be proposed exactly, if we should have a proposal and after feedback it's implemented or we give feedback after whatever you propose to change.

I say this because I've seen the comment from @JeffersonBledsoe #6084 (comment) where he gives the example of how Gov Uk does it and then the simple nav wrap mentioned by @polyester.

I think if we have a proposal and we agree on the markup it would mean fewer changes in the pull request afterward no?
It could be a simple gist or a code paste of a simple toc html output.

@Tishasoumya-02
Copy link
Contributor Author

Tishasoumya-02 commented Jun 18, 2024

Reading through how Gov uk implements the content of table shared by @JeffersonBledsoe , I made some changes in the existing View.jsx of ToC Block (gist of changes below) , should I implement this in this PR for all to test?

<nav className= 'table of contents default' aria-label= 'Table of Contents' >
    <div className= 'table of contents main default' >
    <h2>Content</h2>
     <ul> <li><li> </ul>
    </div>
</nav>

@ichim-david @sneridagh

@davisagli
Copy link
Sponsor Member

@Tishasoumya-02 Is the <div> inside the <nav> really needed?

@Tishasoumya-02
Copy link
Contributor Author

Tishasoumya-02 commented Jun 19, 2024

Not really, we can remove it. As for accessibility it does not serve any purpose

@ichim-david
Copy link
Sponsor Member

ichim-david commented Jun 20, 2024

Reading through how Gov uk implements the content of table shared by @JeffersonBledsoe , I made some changes in the existing View.jsx of ToC Block (gist of changes below) , should I implement this in this PR for all to test?

<nav className= 'table of contents default' aria-label= 'Table of Contents' >
    <div className= 'table of contents main default' >
    <h2>Content</h2>
     <ul> <li><li> </ul>
    </div>
</nav>

@ichim-david @sneridagh

@Tishasoumya-02 I suggest the following:

  1. https://github.com/plone/volto/blob/main/packages/volto/src/components/manage/Blocks/ToC/View.jsx#L168
    Change from div to nav and add aria-label Table of contents translated
  2. Remove data.title condition from here https://github.com/plone/volto/blob/main/packages/volto/src/components/manage/Blocks/ToC/variations/DefaultTocRenderer.jsx#L48
    We want to get the fallback "Table of contents" added in case no title is added. If someone doesn't want a header at all they can use the Hide title and remove that altogether.
    Right now the condition ensures that the fallback will never show up.
    This way we don't have a new nav added and the div that was added from the default View but that div simply changes to be a nav plus aria label.
    The output will be thus:
<nav className= 'table of contents main default' aria-label= 'Table of Contents' >
    <h2>Table of Content</h2>
     <ul> <li><li> </ul>
</nav>

This way the user in need of a screen reader can find the section either from the Landmark rotary which will find "Table of contents navigation" or will use the Headings rotary and know that what follows is a table of contents.

@JeffersonBledsoe
Copy link
Member

One additional thing we also need to consider is the potential for there to be multiple Table of Contents blocks on a page. We don't want every ToC Block to have a nav of Table of Contents.

@ichim-david
Copy link
Sponsor Member

One additional thing we also need to consider is the potential for there to be multiple Table of Contents blocks on a page. We don't want every ToC Block to have a nav of Table of Contents.

@JeffersonBledsoe that's a good point, perhaps the aria-label could contain the title. If you don't enter anything you get the fallback for Table of contents which is with Formatted Message.
If you do enter something it could take that value. I don't know how often we would have a requirement where we have more than one table of contents present though.
If you have another technique in mind I'm all ears :)

@stevepiercy
Copy link
Collaborator

I don't know how often we would have a requirement where we have more than one table of contents present though.

In some documentation projects, it is not an uncommon use case. It allows to break up a wall of text with headings and explanatory text for greater legibility and context. Example of multiple ToCs:

...and so on.

@Tishasoumya-02
Copy link
Contributor Author

One additional thing we also need to consider is the potential for there to be multiple Table of Contents blocks on a page. We don't want every ToC Block to have a nav of Table of Contents.

If we could use a different landmark like section or main instead of nav , would that be an option?

@JeffersonBledsoe
Copy link
Member

Looks good! cc @ichim-david ready if you're happy

- Also use the Table of contents message from the TOC schema to signal
  what content it is when there are no headings to show in edit.
- Rollback default showing of Table of contents h2 since it would be breaking
  change as opposed to how it works now.
…efaultTocRender ToC templates

- Since the fallback would never resolve to true there is no point in referencing them
  instead we show Table of contents if there are no headings to show
…sages

- I want to avoid getting rid of the i18n translated strings and keep CI happy
@ichim-david ichim-david changed the title A11y: ToC Block links A11y: Wrap ToC Block within nav tag Sep 26, 2024
@ichim-david
Copy link
Sponsor Member

@davisagli @pnicolli I wanted todo some more changes such as making actual use of the data.title fallback which was translated saying "Table of contents" by setting that value as default to the title.
That meant that you add the block and you get that title which you can change.
If you wanted not to display it you had the hide title option.

Upon checking https://light-theme.kitconcept.io/ and seeing that you only have the title option empty by default
I removed that change.
The only front-facing changes are wrapping the table of contents with a nav tag instead of a div.
light-theme-toc

Given the fact that the styling references the class in order to style it, it won't affect your projects.
If a title is added then that title will be used as part of the navigation otherwise it will simply say navigation.
Depending on your needs we could provide the "table of contents" aria-label if no title is added or not so
i'm open to suggestions on what you are ok with.

@davisagli
Copy link
Sponsor Member

@ichim-david I don't have a strong opinion about whether or not we should have a default title (my gut feeling is to leave it as is with no default), or about whether or not we should put "table of contents" in the aria-label when there's no title (my gut feeling is to leave it out, if screen readers will indicate the nav element in some way anyway).

@sneridagh
Copy link
Member

@Tishasoumya-02 @ichim-david Let's wrap things for the final release, and I think this is a good one to be included, so let's move this forward, the change is sound and it already improves things. We just need blessing from someone from the a11y team.

As I see, it's breaking and therefore needs proper news item .breaking and an entry in the upgrade guide.

@polyester
Copy link
Sponsor Member

@ichim-david I don't have a strong opinion about whether or not we should have a default title (my gut feeling is to leave it as is with no default), or about whether or not we should put "table of contents" in the aria-label when there's no title (my gut feeling is to leave it out, if screen readers will indicate the nav element in some way anyway).

I'd agree with @davisagli here; minimal intrusion is better so leave as is with no default.

And agreed with @sneridagh ; it's an improvement, so let's wrap up and get it in.

@davisagli
Copy link
Sponsor Member

@sneridagh @plone/volto-accessibility I updated the upgrade guide. I think it's ready.

packages/volto/news/6082.breaking Outdated Show resolved Hide resolved
@ichim-david
Copy link
Sponsor Member

@Tishasoumya-02 @ichim-david Let's wrap things for the final release, and I think this is a good one to be included, so let's move this forward, the change is sound and it already improves things. We just need blessing from someone from the a11y team.

As I see, it's breaking and therefore needs proper news item .breaking and an entry in the upgrade guide.

@sneridagh this is ready for merging I see the upgrade notice is added and .breaking. To me this is a very soft or not at all breaking given the fact that we modify the wrapper of the table of content block and light-theme hooks on the class and not the html element which we should always do so as html elements can change but classes can remain stable.

@ichim-david
Copy link
Sponsor Member

ichim-david commented Oct 4, 2024

@ichim-david I don't have a strong opinion about whether or not we should have a default title (my gut feeling is to leave it as is with no default), or about whether or not we should put "table of contents" in the aria-label when there's no title (my gut feeling is to leave it out, if screen readers will indicate the nav element in some way anyway).

@davisagli @sneridagh Honestly if we look at the initial issue where @Tishasoumya-02 wanted to add "The link should be marked as Table-of-Contents-Link" I think I shouldn't have asked if we want to have "Table of contents" aria-label to the nav when we have no title and simply add it as this would have achieved the desire to let the user know that the nav is for "Table of contents". If you add a title then that would be the aria-label.

Right now we are doing less by simply saying that it's a nav. Hopefully no one adds the table of contents without a title to make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A11y: ToC Block links are not marked as ToC links
8 participants