1511275 - [a11y] Semantically associate a line number with its respective line of code

Status: RESOLVED FIXED

(Webtools :: Searchfox, enhancement)

Description

[James Teh [:Jamie]]

When viewing a file in Searchfox, the file table contains two cells: one cell containing *all* of the line numbers and one cell containing *all* of the code. The line numbers are aligned appropriately with the code, but this alignment is purely visual. For screen reader users (which generally follow DOM order), this makes it impossible to associate a line number with its line of code and vice versa. The screen reader sees all the line numbers, then all the code.

In small code snippet viewers, it's easiest to just hide the line numbers for accessibility, since they're not useful enough to warrant anything more complex. However, for something like Searchfox which supports blame, direct linking to specific lines, etc., all this functionality is lost with that solution.

I'm not entirely sure why Searchfox (and many other code viewers) has been implemented this way, but my guess is that this relates to copy/paste; i.e. having the line numbers interspersed with the code would result in the line numbers being copied along with the code, since copy also uses DOM order. If preventing copying of line numbers were solvable in some other way, I'd suggest simply refactoring the DOM into a proper table, one row per line, each row containing two cells: line number and line of code.

Failing that, here's a way this can be fixed for accessibility without reordering the DOM:
1. Put role="presentation" on the following elements: #file, #file tbody, #file tbody tr, #file td.code
2. Get rid of the thead and header row. It's visually hidden anyway and it isn't useful to screen reader users either. (In fact, it might cause extraneous verbosity depending on settings.)
3. Put aria-hidden="true" on #line-numbers.
4. Put role="table" on #file pre. This is our "fake" table.
5. Give each .line-number role="cell".
6. Give each code element role="cell".
7. Get rid of the aria-labelledby on each code element. It is broken (it points to an invalid id) and it doesn't have the desired effect anyway.
8. Add a span element, either before each code element or wrapping each code element (either is fine). Give this role="row". Give it aria-owns="lxx line-xx", where xx is the line number. Here's an example with a wrapper:
<span role="row" aria-owns="l1 line-1">
  <code id="line-1">
    ...
  </code>
</span>
Here's the same example with the new node before the code:
<span role="row" aria-owns="l1 line-1">
</span>
<code id="line-1">
  ...
</code>

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Looking at how github currently does its source listings, they do exactly what's proposed in terms of being a table. Here's a 2-line excerpt from https://github.com/mozsearch/mozsearch/blob/master/tools/src/format.rs#L12

      <tr>
        <td id="L11" class="blob-num js-line-number" data-line-number="11"></td>
        <td id="LC11" class="blob-code blob-code-inner js-file-line"><span class="pl-k">use</span> languages<span class="pl-k">::</span>FormatAs;</td>
      </tr>
      <tr>
        <td id="L12" class="blob-num js-line-number" data-line-number="12"></td>
        <td id="LC12" class="blob-code blob-code-inner js-file-line"><span class="pl-k">use</span> links;</td>
      </tr>

They do use some intriguing styling though:

.blob-num::before {
    content: attr(data-line-number);
}

(Generated content doesn't get selected to the clipboard.)

Note that while github does seem to run some JS when ctrl-C is invoked, it doesn't seem necessary for the clipboard to do the right thing. I quickly pasted all of this into https://codepen.io/anon/pen/vMqGNb and it seems to work correctly.

And then some less intriguing styling:

.blob-code-inner {
	color: #24292e;
	font-family: SFMono-Regular,Consolas,Liberation Mono,Menlo,Courier,monospace;
	font-size: 12px;
	overflow: visible;
	white-space: pre;
	word-wrap: normal;
}
.blob-code {
	line-height: 20px;
	padding-left: 10px;
	padding-right: 10px;
	position: relative;
	vertical-align: top;
}

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Hm, noting that I'm not quite sure how to make "position: sticky" work with a table given the nesting we need to get layout to do most of the legwork for sticky scopes in bug 1533802. A similar problem arises for CSS grid, although maybe subgrids might work? Although we wouldn't really be leveraging the layout logic of either, and that might just give rise to performance issues...

The generated content trick doesn't need to involve a table, so maybe we could just interleave line numbers and code using div's still.

Comment 3

[James Teh [:Jamie]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

The generated content trick doesn't need to involve a table, so maybe we could just interleave line numbers and code using div's still.

Note that from an a11y perspective, interleaving with divs is fine, but it should still have table semantics for a11y. For example, this allows screen reader users to walk down the code column and skip all the line numbers if they wish, but still get to the line numbers on demand. This can be done with divs using ARIA roles:

<div role="table">
  <div role="row">
    <div role="cell">1</div>
    <div role="cell">// First line of code</div>
  </div>
</div>

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I'm going to try and do the div thing so that we can make the line-numbers position:sticky. :Jamie, can you recommend a good/representative screen-reader pairing for Firefox nightly for searchfox purposes (ex: JAWS or NVDA)? I normally develop on Linux but have OS X and Windows available in increasing amount of distance I have to walk from my primary desk.

Comment 5

[James Teh [:Jamie]]

I'd suggest NVDA; that's what I'm using here (and also what Marco Zehe uses, though he's on leave at the moment), which means we'll be able to provide the best guidance with that. Orca on Linux should work well too, but I'm not super familiar with it and thus can't provide solid guidance regarding expected results. I'm also very happy to test and provide feedback myself, though I don't know if we have a test instance of Searchfox anywhere that would make that possible...

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

My current changes are visible at https://dev.searchfox.org/. I haven't had a chance to try locally with NVDA.

Comment 7

[James Teh [:Jamie]]

Nice! This is awesome except for one thing:

The blame control for each line (.blame-strip) isn't in either of the ARIA table cells, so it breaks screen reader table navigation. I think the simplest way to solve this is to put the blame button in its own ARIA table cell (so it's a three column table). Unfortunately, because .blame-strip already has an ARIA role, that means you're going to need a wrapping span around it:

