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

<include-fragment> and Screen Reader Accessibility #61

Open
Menelion opened this issue Feb 17, 2021 · 16 comments
Open

<include-fragment> and Screen Reader Accessibility #61

Menelion opened this issue Feb 17, 2021 · 16 comments

Comments

@Menelion
Copy link

I have the following issue.
I'm a totally blind software engineer and use a screen reader. Obviously, in our company we use Github for managing code. I'm used to review pull requests without any hassles, but recently that has started.
In our company larger pull requests are common (that's debatable whether it's good or bad, but it is as it is). I had to review a pull request containing 54 changed files. With my screen reader I could see only 18 first diffs rendered, then there was nothing at all, nada. The remaining diffs were however visible on screen for sighted people.
When inspecting the source code of the page in my browser, I saw that the first 18 diff tables are indeed rendered correctly, then there is an <include-fragment src="..."> element with a link to all of the remaining diffs. I tried to open that link as a separate web page, but of course I had no luck in seeing comments properly and so on.

My question is: Is it incorrect usage pattern of this library on github or is this library inaccessible in its nature?
this is a huge show stopper for my work. Github has always been an extremely accessible and friendly network to work with, but now it's really disappointing.

@keithamus
Copy link
Member

Thanks very much for the issue @Menelion and I'm sorry that this isn't working for you 😞. We absolutely want to ensure workflows on GitHub are accessible both with and without screen readers so if this is not the case, then we consider this a bug and want to fix it.

The <include-fragment> element should load make a request for the page contents upon page load, so <include-fragment> elements should not be present on a fully loaded page, unless the request for the additional page content failed. Based on our research & development this pattern should be accessible. Our example page has a simple demonstration which should read "Loading", that then eventually changes to "Works".

If you could please reach out to GitHub Support via https://support.github.com/contact, with the PR you mentioned, we can investigate the issue further with you.

@Menelion
Copy link
Author

Thanks @keithamus for your reply. Your demo page shows "Works" for me, so it should indeed be accessible.
I did contact Github with that. Unfortunately, it's a private repo, so I can't show you the PR straight away. If you want me to make a test repo with a test PR, please don't hesitate to ask, I'll try to reproduce this. for some reason, I can see only the first eighteen diffs...

@Menelion
Copy link
Author

Hey @keithamus! It seems, I found a large pull request in a public repo where I reproduce this. Here I see exactly five tables, however my sighted wife confirmed there are at least twenty of them (should be 57, see the top of the page):
symfony/symfony#40145

@koddsson
Copy link
Contributor

Hey @Menelion!

@keithamus and I have been looking into this today and we can reproduce this using VoiceOver on macOS. We aren't quite sure why this is happening. We're going to continue to look into the issue.

As a workaround, you can access the diffs using the "Jump to…" form control. This however isn't ideal and we'd like to fix the original issue so you can return to using your normal workflow.

@Menelion
Copy link
Author

Hi @koddsson, Jump To is not a workaround in any way. I don't remember how VoiceOver works (it's a... well... particular screen reader, so to say), but on Windows both JAWS and NVDA when you select a file to jump to (you do it in Forms mode in JAWS or Focus mode in NVDA), it shows the corresponding diff indeed, but Forms/Focus mode is for editing and menus only, and you can't interact with the tables in this mode. But as soon as you exit the Forms/Focus mode and return to normal browsing (Virtual cursor in JAWS / Browse mode in NVDA), you are thrown to the top of the page and the table disappears again.
So, in fact, I can't review larger PRs now at all. It's really frustrating that Github became an inaccessible tool with such a small change.

@koddsson
Copy link
Contributor

@Menelion Oh, that's no good 😞 . We're trying to chase up what change was made and how we can revert or fix it.

@alex19EP
Copy link

Hey @keithamus! It seems, I found a large pull request in a public repo where I reproduce this. Here I see exactly five tables, however my sighted wife confirmed there are at least twenty of them (should be 57, see the top of the page):
symfony/symfony#40145

hello @Menelion just trying to be helpful.
my setup is:

  • NVDA 2020.4
  • firefox 86.0
  • windows 10 pro 19042.844

I see all modified files in PR that you provided. moreover if I use Jump To file and select last file in pr, then exit from form mode - nvda stays on the selected file.

@Menelion
Copy link
Author

@alex19EP Do you see those in chrome also? I tried Chrome and Edge. @zersiax confirmed upon my request that he also sees only 5 tables.

@zersiax
Copy link

zersiax commented Feb 25, 2021

Confirmed, in the pull request I looked at, using the t command to jump from table to table only yields 5 distinct tables. Tricks to manually scroll the page like holding down pageDown or the down arrow in focus mode with NVDA did not yield more tables.
The window was maximized, and no extensions or addons can have messed with the output.

@keithamus
Copy link
Member

Through further testing we've noticed that spending some time away from the browser tab (doing other tasks) and subsequently coming back to it, the tables listing updates to list all 57 diff tables. It is almost as if there is a stale tree which gets garbage collected somehow, then returning to the tab updates the tree which in turns means the screen reader can access all tables. So far attempts to reproduce with reduced code samples have been unsuccessful. We'll keep investigating.

@zersiax
Copy link

zersiax commented Feb 25, 2021

Just to be clear, did you manage to reproduce the problem with NVDA? I'd be happy to hop on a screen share to show this off if that helps, I'm not doing anything for the next few hours

@alex19EP
Copy link

@alex19EP Do you see those in chrome also? I tried Chrome and Edge. @zersiax confirmed upon my request that he also sees only 5 tables.

yes in chrome i see only 4 tables and goto button isn't working either.

to be honest, it looks like a bug in chrom itself. since for several versions I have seen pages elements on which are not visible to nvda, unfortunately I have not yet found a page that does not require registration in order to send an issue to chromium.

@koddsson
Copy link
Contributor

koddsson commented Mar 3, 2021

We've investigated internally and got to the conclusion that this is a Chrome bug. We've filed https://bugs.chromium.org/p/chromium/issues/detail?id=1183898 to the chromium team.

@koddsson
Copy link
Contributor

koddsson commented May 4, 2021

We've investigated internally and got to the conclusion that this is a Chrome bug. We've filed https://bugs.chromium.org/p/chromium/issues/detail?id=1183898 to the chromium team.

Looks like Chrome has landed the fix so I'm gonna close this issue. Feel free to comment or re-open if you think this is in error.

@koddsson koddsson closed this as completed May 4, 2021
@smockle
Copy link

smockle commented Jul 15, 2021

As noted in https://bugs.chromium.org/p/chromium/issues/detail?id=1183898#c23, this was partly a Chrome bug, but there is a second aspect we can improve—namely, taking steps (e.g. explicitly applying role="table") to identify these tables as data tables (not layout tables).

@smockle smockle reopened this Jul 15, 2021
@cbieser
Copy link

cbieser commented Sep 16, 2022

What about the aria-live=polite attribute? While loading the fragment set the aria-busy=true attribute.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-busy

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

No branches or pull requests

7 participants