Inconsiderate use of ARIA roles broke screen reader accessibility of merge request diff view.

Summary

Changes introduced in !207402 (merged) and released in GitLab 18.5 made MR diff view inaccessible to screen reader users.

Steps to reproduce

  1. Have a screen reader (in my case NVDA) running
  2. Open !207402 (diffs)
  3. Try to read changes done to files other than app/assets/javascripts/repository/file_tree_browser/components/tree_list.vue

Example Project

I have linked to the MR on gitlab.com, so I assume this should be sufficient.

What is the current bug behavior?

Only changes done to the first file are rendered in browse mode. Additionally the changes are rendered as part of the tree view, making reading them extremely inefficient, as the role of the control is being read with each line.

What is the expected correct behavior?

  • The MR diff view is not a tree view, as that is simply not an appropriate control from the accessibility perspective - more on that below.
  • Content of all files modified in the Mr can be accessed in browse mode.

Relevant logs and/or screenshots

Output of checks

<This bug happens on GitLab.com

Results of GitLab environment info

Expand for output related to GitLab environment info 

(For installations with omnibus-gitlab package run and paste the output of:
`sudo gitlab-rake gitlab:env:info`)

(For installations from source run and paste the output of:
`sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)

Results of GitLab application Check

Expand for output related to the GitLab application check 
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true)


(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true)


(we will only investigate if the tests are passing)

Possible fixes

Some context here, to ensure everyone has the same understanding. When reviewing merge requests, screen reader users stay in the browse mode all the time, unless they need to write an actual comment. That is because, (unless more advanced screen reader techniques such as object navigation are used) that is the only mode where static text such as code changes can be read. Tree views however, are inherently operated in the focus / forms mode, because you need to use an actual navigation keys to move between tree items, rather than just for reading the web page. Therefore screen readers supporting virtual buffers render only the first tree view entry in browse mode, as the expectation is that upon encountering the tree view user will switch to focus mode to interact with the tree. This means that introducing ARIA roles marking a control as a tree, yet not implementing keyboard navigation at the same time, breaks accessibility of the interface significantly. In this particular case however, even with the proper keyboard accessibility in-place, the experience would be very sub-optimal, as the tree items are not supposed to be interactive controls accessed in the browse mode, but just the simple items with textual names, between which you can navigate. In my opinion two things should happen to resolve this:

  • Most importantly the roles introduced in !207402 (merged) should be reverted from the MR diff view, to restore the pre 18.5 level of accessibility
  • Long term, if the tree has to be used in the diff view at all, the only workable approach I can see here, would be to make the list of modified files a tree, implement proper keyboard navigation at the same time, move control displaying actual code changes out of the tree so that it can be interacted with in browse mode and ensure that whenever the file in the tree view changes the content of the file in the separate control mentioned above follows.

This breaks my ability to effectively participate in code reviews at work. The work around I employ, is to use AxSHammer to remove all ARIA roles on the web page. If that won't be fixed long term I'll probably have to write an user script working around the issue, but I'd really prefer to avoid this.

Also looking at the MR which introduced this regression, I don't see any mention of screen reader testing being performed, so that probably is also something worth adding to the acceptance process.

CC-ing @psjakubowska and @jerasmus as an reviewer and author of the regressor.

Patch release information for backports

If the bug fix needs to be backported in a patch release to a version under the maintenance policy, please follow the steps on the patch release runbook for GitLab engineers.

Refer to the internal "Release Information" dashboard for information about the next patch release, including the targeted versions, expected release date, and current status.

High-severity bug remediation

To remediate high-severity issues requiring an internal release for single-tenant SaaS instances, refer to the internal release process for engineers.

Edited by 🤖 GitLab Bot 🤖