Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 81 additions & 19 deletions src/components/schema-table.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,12 @@ export default class SchemaTable extends LitElement {
animation-duration: 0s !important;
}
.tr:not(.collapsed) + .object-body {
visibility: visible;

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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: hidden or display: 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 and display:none cannot be animated (and disrupts animations), visibility was used instead.

Copy link
Copy Markdown
Member

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.

@ahamelers ahamelers Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

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

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.

Copy link
Copy Markdown
Contributor Author

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.

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.

@wparad wparad Jul 15, 2026

Copy link
Copy Markdown
Member

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:

  • update visibility to handle cases when the object-body isn't actually shown
  • improving aesthetic viewing of description fields when they are empty
  • Adding treegrid role/attributes to different places
  • Whatever is happening with the javascript that I just gave up reviewing at that point.

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:

Accessibility auditors discovered that in addition to containing clickable elements that cannot be navigated to via the keyboard, the schema table is a table in data, but uses non-table elements to display the tabular data. This means the tabular data is not accessible or understandable for screen readers.

Because the number of rows in the table can be changed after loading through the collapsible sections, the element cannot use a native HTML table or table roles, which are for static tables only, and must use ARIA treegrid (See the warning on the table role description).

This PR adds all the needed treegrid roles, required secondary aria attributes like labels and accurate aria-controls and aria-expanded for the expandable sections, and the recommended keyboard navigation for treegrid cells.

Accessibility auditors discovered that in addition to containing clickable elements that cannot be navigated to via the keyboard, the schema table is a table in data, but uses non-table elements to display the tabular data. This means the tabular data is not accessible or understandable for screen readers.

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:

Currently, the elements in the table are not accessible, when a screen reader navigates through the table, insert exact problem, instead of what should happen. This PR fixes this by insert description of why the fix in the PR fixes the problem and why it is the right solution.

Because the number of rows in the table can be changed after loading through the collapsible sections, the element cannot use a native HTML table or table roles, which are for static tables only, and must use [ARIA treegrid](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/treegrid_role) (See the warning on [the table role description](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/table_role#description)).

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 PR adds all the needed treegrid roles, required secondary aria attributes like labels and accurate aria-controls and aria-expanded for the expandable sections, and the recommended keyboard navigation for treegrid cells.

This just repeats what is in code, and offers no value, it is tantamount to a comment in code that looks like this:

// adds 1 plus 1 to get 2 and stores this in a variable called sum
const sum = 1 + 1;

Completely useless.

Does that help at all?

Copy link
Copy Markdown
Contributor Author

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, use treegrid here 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:

Data tables lack table markup that conveys relationships between cells and headers. Applies to all pages with column headers such as Field, Type, Description. Applies to multiple instances.

"Show/hide uses improper role and does not announce states. Applies to arrowheads on nav categories and inside tables under ""Model"" tabs.

Also applies to characters in ""Example"" code sections such as brackets and braces, which announce only ""clickable""."

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.

animation: linear 0.2s expand-height;
}
.tr.collapsed + .object-body {
animation: linear 0.2s collapse-height;
visibility: hidden;
max-height: 0;
}
.obj-toggle {
Expand Down Expand Up @@ -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');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does this have to do with tree grid role

@ahamelers ahamelers Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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:

To make the schema table accessible means that — list of requirements — the technical implementation to achieve that will be....

I could imagine in the requirements are things like all divs must have an aria role, that's easily understood, and more importantly reviewable.

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>
Expand Down Expand Up @@ -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' ? ']' : ''}
Expand All @@ -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>
`;
Expand All @@ -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>
Expand All @@ -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) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does any of this have to do with tree grid role

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A row that can be expanded or collapsed to show or hide a set of child rows is a parent row. Each parent row has the aria-expanded state set on either the row element or on a cell contained in the row.

From https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/treegrid_role

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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:

  1. Why are we explicitly excluding handling for the shift? doesn't that violate the tree grid accessibility?
  2. Is "gridcell" the correct role to be using here to justify not including it in the handling, maybe, it isn't immediately obvious for me.
  3. What is rIndex supposed to represent? index is almost always a bad name for things, I think we meant to call it columnNumber and rowNumber, is that what we meant?
  4. Why are we setting the tabIndex to -1 / 0, that feels very hacky what problem is it solving
  5. Why do we prevent default for some of these and not others
  6. Why isn't every treegrid keyboard interaction supported

@ahamelers ahamelers Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

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.

Why are we explicitly excluding handling for the shift? doesn't that violate the tree grid accessibility?

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.

Is "gridcell" the correct role to be using here to justify not including it in the handling, maybe, it isn't immediately obvious for me.

Gridcells are in fact used in the handling

What is rIndex supposed to represent? index is almost always a bad name for things, I think we meant to call it columnNumber and rowNumber, is that what we meant?

You are 100% correct

Why are we setting the tabIndex to -1 / 0, that feels very hacky what problem is it solving

This is called roving tabindex.

From the treegrid role docs on MDN:

If the treegrid container itself receives focus, the value of its aria-activedescendant property should reference the id of the selected row, columnheader, rowheader or gridcell, unless roving tabindex is used to manage focus between those roles, in which case aria-activedescendant should not be used.

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)

Why do we prevent default for some of these and not others

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)

Why isn't every treegrid keyboard interaction supported

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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')) {
Expand Down
2 changes: 2 additions & 0 deletions src/styles/schema-styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment thread
ahamelers marked this conversation as resolved.
text-overflow: ellipsis;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's going on here

@ahamelers ahamelers Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the treegrid each gridcell is focussable with the keyboard. Without flex-grow as well as flex shrink, if the last column of data has no content, it ends up also having zero width, which looks very strange when it's focused.

Desired effect:

Screenshot 2026-07-15 at 10 36 53

Without flex-grow: 1:

Screenshot 2026-07-15 at 10 37 18

overflow: hidden;
display: none;
Expand Down
Loading