fix firefox/all screen reader flow #15082

[Ingi-Hong]

One-line summary

Realized I accidentally closed the last PR so reopening this one. My attempt at fixing the a11y issues brought up in the original issue. It extends off of #15293 which I thought was 99% of a solution and worth exploring further. This is my first time trying to fix a screen reader specific issue so feedback is appreciated!

Significant changes and points to review

This change focuses the header of the active step after the innerHTML is set. This ensures the focus only occurs when the user is navigating through the steps, and not on a full page load.

In #15293 the #installer-help and #browser-help modals were not working, due to the innerHTML being set and losing all attached listeners. Now after the innerHTML is set the modal listeners are re-attached.

Issue / Bugzilla link

#15082

Testing

Works with Orca, have not tested with other screen readers.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.05%. Comparing base (85f003a) to head (4f2bf13).
Report is 102 commits behind head on main.

Files with missing lines	Patch %	Lines
bedrock/firefox/views.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16172      +/-   ##
==========================================
+ Coverage   79.81%   80.05%   +0.24%     
==========================================
  Files         160      158       -2     
  Lines        8495     8495              
==========================================
+ Hits         6780     6801      +21     
+ Misses       1715     1694      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[robhudson]

If this is based off of #15293 it would be nice to keep that 1 commit intact and add your changes in a 2nd commit. That would help review the changes between the two.

[Ingi-Hong]

@robhudson sure, should I close this PR and restart off that branch or is there an easy way to merge that history into this branch?

[janbrasna]

BTW I think testing with the actual assistive technology first is essential, e.g. I was not able to get any change announced in VoiceOver using this focusable/tabindex angle or jumping to actual anchors added at that headings etc.; so I started trying live areas, but content changes also in the other column so that made it a bit harder to get the assertive announcements reliable/right (while also not replacing itself as part of whatever's being swapped in DOM asynchronously at the same time…)

I've only rebased Rob's commit to current codebase and it still applies cleanly, and started trying various things on top of it (compare/main...janbrasna:df21080) … and you're right, one of the things to sort out first is the lost listeners. I also noticed the innerHTML may need changing to outerHTML to get the correct markup given the partial returned?

[Ingi-Hong]

To be honest I just assumed it would work, was planning on testing later this week when I could get my hands on a mac. I'll convert this into a draft and experiment on top of it.

[janbrasna]

@Ingi-Hong Your assumption was nonetheless correct!;) I did quickly test this in Firefox and Safari on macOS with VoiceOver and the headings are indeed announced as they should! 🎉 This strategy looks promising.

[Ingi-Hong]

I'm curious - how did you implement it with tabindex originally?

[Ingi-Hong]

Hi @reemhamz, since you opened the original issue would you willing to test this out locally?

[reemhamz]

I no longer work on bedrock, but maybe @stephaniehobson can take a look if you need a front-ender

[maureenlholland]

Sorry for the delay. I think this is an improved experience. I tested on VoiceOver & NVDA and got the expected announcements.

Going to push to demo and see if we can get a member of the accessibility team to review

[robhudson]

FYI: I updated the 2nd commit to remove the extra spacing that was added so the diff is a bit cleaner.

[janbrasna]

I did test this over time in a few browsers in VoiceOver and it works beautifully. Really nice job! And the async flow is really nice in general — I also tinkered with its history state and it all seems happy.

(I only noticed one unrelated issue with no close button description announced after opening the help modals, but that's to track separately if you don't have a quick fix to slap onto this while testing…)

"I'm curious - how did you implement it with tabindex originally?"

@Ingi-Hong Ah, so I was lazy and just scripted the tabindex values in the first place, which might be kinda fragile, but I was also actually trying to focus one heading above (figting with its disabled state at that moment, hence why scripting the values) to actually announce what got selected as a confirmation, only then to flow naturally to the heading below etc. … failing that, I went to include the left column with its changes too as live regions, to announce the change of product (Beta, Developer etc.) picked up there… and it went overboard from that;) This looks much more promising in not trying to be too smart, and just honestly moving the focus after each selection to the next active anchor.

@robhudson Thanks! Yep noticed that too, there seemed to be actually an attempt to fix some whitespace in the license preamble, but a lot of unrelated things got reformatted with it too. Much easier to spot any changes now. That said, I think there might be some unintended changes left, that don't belong in the PR:

[janbrasna]

I think this may have been removed unintentionally(?)

[Ingi-Hong]

I ended up setting the string directly in main.html. Would it be best to define page_desc in base.html?

I'm not overly familiar with jinja, but it looks like I could use set for this.

[janbrasna]

This one too?

[Ingi-Hong]

I removed the fallback as it doesn't exist in the current version of main. It must be from an older version of main that Rob branched from.

[Ingi-Hong]

Will squash once I get the sign off!

[Ingi-Hong]

I only noticed one unrelated issue with no close button description announced after opening the help modals, but that's to track separately if you don't have a quick fix to slap onto this while testing

Tried to take this on in the latest commit. As it stands, it announces the same message per step navigation. If you think multiple readouts of the same message is redundant, we can manipulate the sr-only area with javascript. Everytime we move steps we can set the innerHTML to trigger a screen reader readout. The screen reader will only read the message out once, since the inner content will only change once.

Like this: https://stackoverflow.com/a/69741469

[stevejalim]

Hi @Ingi-Hong - just wanted to let you know we haven't forgotten about this improvement. We're juggling some bigger pieces right now and don't want to add in too many changes this week, but once things are quieter we'll get this approved and merged. Thanks for the contribution!

[stevejalim]

Hi @Ingi-Hong - just wanted to let you know we haven't forgotten about this improvement. We're juggling some bigger pieces right now and don't want to add in too many changes this week, but once things are quieter we'll get this approved and merged. Thanks for the contribution!

Actually, thinking about it, once our bigger changes have been done, this PR is more applicable to the www.firefox.com codebase over at github.com/mozmeao/springfield/

If you would like to re-open it there, that would be great, or one of the team here can port over your work (but you may lose the attribution/namecheck history in the process, if that's important to you)

Cheers,
Steve

[janbrasna]

(!'m following this one, and it will for some time apply to both; given the inside pages still keep the user on the site the download flow started; so once fine-tuned here /the last update was not tested yet, I think that's not the impact we want to add given a11y reviews actually pointed against this pattern per se/ I'll make sure to port it over to firefox.com and leave all the proper authorship along the way; it will just need some heavy path updates that happened when relocating these. — So also for these reasons, please keep the branch and WIP/edits without squashing; whoever will be in charge of merging this they'll take care of it, and it will leave the PR more manageable to port over to the other site.)

[Ingi-Hong]

No worries at all :). Just doing this for the learning experience.

[Ingi-Hong]

@janbrasna Could you expand on this?

the last update was not tested yet, I think that's not the impact we want to add given a11y reviews actually pointed against this pattern per se

[janbrasna]

@Ingi-Hong There is an internal advisory on a11y of the whole paged selection pattern and the components to amend or change one's decision are being pointed out as one of the weakest points in understanding how to navigate between the states. So they may need redoing in the near future, and it's probably not worth a) triggering all the locale communities to translate a new string that might get removed; and also b) any wording thereof might need to go through a11y advisory as well. So basically we would pretty much leave out 4f2bf13 — but I'll confirm it in a review later.