<div role="row" class="source-line-with-number">
  <span role="cell">
    <div class="blame-strip c1" data-blame="1846871a480b3c191b5e4fb93af1fc7e5068f597#accessible/src/base/RelationType.h#1" role="button" aria-label="blame" aria-expanded="false"></div>
  </span>
  <div id="l1" role="cell" class="line-number" data-line-number="1"></div>
  <code role="cell" class="source-line" id="line-1"><span class="syn_comment">/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */</span>
</code>
</div>

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Thanks for the very prompt feedback! I've kicked off another indexing run with these changes. I think it should take less than 3 hours to run. Note that the server provides caching directives of 1 day for most things, so you will need to explicitly refresh to see any changes. (If you have any advice on caching directives, help is also appreciated there! ;)

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

The re-index worked and I'm able to navigate the table using NVDA inexpertly. (I use NVDA+space to enter ?browse? mode, push "t" to navigate to the table, then use ctrl+alt+arrows to move around the table.) If I enter the blame cell I'm able to hit the enter/return key on the keyboard to make the blame pop-up visually toggle onto the screen, but I'm not sure what should be happening from an a11y perspective at that point. Should we be doing something so NVDA announces the pop-up or starts reading it, or am I not even in the right mode for that? Should the pop-up be a landmark?

At a high level, I'd like for us (in another bug) to add a doc to the mozsearch source code at mozsearch/docs/a11y.md that would:

1. Provide links to a good NVDA usage introduction. (No need to re-invent the wheel.)
2. Help walk a searchfox developer through the basics of how someone might use NVDA to use Searchfox.

My motivation is that in the near term I'm planning to (on a branch) do a bunch of feature additions that could complicate the pop-up menu and other UI affordances and I'd like to make sure that the core searchfox use-cases using NVDA don't get regressed by that.

Comment 10

[James Teh [:Jamie]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #9)

The re-index worked and I'm able to navigate the table using NVDA inexpertly. (I use NVDA+space to enter ?browse? mode, push "t" to navigate to the table, then use ctrl+alt+arrows to move around the table.)

Awesome! And that's precisely what I might do. :)

If I enter the blame cell I'm able to hit the enter/return key on the keyboard to make the blame pop-up visually toggle onto the screen, but I'm not sure what should be happening from an a11y perspective at that point. Should we be doing something so NVDA announces the pop-up or starts reading it, or am I not even in the right mode for that?

You're correct: the pop-up doesn't get read automatically and this has nothing to do with modes, etc. Making this read automatically is tricky. If this were a menu, I'd say focus the first item. However, it isn't a menu and it doesn't really make sense to focus the bug link. Ideally, .blame-strip should also be focusable so at least NVDA would read 'expanded" when enter is pressed. I'd suggest we address these issues separately.

There's a bigger problem here, though. Even though the pop-up won't get read automatically, the user should still be able to cursor down to it once it appears. That no longer works because the pop-up is contained inside the .blame-strip "button". In the production instance, .blame-strip and .blame-popup are siblings, rather than .blame-popup being a descendant. .blame-popup can't be a descendant of .blame-strip for this to b accessible.

At a high level, I'd like for us (in another bug) to add a doc to the mozsearch source code at mozsearch/docs/a11y.md that would:

1. Provide links to a good NVDA usage introduction. (No need to re-invent the wheel.)

These articles were written especially for web testers:
https://marcozehe.wordpress.com/articles/how-to-use-nvda-and-firefox-to-test-your-web-pages-for-accessibility/
https://webaim.org/articles/nvda/

2. Help walk a searchfox developer through the basics of how someone might use NVDA to use Searchfox.

The table navigation you've already experienced is probably the major task. Blame is tricky because the current experience has some rough edges as noted above, but I could walk you through how I would use it right now.

My motivation is that in the near term I'm planning to (on a branch) do a bunch of feature additions that could complicate the pop-up menu and other UI affordances and I'd like to make sure that the core searchfox use-cases using NVDA don't get regressed by that.

I really appreciate your willingness to test this yourself. I'm also super happy to quickly look over any changes if that's helpful.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

I've corrected the blame-popup regression. I had regressed that after reading some code in blame.js that only made sense if the blame-popup was parented under the .blame-strip itself. Also, I needed a "position: relative" containing block to make the blame-popup's "position: absolute" work correctly. Since we now have the extra role=cell element in there, making the blame-popup a sibling of the role=button .blame-strip works after styling the role=cell to be position:relative.

I also tested that I could use NVDA to hear the contents of the blame-popup, although there was no event notifying me anything had happened when I hit enter to display the blame data. Specifically, I am able to hit the enter key, then (after the blame popup appears... it does depend on an async XHR) I am able to hit the down arrow to hear the first line of the popup and keep hitting down to hear the rest of the lines.

I've attempted to document some of this at https://github.com/mozsearch/mozsearch/blob/master/docs/a11y.md (which doesn't get rendered to HTML for some reason?).

I've landed the changes in https://github.com/mozsearch/mozsearch/pull/221 and they should show up in tomorrow's indexings that should kick off at ~10am eastern and finish a few hours after that. Assuming I didn't mess anything up, I think we can then mark this bug fixed and file the appropriate follow-ups to improve the a11y blame UX.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

The latest changes have finished baking and are available on https://searchfox.org/. Because of the current caching setup, there's a real chance you might need to hit ctrl-r twice or until the display converges :).

Needinfo-ing to confirm that the blame-popup works as well as it previously did.

Comment 13

[James Teh [:Jamie]]

This is fantastic! Both the table and the blame popup work as expected. Thanks so much.