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

Misc updates: styles, accessibility #59

Merged

Conversation

clpetersonucf
Copy link
Member

Some small visual + behavioral polish after the accessibility update:

  • Removed splash screen keyboard instructions button and rolled associated screenreader introduction into the start button
  • Style adjustments in player to prevent element "shifting" during certain transition animations
  • Fixed Next button location, movement, and behavior
  • Next button no longer auto focuses, to accommodate above fixes and to prevent feedback live region readback from being interrupted
  • Modest adjustments to several elements in the player to improve layout
  • Added slight debounce to focus-me directive (premature focusing during CSS transitions contributed to element "shifting")
  • Added preconnects for fonts to improve rendering

Tested in Firefox, Chrome, Safari, with and without VoiceOver. VO + Safari has several small issues - including a different tab sequence position for the Next button, and residual "shifting" when the Next button is clicked - but the issues are minor and shouldn't significantly affect non-sighted users.

@clpetersonucf clpetersonucf mentioned this pull request Oct 27, 2023
@dmols
Copy link
Contributor

dmols commented Oct 27, 2023

Runs great and tabbing works as expected, except for one small case that seemingly appears only when VO is on. You can tab into the top here and the VO will basically say "you've completed all the questions". This could be really misleading for non-sighted users, though I haven't found why it tabs to that specifically and why it doesn't highlight the whole banner block instead.

Screenshot 2023-10-27 at 5 53 43 PM

Note: This happened on Google Chrome.

Copy link
Contributor

@FrenjaminBanklin FrenjaminBanklin left a comment

Choose a reason for hiding this comment

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

For the most part the cleanup looks to be functioning well and looking good, though there are a couple of minor things.

When closing the keyboard instructions modal, it might be worth sending focus to the keyboard instructions button rather than the frame in general. Considering the modal can be opened at any time by pressing the 'H' key it's possible that the button won't be the last place that had focus prior to the modal opening, but it's a pretty good guess. Ideally the 'last focused' element could be tracked before moving focus to the modal, and then focus can be given back to that element after the modal is closed - but that feels like it'd be a bit much for a slight convenience.

That being said, I'm not sure that having a dedicated button to open the keyboard instructions modal is valuable for sighted users. I have no mobility issues so I don't know what would or would not be useful in that scenario, but I feel like having the H key just read out the instructions rather than open the modal (and leave the H key out of the sighted instructions altogether) might less cumbersome, since unsighted users don't need to close a modal after reading the instructions and sighted users would only know about the shortcut if they've manually opened the instructions anyway.

I'm not sure if I'm a fan of the added delay in the focus-me directive. If it was meant to delay giving focus to the 'Next' button until after it was done animating, I think that problem was handled more directly by not auto-focusing the 'Next' button after selecting an option. If a delay is absolutely necessary in the case of the 'Next' button I think it would be better to add some kind of condition inside the directive to determine whether it's acting on that button specifically, rather than delaying the focus for everything. It may also be worth adding some logic in the controller to make the Next button un-focusable until it's done animating just to be safe.

@clpetersonucf
Copy link
Member Author

  • Removed debounce from the focus-me directive, since it was originally added to accommodate the animation for the next button, but since the next button is no longer being autofocused, the debounce is no longer necessary.
  • The keyboard instructions dialog actually had support for focusing the previously selected element, but it did not appear to be working. It now works as intended.

That being said, I'm not sure that having a dedicated button to open the keyboard instructions modal is valuable for sighted users. I have no mobility issues so I don't know what would or would not be useful in that scenario, but I feel like having the H key just read out the instructions rather than open the modal (and leave the H key out of the sighted instructions altogether) might less cumbersome, since unsighted users don't need to close a modal after reading the instructions and sighted users would only know about the shortcut if they've manually opened the instructions anyway.

Thinking about this some more, I don't entirely disagree with your point but I'd prefer to err on the side of providing too much information instead of too little. I'd rather leave the modal intact for now instead of making additional sweeping change associated with removing it.

The only nuance with the current implementation is the focus behavior after selecting an answer choice: since the answer choices are rendered inert, focus changes to the widget frame after selecting a response, and tabbing focuses the next button (with the added mention of re-entering the widget frame.) It's a small thing, but slightly annoying. I experimented with using disabled instead of inert but then we lose the ability to quickly focus the next button after making a selection. Let me know if you all have opinions either way.

Copy link
Contributor

@dmols dmols left a comment

Choose a reason for hiding this comment

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

Turns out the tabbing issue I had previously was linked to my browser version. Having updated to the most recent version, the widget runs great! Only thing I'd suggest is that the media playback controls are disabled or hidden once the answer's been selected. As seen here, they're still in the tab order.

Screenshot 2023-10-30 at 5 22 31 PM

Copy link
Contributor

@cayb0rg cayb0rg left a comment

Choose a reason for hiding this comment

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

The only nuance with the current implementation is the focus behavior after selecting an answer choice: since the answer choices are rendered inert, focus changes to the widget frame after selecting a response, and tabbing focuses the next button (with the added mention of re-entering the widget frame.) It's a small thing, but slightly annoying. I experimented with using disabled instead of inert but then we lose the ability to quickly focus the next button after making a selection. Let me know if you all have opinions either way.

Since the de-focus to widget frame doesn't disrupt the tab order, this seems like a fine substitute. Tabbing to focus the next button feels natural, at least to me.

The media-playback controls are hidden to the screen reader but it's still possible to tab to the audio container (not possible to use VO keys, however).

That said, this looks good to go! Nice work.

Copy link
Member

@dgwn dgwn left a comment

Choose a reason for hiding this comment

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

Tested with Safari, Firefox, and Chrome. I was able to duplicate the discrepancy with Safari's tab-sequence being different (takes 2 extra tabs to reach 'Next' after getting question feedback). After viewing and closing instructions, the previously selected element is indeed highlighted again. Other than this, the navigation is intuitive and the layout looks good.

Copy link
Contributor

@FrenjaminBanklin FrenjaminBanklin left a comment

Choose a reason for hiding this comment

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

Audio controls do still seem to be reachable in the tab order after an option is selected (regardless of whether the audio controls are attached to the selected option), but otherwise it looks like this is ready to go.

Copy link
Contributor

@dmols dmols left a comment

Choose a reason for hiding this comment

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

Like Brandon mentioned, the audio elements are still accessible via tabbing. Not sure how confusing this could make navigating for non-sighted users. If it seems situational enough that we can leave it as is, I'm good saying this is ready to push!

@clpetersonucf clpetersonucf merged commit 83c2613 into ucfopen:master Nov 6, 2023
1 check passed
@clpetersonucf clpetersonucf deleted the update/style-and-misc-cleanup branch November 6, 2023 19:56
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.

5 participants