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 accesibility and keyboard navigation issues in examples #1433

Merged
merged 6 commits into from Jun 30, 2023

Conversation

Renerick
Copy link
Collaborator

See #1431

This PR fixes a number of issues with accessibility practices in the examples section of the docs. Most notably:

  • fix missing hrefs on links
  • replace links with buttons
  • add autofocus attribute to inline editing examples and "click to load"
  • fix missing input labels
  • make a progress bar a Proper Progress bar (aria attributes)

This is not an exhaustive fix of all issues that probably exist in the examples, but an effort to cover most obvious low hanging bugs

// templates
function lazyTemplate(page) {
return `<div hx-get="/graph" hx-trigger="load">
return `<div hx-get="/graph" aria-busy='true' hx-trigger="load">
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for single quotes here?

<h3 id="pblabel">${job.complete ? "Complete" : "Running"}</h3>

<div>
<div class="progress" role="progressbar" aria-valuemin="0" aria-valuemax="100" aria-valuenow="${job.percentComplete}" aria-labelledby="pblabel">
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use the <progress> tag? It would eliminate the need for ARIA annotations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CSS transitions

<a hx-get="/tab1" class="selected">Tab 1</a>
<a hx-get="/tab2">Tab 2</a>
<a hx-get="/tab3">Tab 3</a>
<a hx-get href="#" class="selected">Tab 1</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a mistake? The hx-get attrs now have no value.

Copy link
Collaborator Author

@Renerick Renerick May 10, 2023

Choose a reason for hiding this comment

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

yep, a typo, artifact of mass find and replace, thanks for noticing

Copy link
Contributor

@Htbaa Htbaa May 25, 2023

Choose a reason for hiding this comment

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

If a link isn't linking to anything it probably should be a button as this can be confusing to screen readers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reread the spec for tab in ARIA and updated this example accordingly, thanks for pointing this out

<a hx-get="/tab1" class="selected">Tab 1</a>
<a hx-get="/tab2">Tab 2</a>
<a hx-get="/tab3">Tab 3</a>
<a hx-get="/tab1" href="#" class="selected" autofocus>Tab 1</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This autofocus attribute causes the tab to be focused as soon as the page loads, before the user has interacted with or even seen the tab. Maybe use hyperscript or hx-on to do the focus instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem was from the fact that the first tab was being loaded lazily. Setting it with custom event handling would require filtration by events. I just made tabs prerendered, to keep the example simple

<h3 id="pblabel">${job.complete ? "Complete" : "Running"}</h3>

<div>
<div class="progress" role="progressbar" aria-valuemin="0" aria-valuemax="100" aria-valuenow="${job.percentComplete}" aria-labelledby="pblabel">
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the setup I tested (Firefox and Chrome, Linux, Orca) this moves focus to the body, announcing the title of the page and causing me to lose my position..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one I will get back to later. On my setup FF/Win10/NVDA focus (both tab and reader) stays around the progress bar, so I will have to dig a bit deeper

function rowsTemplate(page, contacts) {
var txt = "";
for (var i = 0; i < contacts.length; i++) {
var c = contacts[i];
txt += "<tr><td>" + c.name + "</td><td>" + c.email + "</td><td>" + c.id + "</td></tr>\n";
txt += `<tr ${i==0&&'autofocus'}><td> ${c.name} </td><td> ${c.email} </td><td> ${c.id} </td></tr>\n`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Elements that are focusable should have a name so that AT has something to announce. Maybe move the autofocus to a tbody (having one tbody per page) and adding a label of some sort?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Possible impl:

txt += `<tbody autofocus aria-label="Page ${page}">`
for (var i = 0; i < contacts.length; i++) {
  txt += "<tr><td>" + c.name + "</td><td>" + c.email + "</td><td>" + c.id + "</td></tr>\n";
  txt += `<tr><td>  ${c.name}  </td><td>  ${c.email}  </td><td>  ${c.id}  </td></tr>\n`;
}
txt += `</tbody>`

// templates
function lazyTemplate(page) {
return `<div hx-get="/graph" hx-trigger="load">
return `<div hx-get="/graph" aria-busy='true' hx-trigger="load">
Copy link
Collaborator

Choose a reason for hiding this comment

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

aria-busy is useless without aria-live.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@1cg
Copy link
Contributor

1cg commented May 11, 2023

is this mergeable?

@Renerick
Copy link
Collaborator Author

@1cg Kinda? Some of the issues are resolved, I'm currently stuck on the progress bar focus behavior

@Renerick
Copy link
Collaborator Author

I completely reworked how progress bar example works. Now instead of reloading an entire section, the bar is updated separately and "done" event is raised upon completion. The focus is moved on the status label with autofocus tabindex="-1". @dz4k please check if this implementation works on your setup

@dz4k
Copy link
Collaborator

dz4k commented May 15, 2023

Seems to work here. Orca reads "JOB" as "jay oh bee" but we should probably leave that be

@1cg
Copy link
Contributor

1cg commented May 23, 2023

am I to be merging?

@Renerick
Copy link
Collaborator Author

@1cg i fixed what I wanted to fix, so I think yes

@1cg 1cg merged commit 18aa247 into bigskysoftware:dev Jun 30, 2023
5 checks passed
@1cg
Copy link
Contributor

1cg commented Jun 30, 2023

thank you @Renerick @dz4k and @yawaramin

@Renerick Renerick deleted the 1431-a11y-examples-review branch July 19, 2023 10:58
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.

None yet

5 participants