Bug 1600752 - Accessibility improvements Compare and Alert Views

[yogmel]

Description of issue

There are a few suggested changes to improve accessibility in Perfherder's Compare and Alert Views:

Compare View:

• 1a. Add aria-label or aria-value attributes to progress bar
• 1b. Permalink title/aria text should include the test name
• 1c. Column titles and links should be shown all the time

Alert View:

• 2a. Checkbox needs label
• 2b. Alert Summary should be caption element
• 2c. Add Column titles
• 2d. Start needs to be button
• 2e. "Show more/less" needs to be buton

Proposed Solution

As most of the tasks were straightforward, I'm going to point their location and some comments below:

• 1a. ProgressBar.jsx: According to Reactstrap Github, there are still some issues in ProgressBar element. In this element, the props are not spread, making it not possible to insert more aria- attributes. The proposed solution is to insert an aria-label on the wrapper ProgressBar, insert a .sr-only <span> element with the description and insert a Table Header in CompareTable.jsx:127.
• 1b. CompareTable.jsx
• 1c: perf.css:272:303: I tried to show the links only when the table was focused, but the :focus attribute didn't work very well. The table may have become visually busy.

---

• 2a. AlertTableRow.jsx:211:216: an id was needed to link the Label's for attribute.
• 2b. AlertHeader.jsx: and additional css was needed to keep style in perf.css:260:263
• 2c. AlertTable.jsx: I may need some guidance on what the header titles should read.
• 2d. AlertTableRow.jsx:232:238: Utilities classes were needed to keep same style.
• 2e. I could not find this button.

---

P.S.: An additional commit was created because yarn lint returned Unused eslint-disable directive. This can be squashed into another commit if needed.

Testing

For both Views, visual test was made to make sure their layout was the same as before, except from what was requested.
With the screen reader (SR), navigation on the changed elements was made. It was also tested the headings navigation for 2b.

Compare View:
Link used to testing or any with results available can be used.

Alerts View:

• [SR] It reads both tables' headers, because they are one table inside another. This was not pointed in the bug, but I wonder if there is any way it could be better.
• Could not test checkbox and start functionality, as it is restricted to Sheriffs.

[sarah-clements]

Could not test checkbox and start functionality, as it is restricted to Sheriffs.

You can temporarily comment out this line for the checkbox: disabled={!user.isStaff} when testing it locally.

[sarah-clements]

You have some failing tests :) You'll need to run yarn test locally to see what tests have failed.

[sarah-clements]

A few notes:

• The "show more/less button" is in the TruncatedText component.
• I agree changing the links on the compare table rows to visible (not on hover) makes it look a little too busy. Is there another solution (or try making the text color a little bit lighter)?

[sarah-clements]

Looks great so far :)

[yogmel]

I have made some comments, mostly on what text to write on the tables. Once that's settled, I will send another commit with all the revision changes.

...
* I agree changing the links on the compare table rows to visible (not on hover) makes it look a little too busy. Is there another solution (or try making the text color a little bit lighter)?

I'll investigate, but making the text color a little bit lighter can be a solution.

You have some failing tests :) You'll need to run yarn test locally to see what tests have failed.

This should be solved on the next revision!

Looks great so far :)

Thanks!

[sarah-clements]

@yogmel Not sure if I've mentioned this before, but please include your changes in a separate commit that's called something like "changes based on feedback". We'll squash all of the commits into one once it's merged.

[yogmel]

@yogmel Not sure if I've mentioned this before, but please include your changes in a separate commit that's called something like "changes based on feedback". We'll squash all of the commits into one once it's merged.

Ok! I think I remembered that from the contribution period.

---

Also, the yarn testis failing, because of test/ui/perfherder/compare_table_test.jsx:241:246. For that reason, in CompareTable.jsx, I will insert an aria-label instead of changing the title attribute, if that's ok (I will send in on the next commit).

[codecov-io]

Codecov Report

Merging #5733 into master will increase coverage by 0.05%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #5733      +/-   ##
==========================================
+ Coverage   38.67%   38.73%   +0.05%     
==========================================
  Files         205      205              
  Lines        6668     6676       +8     
  Branches     1425     1430       +5     
==========================================
+ Hits         2579     2586       +7     
- Misses       3777     3778       +1     
  Partials      312      312

