-
Notifications
You must be signed in to change notification settings - Fork 52
Use treegrid role for schema table #310
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,10 +97,12 @@ export default class SchemaTable extends LitElement { | |
| animation-duration: 0s !important; | ||
| } | ||
| .tr:not(.collapsed) + .object-body { | ||
| visibility: visible; | ||
| animation: linear 0.2s expand-height; | ||
| } | ||
| .tr.collapsed + .object-body { | ||
| animation: linear 0.2s collapse-height; | ||
| visibility: hidden; | ||
| max-height: 0; | ||
| } | ||
| .obj-toggle { | ||
|
|
@@ -148,12 +150,16 @@ export default class SchemaTable extends LitElement { | |
| max-width: 25%; | ||
| } | ||
| </style> | ||
| <div class="table ${this.interactive ? 'interactive' : ''}"> | ||
| <div class="table ${this.interactive ? 'interactive' : ''}" role="treegrid" aria-label="JSON model" tabindex="0" @keydown="${(e) => this.treegridKeydown(e)}" @focus="${(e) => { | ||
| e.target.removeAttribute('tabindex'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this have to do with tree grid role
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, this is a bit weird! First, this sets the role and adds a required accessible label. There are a few different ways of implementing keyboard nav with treegrid (see https://www.w3.org/WAI/ARIA/apg/patterns/treegrid/examples/treegrid-1/ for some examples). Due to the treegrid only having the expandable sections and not other complex functionality, I wanted to go with the simplest, where only individual cells are focusable and with a roving tabindex. This would require setting tabindex to 0 on the very first gridcell in the treegrid, and -1 on all other gridcells. However, with the recursive way this component is engineered, I couldn't see a simple way to hard code the tabindex to 0 on only the very first gridcell. This is the simplest solution I saw considering that setup: to set the tabindex on the grid itself, but immediately switch it to the first gridcell and remove it from the treegrid once it is accessed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly it feels like we should start at the beginning with a design. Can you share what your over technical strategy is here, and why it is that one? I don't like that I'm reviewing code that doesn't ever explain what we are doing and why. I would appreciate to read some words from you that start with something like:
I could imagine in the requirements are things like |
||
| e.target.querySelector('.td').tabIndex = 0; | ||
| e.target.querySelector('.td').focus(); | ||
| }}"> | ||
| <div style = 'border:1px solid var(--light-border-color)'> | ||
| <div style='display:flex; background-color: var(--bg2); padding:8px 4px; border-bottom:1px solid var(--light-border-color);'> | ||
| <div class='key' part="schema-key schema-table-header" style='font-family:var(--font-regular); font-weight:bold; color:var(--fg); padding-left:${firstColumnInitialPadding}px'> Field </div> | ||
| <div class='key-type' part="schema-type schema-table-header" style='font-family:var(--font-regular); font-weight:bold; color:var(--fg);'> Type </div> | ||
| <div class='key-descr' part="schema-description schema-table-header" style='font-family:var(--font-regular); font-weight:bold; color:var(--fg);'> Description </div> | ||
| <div style='display:flex; background-color: var(--bg2); padding:8px 4px; border-bottom:1px solid var(--light-border-color);' role="row" aria-level="1"> | ||
| <div class='key' role="columnheader" part="schema-key schema-table-header" style='font-family:var(--font-regular); font-weight:bold; color:var(--fg); padding-left:${firstColumnInitialPadding}px'> Field </div> | ||
| <div class='key-type' role="columnheader" part="schema-type schema-table-header" style='font-family:var(--font-regular); font-weight:bold; color:var(--fg);'> Type </div> | ||
| <div class='key-descr' role="columnheader" part="schema-description schema-table-header" style='font-family:var(--font-regular); font-weight:bold; color:var(--fg);'> Description </div> | ||
| </div> | ||
| ${result || ''} | ||
| </div> | ||
|
|
@@ -245,20 +251,20 @@ export default class SchemaTable extends LitElement { | |
| const outerResult = html` | ||
| ${newSchemaLevel >= 0 && key | ||
| ? html` | ||
| <div class='tr ${newSchemaLevel <= this.schemaExpandLevel ? '' : 'collapsed'} ${data['::circular'] ? 'circular-object' : 'object'} ${data['::type']}' data-obj='${keyLabel}'> | ||
| <div class="td no-select key ${data['::deprecated'] ? 'deprecated' : ''}" part="schema-key" | ||
| style='padding-left:${leftPadding}px; cursor: pointer' @click=${(e) => this.toggleObjectExpand(e)}> | ||
| <div class='tr ${newSchemaLevel <= this.schemaExpandLevel ? '' : 'collapsed'} ${data['::circular'] ? 'circular-object' : 'object'} ${data['::type']}' data-obj='${keyLabel}' role="row" aria-level="${schemaLevel}"> | ||
| <div class="td no-select key ${data['::deprecated'] ? 'deprecated' : ''}" part="schema-key" tabindex="-1" role="gridcell" | ||
| style='padding-left:${leftPadding}px; cursor: pointer' @click=${(e) => this.toggleObjectExpand(e)} @keydown="${(e) => { if (e.key === 'Enter') { e.target.click(); }}}" aria-expanded="true" aria-controls="${keyLabel}-obj-body"> | ||
| <div style="display: flex; align-items: center"> | ||
| ${(keyLabel || keyDescr) ? html`<div class='obj-toggle' data-obj='${keyLabel}'>▾</div>` : ''} | ||
| ${(keyLabel || keyDescr) ? html`<div class='obj-toggle' data-obj='${keyLabel}' aria-hidden="true">▾</div>` : ''} | ||
| ${data['::type'] === 'xxx-of-option' || key.startsWith('::OPTION') | ||
| ? html`<span class="key-label xxx-of-key">${keyLabel}</span><span class="${isOneOfLabel ? 'xxx-of-key' : 'xxx-of-descr'}">${keyDescr}</span>` | ||
| : keyLabel.endsWith('*') | ||
| ? html`<span class="key-label requiredStar" style="display:inline-block;" title="Required"> ${keyLabel.substring(0, keyLabel.length - 1)}</span>` | ||
| ? html`<span class="key-label requiredStar" style="display:inline-block;" role="img" aria-label=" (Required)" title="Required"> ${keyLabel.substring(0, keyLabel.length - 1)}</span>` | ||
| : html`<span class="key-label" style="display:inline-block;">${keyLabel === '::props' ? '' : keyLabel}</span>` | ||
| } | ||
| </div> | ||
| </div> | ||
| <div class='td key-type' part="schema-type"> | ||
| <div class='td key-type' part="schema-type" role="gridcell" tabindex="-1"> | ||
| ${displaySchemaLink | ||
| ? html`<div class="schema-link" style="overflow: hidden; text-overflow: ellipsis" @click='${() => this.scrollToSchemaComponentByName(displaySchemaLink)}'> | ||
| ${dataType === 'array' ? '[' : ''}<span style="color: var(--secondary-color)">${detailObjTypeDisplay}</span>${dataType === 'array' ? ']' : ''} | ||
|
|
@@ -267,20 +273,20 @@ export default class SchemaTable extends LitElement { | |
| } | ||
| <div class="attributes" title="${flags['🆁'] && 'Read only attribute' || flags['🆆'] && 'Write only attribute' || ''}">${flags['🆁'] || flags['🆆'] || ''}</div> | ||
| </div> | ||
| <div class='td key-descr' part="schema-description"> | ||
| <div class='td key-descr' role="gridcell" tabindex="-1" part="schema-description"> | ||
| <span class=" m-markdown-small">${unsafeHTML(toMarkdown(displayLine))}</span> | ||
| ${data['::metadata']?.constraints?.length | ||
| ? html`<div style='display:inline-block; line-break:anywhere; margin-right:8px'><span class='bold-text'>Constraints: </span>${data['::metadata'].constraints.join(', ')}</div><br>` : ''} | ||
| </div> | ||
| </div>` | ||
| : html` | ||
| ${data['::type'] === 'array' && dataType === 'array' | ||
| ? html`<div class='tr'> <div class='td'> ${dataType} </div> </div>` | ||
| ? html`<div class='tr' role="row"> <div class='td' role="gridcell" tabindex="-1"> ${dataType} </div> </div>` | ||
| : '' | ||
| } | ||
| ` | ||
| } | ||
| <div class='object-body'> | ||
| <div class='object-body' id='${keyLabel}-obj-body'> | ||
| ${recursiveResult} | ||
| <div> | ||
| `; | ||
|
|
@@ -301,20 +307,20 @@ export default class SchemaTable extends LitElement { | |
| } | ||
|
|
||
| const result = html` | ||
| <div class = "tr"> | ||
| <div class="td key ${deprecated ? 'deprecated' : ''}" part="schema-key" style='padding-left:${leftPadding}px'> | ||
| <div class = "tr" role="row" aria-level="${schemaLevel}"> | ||
| <div class="td key ${deprecated ? 'deprecated' : ''}" role="gridcell" tabindex="-1" part="schema-key" style='padding-left:${leftPadding}px'> | ||
| ${keyLabel?.endsWith('*') | ||
| ? html`<span class="key-label requiredStar" title="Required">${keyLabel.substring(0, keyLabel.length - 1)}</span>` | ||
| ? html`<span class="key-label requiredStar" role="img" aria-label=" (Required)" title="Required">${keyLabel.substring(0, keyLabel.length - 1)}</span>` | ||
| : key.startsWith('::OPTION') | ||
| ? html`<span class='xxx-of-key'>${keyLabel}</span><span class="xxx-of-descr">${keyDescr}</span>` | ||
| : html`${keyLabel ? html`<span class="key-label"> ${keyLabel}</span>` : html`<span class="xxx-of-descr">${schemaTitle}</span>`}` | ||
| } | ||
| </div> | ||
| <div class='td key-type' part="schema-type"> | ||
| <div class='td key-type' role="gridcell" tabindex="-1" part="schema-type"> | ||
| <div>${dataType === 'array' ? '[' : ''}<span class="${cssType}">${format || type}</span>${dataType === 'array' ? ']' : ''}</div> | ||
| <div class="attributes ${cssType}" style="font-family: var(--font-mono);" title="${readOrWriteOnly === '🆁' && 'Read only attribute' || readOrWriteOnly === '🆆' && 'Write only attribute' || ''}">${readOrWriteOnly}</div> | ||
| </div> | ||
| <div class='td key-descr' part="schema-description"> | ||
| <div class='td key-descr' role="gridcell" tabindex="-1" part="schema-description"> | ||
| <span class="m-markdown-small" style="vertical-align: middle;"> | ||
| ${unsafeHTML(toMarkdown(`${`${(schemaTitle || title) ? `**${schemaTitle || title}${schemaDescription || description ? ':' : ''}**` : ''} ${schemaDescription || description}` || ''}`))} | ||
| </span> | ||
|
|
@@ -333,6 +339,62 @@ export default class SchemaTable extends LitElement { | |
| toggleObjectExpand(e) { | ||
| const rowEl = e.target.closest('.tr'); | ||
| rowEl.classList.toggle('collapsed'); | ||
| e.target.setAttribute('aria-expanded', !rowEl.classList.contains('collapsed')); | ||
| } | ||
|
|
||
| treegridKeydown(e) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does any of this have to do with tree grid role
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/treegrid_role
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you are saying that you want to support keyboard navigation for the schema table, and that the treegrid defines expected accessible controls for such a dynamic table. Is that what you were saying? If so, I think we need to comment up this method with all the oddities because there are so many things here that make no sense to me:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This and the markup of the table itself is also important. I am not sure of what the experience is like for a screen reader user, but I believe there's important navigation options available with accurately tagged tables (being able to get the information of what row and column each cell belongs to). Which a treegrid also performs.
In my testing all the required keyboard options with shift + arrows were the existing default browser functions (selecting the text), so by excluding operations with the shift key I allowed for those automatically, rather than having them trapped by the arrow functions.
Gridcells are in fact used in the handling
You are 100% correct
This is called roving tabindex. From the treegrid role docs on MDN:
Using activedescendant instead of roving tabindex would require setting an id on each individual cell, and updating the activedescendant with each one as it was selected (a huge headache)
Home and End have a default action that must be prevented, the arrow keys on their own do not, and I don't want to prevent the default for keys in general (like tabs, space bar)
By only having the cells focussable, and not the rows as well, no row keyboard interactions need to be supported. It keeps things much, much simpler.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It sounds like I may be undercommenting the code itself. I just based this on what I saw in there already, but I can definitely go through and add more comments. |
||
| if (e.shiftKey) {return true;} | ||
| const cell = e.target; | ||
| if (cell.getAttribute('role') !== 'gridcell') {return true;} | ||
| const row = Array.from(cell.parentNode.children); | ||
| const table = Array.from(cell.closest('.table').querySelectorAll('.tr')); | ||
| const index = row.indexOf(cell); | ||
| const rIndex = table.indexOf(cell.parentNode); | ||
| switch (e.key) { | ||
| case 'ArrowRight': | ||
| if (index < row.length - 1) { | ||
| row[index + 1].tabIndex = 0; | ||
| row[index + 1].focus(); | ||
| cell.tabIndex = '-1'; | ||
| } | ||
| break; | ||
| case 'ArrowLeft': | ||
| if (index > 0) { | ||
| row[index - 1].tabIndex = 0; | ||
| row[index - 1].focus(); | ||
| cell.tabIndex = '-1'; | ||
| } | ||
| break; | ||
| case 'ArrowDown': | ||
| if (rIndex < table.length - 1) { | ||
| table[rIndex + 1].children[index].tabIndex = 0; | ||
| table[rIndex + 1].children[index].focus(); | ||
| cell.tabIndex = '-1'; | ||
| } | ||
| break; | ||
| case 'ArrowUp': | ||
| if (rIndex > 0) { | ||
| table[rIndex - 1].children[index].tabIndex = 0; | ||
| table[rIndex - 1].children[index].focus(); | ||
| cell.tabIndex = '-1'; | ||
| } | ||
| break; | ||
| case 'Home': | ||
| e.preventDefault(); | ||
| cell.tabIndex = '-1'; | ||
| row[0].tabIndex = 0; | ||
| row[0].focus(); | ||
| break; | ||
| case 'End': | ||
| e.preventDefault(); | ||
| cell.tabIndex = '-1'; | ||
| row[row.length - 1].tabIndex = 0; | ||
| row[row.length - 1].focus(); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| return true; | ||
| } | ||
| } | ||
| if (!customElements.get('openapi-explorer')) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,8 @@ export default css` | |
| .key-descr { | ||
| font-family:var(--font-regular); | ||
| flex-shrink: 1; | ||
| /* Force the description field to fill up the remaining space because some specs don't have descriptions for every property. */ | ||
| flex-grow: 1; | ||
|
ahamelers marked this conversation as resolved.
|
||
| text-overflow: ellipsis; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's going on here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| overflow: hidden; | ||
| display: none; | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR says use treegrid role, it doesn't say why or why the visibility needs to be changed to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The visibility change is about the expanding and collapsing sections, and their continued visibility to screen readers and the focus order. If a section is collapsed without either
visibility: hiddenordisplay: none, the section is still (confusingly) read by screen readers, and keyboard focus will become trapped in those sections (focus disappears visually from the screen, as the focussable content is accessible by keyboard, even though the content is not on screen). Because these sections use animations/transitions anddisplay:nonecannot be animated (and disrupts animations),visibilitywas used instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. If you split that out into its own PR, we can get that merged in while we address the other comments.
And fwiw, this response you provided is what is required in every PR description for the changes that are being made otherwise reviewers have no idea why the change that is being made is the right one.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I explained why the treegrid role should be applied in the PR description, if it's not clear please let me know what needs further explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the other changes, the buttons to expand and collapse these sections are not keyboard accessible at all (another of the original problems pointed out by our accessibility auditors). I'm sorry for the large PR, but it is all interconnected.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can always be split, there looks like 4 different PRs here:
I don't care the order that we go through these, but my recommendation is to start with the one that you believe is most important first. Because if we don't do that, it might mean nothing else is of value. Is that the javascript one?
Regarding the PR description, I want to help you improve future PR descriptions, for context here is what was said:
That doesn't mean anything to me. I don't care what auditors discovered, it doesn't matter that table uses non-table elements. That's not important. This segment could say something like:
This is equally useless, we don't need a review of why solution XYZ is irrelevant and doesn't apply, that offers no value what so ever, focus on what the solution is. An equally useless text that could have been here might as well have said, using ReactNative here wouldn't work because we aren't building a mobile application. Then the message goes on to say "Use this ARIA treegrid" but nowhere is explained why.
This section could have followed from the above one with
The solution to make it accessible is using treegrid which solves this problem, applying treegrid causes _these UX changes_ and causes screen readers to instead _fill in the blank_.and the last statement:
This just repeats what is in code, and offers no value, it is tantamount to a comment in code that looks like this:
Completely useless.
Does that help at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does give me a stronger idea of what you are looking for as far as explanations, and some of this (more generalized) could be added to the contributions guide, which is basically empty. For example, I'm not clear on what makes something a large "number of changes", like if I need to make a similar change to multiple files, can I make one PR, or do I need one for each file, or what?
I am just a developer myself. I am not an accessibility auditor or a screen reader user, it is difficult for me to say exactly what happens, to "insert the exact problem", in all of these use cases for inaccessible code. While keyboard navigation is easy to test, I have no idea what the bad experience vs the good experience is for screen reader users in all these cases—like any average developer, I have to rely on what MDN/ARIA docs say you need to do ("you have to do something for tabular data, but if you can modify the table, don't use
table, usetreegridhere we say in big red text!")—which is why I linked to the big red text.What I do have is a huge list of accessibility problems stemming from this project that were given to me to fix. For this specific PR, the relevant problems listed for me are:
I spent many days figuring out what all the feedback meant as far as implementation requirements. I tried to explain the requirement and the solution as succinctly as I could, but it is impossible for me to do so without pointing to outside expertise, or to fully explain the problem behind the requirement without pointing to the same.
I was legally required to make changes to this project code, to get my own project's website to pass an accessibility audit (accessibility requirements are in fact strong legal requirements, not meeting accessibility needs opens organizations up to lawsuits). I wanted to provide what I was able to back up the chain, as a courtesy to the open source project. But to me, this is very secondary to saving my own project from legal exposure, which is now already done. I can always update and publish my own fork as the "accessible version" of this project, if these changes are not considered worthwhile to the project core.