[Accessibility]: move blame info from gutter into CodeMirror decorations to make them readable by screenreaders #44148
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 22bd1b1 and 5ee629d or learn more. Open explanation
|
taras-yemets
requested review from
fkling,
lrhacker,
philipp-spiess and
a team
|
Thanks for the writeup @taras-yemets.
Yeah if this would be possible this would be the best solution. I think another problem with announcing line numbers with this DOM structure is that the screenreader would first read all line numbers one by one (1, 2, 3, ...) before it even gets to the code part. So this does not seem very useful :/ So maybe the line numbers should stay The disconnect between the git blame annotation and the line of code is also annoying but maybe we can add a screenreader-only line number in here (so we only announce line numbers if we render git blame). This way there is at least some connection between the git blame information and the line of code? E.g.: <div class="cm-gutter BlameColumn">
<div class="BlameRow">
+ <div class="sr_only">123</div>
...the rest...
</div>
</div>
</div> |
|
How is the code read? Line by line? It would be possible to insert a screenreader only widget decoration at the start of each line. |
| // Make gutters content visible for the screen readers | ||
| useEffect(() => { | ||
| if (editor) { | ||
| editor.dom.querySelector('.cm-gutters')?.removeAttribute('aria-hidden') |
There was a problem hiding this comment.
This might not work anyways because the DOM is controlled by CodeMirror (so the attribute might be set again at some point).
There was a problem hiding this comment.
Yeah our other consideration was to use something like patch-package. The default here is really bad since it makes none of the gutters accessible for screenreaders, so maybe there's an argument for fixing it upstream (and only apply aria-hidden to the line numbers) 🤔
There was a problem hiding this comment.
This might not work anyways because the DOM is controlled by CodeMirror (so the attribute might be set again at some point).
What if we add gutters' dependencies (file blob, blame hunks, etc.) as useEffect dependencies? Could it help?
But yeah, it still sounds unsafe.
Yep:
Edit: oh looks like the text contents changed when I made the screenshot but it was saying that I’m on a text node and I could navigate through the rows with ctrl+option+arrow right
Would it be possible to also put links into a screenreader only widget? In that case, is the idea that render the git blame view as-is with |
Yes, but only on inside code rows; blame decorations don't seem to be available this way.
I can hardly imagine a hidden clickable element. Maybe I'm missing some context here... |
|
Anything can be put into an |
|
It seems like a really bad issue for CodeMirror that Gutter decorations like blame and line numbers are completely inaccessible. GitHub's code view and our old blob view expose these. We should definitely at least report it upstream, I couldn't find any open issue from a quick search... |
|
On second thought, it's understandable why they do this. They want to use You really want the line numbers, gutter etc to be associated with each other somehow. We will have this problem with the diff view too, if we want to migrate to CodeMirror there – you want the rows of the left and right-hand side of the diff to be associated with each other to enable side-by-side and up-and-down traversal. That's why we used a For line numbers, maybe the "association" could be achieved with The only idea I have is if CodeMirror used a table too, and maybe it would be possible to set |
|
A couple of thoughts:
|
|
Taras and I were looking into using widget decorations for this earlier today: We inserted a span at the beginning of every visible line to add a link (to mimic the git blame column). We got pretty far with that approach: The blame annotation is read by the screenreader as part of every line and it is automatically added as The screenreader would then read the blame information at the beginning of every line, so this approach mimics GitHub's blame view in that regard. So one option we can consider now is to move all of the git blame column inside the row as a widget: It doesn't even have to be sr_only, we can probably get it to look the same as the current gutter and we can easily add the hover overlays as well. The tab order will be the same as it is now until we make symbols inside the code tab-navigateable.
Yeah that is the big question. How can we find out an answer to that? I looked at a few other git blame views across the web: including GitHubs blame view, VS Code with GitLens, and Google Code Search: GitHub: Reads the blame information for each row. Most notably if a blame hunk is shown for multiple lines, it will only read it before the first line of this hunk. This is the order that the screenreader is reading: github.order.movVSCode with GitLens: Does not read the git blame data at all Google Code Search: They had clearly the worst experience, the screenreader was first reading all line numbers "1, 2, 3, ..." then all the source code and then all the blame rows 🤷 From what I learned by this exercise it's very safe for me to say that GitHub was the only thing remotely useful.
I don't see any way of doing this without forking CodeMirror, but maybe I’m missing something? |
|
^ To add to the examples above: GitLab: Similar to GitHub but slightly worse: The screenreader starts with the blame information, then reads all line numbers for the affect hunk, followed by the code: gitlab.mov |
|
I noticed another minor improvement that moving the git blame rendering inside a line could bring: Right now, when the viewport width is constrained, we resort to scrolling of the code view. If the git blame gutter is shown, it will take up a lot of space and make the scrollable area very small:
|
|
A couple of findings 👇🏻
Screen.Recording.2022-11-10.at.18.54.32.mov
Screen.Recording.2022-11-10.at.19.09.43.mov
Screen.Recording.2022-11-10.at.19.16.43.movAfter trying a bunch of different approaches, it seems to me that neither of them improves the screen reader's user experience dramatically, so I'm not sure if we should invest in the future development of any of them until we have a good understanding of what the ideal screen reader user journey is. Just to announce all the text content (line numbers, blame decorations, code content) without distinguishing it in a meaningful way seems insufficient (GitHub, GitLab, VSCode, and Google Code Search examples gathered by @philipp-spiess prove that, in my opinion). |
I can only reproduce this with tab navigation, not with More on that: https://accessibilityinsights.io/info-examples/web/aria-hidden-focus/
Yeah one thing I noticed at both GitHub and GitLab is that they announce the line number between the blame text and the code text. I think we could do this to (sr_only). This could make up for the |
I want to add here that It's probably hard for us to judge what is comprehensive and what not with how little experience (at least I) have with screen readers. There is still a difference though between connecting the blame information with the code line vs. not doing that. I would err on the side of giving more data (in this case the connection to the line of code) over making the information inaccessible. If this is distracting for users, they can always disable git blame anyways. |
I totally missed that! Thank you, @philipp-spiess! |
|
@taras-yemets Thanks for putting the PR up for review! I was checking it out locally and noticed a strange behavior while scrolling: There seems to be a delay between the git blame being rendered and the code being rendered. In that the, the text is shifted and rendered where the git blame should be. It also felt awkward when I first opened git blame and there was no UI feedback until the git blame data appeared from the backend. Is there any chance we can always render at least an empty placeholder? 🤔 Or maybe at least make it so that the text always starts offset so that there's no layout shift? Screen.Recording.2022-11-18.at.11.20.47.mov |
@philipp-spiess, nice catch! Thank you! On it. |
@philipp-spiess, addressed by rendering placeholders while waiting for the actual blame data. blame.big.file.mov |
felixfbecker
changed the title
| * so that they can be used as gutter spacer. | ||
| */ | ||
| const longestColumnDecorations = (hunks?: BlameHunk[]): BlameHunk | undefined => | ||
| hunks?.reduce((acc, hunk) => { |
There was a problem hiding this comment.
Phew good that we got rid of this, this requires traversal through all hunks 😅
| for (const { from, to } of view.visibleRanges) { | ||
| for (let position = from; position <= to; ) { | ||
| const line = view.state.doc.lineAt(position) | ||
| const matchingHunk = hunks.find(hunk => hunk.startLine === line.number) |
There was a problem hiding this comment.
Should we convert hunk into a Map so we have faster lookup here in a follow-up?
There's a catch though: With #44287 we'll need to be able to know what the hunk is for every line, not just the start line, so we can render the proper recency color.
So we'll need to do something like:
const map = new Map<number, Hunk>();
for (const hunk of hunks) {
for (const line = hunk.startLine; line <= hunk.endLine; line++) {
if (map.has(line)) {
console.error(
"This shouldn't happen, backend sent overlapping blame hunks"
);
}
map.set(line, hunk);
}
}Let's wait for this change though until we also merge #44199 which changes a lot on that infra.
taras-yemets
deleted the
ps/make-codemirror-gutters-readable-by-screenreaders
branch
@philipp-spiess could you elaborate the "obviously hacky" aspects and maybe file a follow-up issue to fix those? Since the whole CodeMirror migration is supposed to be a better more flexible architecture, we should of course strive to not have core features like this implemented in an "obviously hacky" way |
|
@felixfbecker I was replying to the updated message in the PR description, to quote @taras-yemets:
I think the follow-up that Taras proposed sounds good as well. I think this is something that should be fixed upstream and maybe we can even help with that. Sorry if "obviously" came a bit too harsh. This change is a great improvement to our accessibility so it's definitely worth it, even if we had to bend the rules a bit. |
Closes https://github.com/sourcegraph/sourcegraph/issues/43211
Moves blame decorations from gutter to the code line.
Two-dimensional code navigation suggestion suggested in this comment will be addressed in a follow-up issue https://github.com/sourcegraph/sourcegraph/issues/44594.
Screen.Recording.2022-11-18.at.11.04.17.mov
::beforeand::after), but it neither approach didn't work: spacing increases code line hight and it looks weird when selected/highlighted and pseudo elements are removed by the CodeMirror editor.To achieve the full-height-like column view with inline blame decorations we render empty gutter providing with and background color and shift the line content to the left by
var(--blame-decoration-width)so that inline blame decorations make use of the full-height background provided by the gutter.This approach comes with a drawback: we have to disable gutters sticky position (including line numbers) so that the blame gutter background is not scrolled horizontally with the content while the inline blame decoration is not.
Previous description 👇🏻 (didn't remove as it's referenced in comments, please ignore when reviewing the suggested changes)
>> start
Removes
aria-hidden="false"from the CodeMirror editor gutters.Although it fixes the referenced issue, I'm not sure it is the best approach to the CodeMirror file view with blame decorations open readability.
When the screen reader reaches the code editor area, it reads the content in the following order (see screenshot):
And it doesn't look like a perfect user experience 😞
Although we can hide line numbers from the screen reader (but do we really want it: Monaco editor and GitHub blame view, for example, announce line numbers), it doesn't help with reading all the blame decorations one by one before reaching the file content.
Potential solution
Line numbers and other types of content CodeMirror users may want to put into gutters (gutter description) are not expected to be accessible by the CodeMirror creators (source).
The potential solution could be to move blame decorations (and maybe line numbers) inside the line content (e.g., prepend the line code content with line number and commit info - see screenshot) so that they are announced in a more consumable way (e.g., "line 1", "$COMMIT_INFO", "$LINE_CONTENT"). But I'm not sure if it's a feasible way as I'm not a power CodeMirror user (I hope @fkling can help here).
>> end
Test plan
Tested manually that the git blame column content is read by the screenreader (VoiceOver).
App preview:
Check out the client app preview documentation to learn more.