Impacted Files	Coverage Δ
ui/perfherder/alerts/AlertHeader.jsx	54.54% <ø> (ø)	⬆️
ui/perfherder/compare/CompareTableRow.jsx	56.25% <ø> (ø)	⬆️
ui/perfherder/compare/CompareTable.jsx	80% <ø> (ø)	⬆️
ui/perfherder/alerts/AlertTable.jsx	83.56% <ø> (ø)	⬆️
ui/shared/TruncatedText.jsx	15.38% <0%> (-1.29%)	⬇️
ui/perfherder/ProgressBar.jsx	100% <100%> (ø)	⬆️
ui/perfherder/alerts/AlertTableRow.jsx	80.72% <100%> (+1.23%)	⬆️
ui/push-health/ClassificationGroup.jsx	0% <0%> (ø)	⬆️
ui/perfherder/constants.js	100% <0%> (ø)	⬆️
ui/push-health/Metric.jsx	0% <0%> (ø)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7947b9f...2e5b5a3. Read the comment docs.

[yogmel]

I need some help with the test failure. :(
Locally, there is only one test failing:

   FAIL  tests/ui/job-view/Filtering_test.jsx (33.127s)
  ● Filtering › by author › should have 1 push

    : Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error:

      101 |     });
      102 | 
    > 103 |     test('should have 1 push', async () => {
          |     ^
      104 |       const { getAllByText, getByTestId } = render(<App />);
      105 |       // wait till the ``reviewbot`` authored push is shown before filtering.
      106 |       await waitForElement(() => getAllByText('reviewbot'));

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Suite.describe (tests/ui/job-view/Filtering_test.jsx:103:5)
      at Suite.Object.<anonymous>.describe (tests/ui/job-view/Filtering_test.jsx:76:3)
      at Object.<anonymous> (tests/ui/job-view/Filtering_test.jsx:23:1)

Although there are apparently other test failing, I did a yarn test on the master branch for comparison and they are the same.

Also, there is the security check failure, but I don't know where to look.

Thanks for your help!

[yogmel]

.

[sarah-clements]

await waitForElement(() => getAllByText('reviewbot'));

Sometimes that can happen if an element can't be found, so it keeps trying to find it but it times out. I suggest checking if the 'reviewbot' text or its element has changed (will it be retrieved by the getAllByText query or do you need to use a different one - check the react-testing-library docs).

[sarah-clements]

Although there are apparently other test failing, I did a yarn test on the master branch for comparison and they are the same.

I don't see any tests failing on my local master branch (and this shouldn't happen because we don't merge pull requests that don't pass tests). Be sure that before branching off of your local master you git fetch upstream and git rebase upstream/master to keep your local master branch up to date.

[yogmel]

Alright, the test passed. It was actually a yarn format issue.

[sarah-clements]

One little nit pick and this is ready to merge. Nice work!

[sarah-clements]

I've just noticed the show/more less button on the Alerts view should have the 'text-info' outline prop (same color outline as the text, not a dark gray background as pictured). You might need to change the showMoreClass.

[sarah-clements]

@yogmel Your changes look better but there's a few class names that don't need to in the TruncatedText's Button component anymore, like pointer. With the react-strap button component, the showMoreClass should be passed into the button 'color' prop (and changed according to what it accepts - look at the reactstrap docs).

This is how it used to look (aligned to the right). I think you'll need to change how the text-right and mb-0 classes are used.

[yogmel]

@sarah-clements I managed to make it look like before, with the addition of the focus style, with the classes d-block p-0 ml-auto.

I checked the Reactstrap documentation, but I didn't find the exact color for .text-info; the closest one is color="link". The blue hue is not the same though, so I had to keep the .text-info class, but I could drop 3 other ones.

If I'm looking on the wrong place, please let me know.

---

I also took some time to try again the "show links only when focused" and I'm excited to show that it worked!

:focus pseudo-selector didn't work, so I used :focus-within

[sarah-clements]

I checked the Reactstrap documentation, but I didn't find the exact color for .text-info; the closest one is color="link". The blue hue is not the same though, so I had to keep the .text-info class, but I could drop 3 other ones.

If I'm looking on the wrong place, please let me know.

"text-info" provides the same color as "info" (one is for font color instead of background color), so you can use "info" as the color prop. React strap uses the same CSS styles as Bootstrap, so you can find out more about the colors in those docs.

I also took some time to try again the "show links only when focused" and I'm excited to show that it worked!

Great! Let's go with that then.

[yogmel]

@sarah-clements I came up with this solution for the TruncatedText. When there is a color props passed down, it looks like an outlined button; when there isn't, it looks like a link:

In Compare View, when there is a no test warning. (Example link as of it now)

---

In Alert View, when the note is too long. (Example link)

[sarah-clements]

@sarah-clements I came up with this solution for the TruncatedText. When there is a color props passed down, it looks like an outlined button; when there isn't, it looks like a link:

In Compare View, when there is a no test warning. (Example link as of it now)

In Alert View, when the note is too long. (Example link)

Yes, I like this solution! This is a great way of making a component versatile and how we will frequently design generic components to work.

[sarah-clements]

Ok, I think this is ready to go :)