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

Fix aria-level in treegrid example 1 #2725

Closed
wants to merge 1 commit into from
Closed

Conversation

tryggvigy
Copy link

@tryggvigy tryggvigy commented Jun 12, 2023

aria-level is defined as follows in the example page https://www.w3.org/WAI/ARIA/apg/patterns/treegrid/examples/treegrid-1/

aria-level="number" tr Defines the level of the row in the hierarchical treegrid structure. Counting is one-based. Root rows have aria-level=“1”.

If I understand the treegrids correctly, aria-level should indicate which level in the tree hierarchy the row is in. In this example, only the first row has aria-level="1" even though there are three top-level rows. This PR fixes this issue

Here is how the treegrid looks.
image


WAI Preview Link (Last built on Tue, 13 Jun 2023 18:37:59 GMT).

@mcking65
Copy link
Contributor

@tryggvigy

What is it about the example that leads you to the conclusion that there are 3 level one rows?

The intent of the example is that there is one "root" in this tree; it is all one thread with the subject "Treegrids are awesome". That root row is level 1. There are 3 direct replies that are at level 2.

Note that if you collapse the first row, there is only one row displayed. That is the single level 1 row, and that is why aria-setsize is set to 1 on that row.

After you expand level 1, there are 3 level 2 rows. On each of those rows, aria-setsize is set to 3.

The second and third rows at level 2 each have children as well. Then, the last level 3 row has some level 4 children.

Apparently there is something confusing about the wording or presentation; it would be helpful if we could identify that and do something to rectify it.

@andreancardona
Copy link
Contributor

andreancardona commented Jun 27, 2023

Here is Carbon React's example as a visual comparison: https://react.carbondesignsystem.com/?path=/story/components-treeview--default

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed PR2725: Fix aria-level in treegrid example 1 by tryggvigy.

The full IRC log of that discussion <jugglinmike> subtopic: PR2725: Fix aria-level in treegrid example 1 by tryggvigy
<jugglinmike> github: https://github.com//pull/2725
<jugglinmike> Matt_King: I believe the person who submitted this is fixing an issue that doesn't exist, and I responded as much in a comment
<jugglinmike> Matt_King: I wonder if there is something about the visual design of the tree grid that led the author to suspect that there are three levels
<jugglinmike> howard-e: the indentation does seem visually ambiguous to me
<jugglinmike> jugglinmike: The first level of children are almost vertically aligned with their parent. The offset for that specific level is minimal
<jugglinmike> Andrea_Cardona: Agreed
<jugglinmike> arigilmore: I think this needs more indention for the second level (that is, the first level of children)
<jugglinmike> jugglinmike: It might also be good to add a second "thread", since the example currently only has a single top-level item, making it difficult to recognize

@tryggvigy
Copy link
Author

tryggvigy commented Jun 28, 2023

@mcking65

You're right I misunderstood the hierarchy visually. I understand this now.

I think what might help would be to keep the top-level item collapsed initially. That would make it clearer that there is only a single top-level item.

I got fooled by the small amount of indentation between the expand/collapse arrows and the fact that the text on the first indentation level almost aligned with the root level.

@tryggvigy tryggvigy closed this Jun 28, 2023
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

Successfully merging this pull request may close these issues.

4 participants