Bug 1600759 - Perfherder's Graph Alt. View for Screen Readers #6001

yogmel · 2020-02-13T16:51:30Z

Description of issue

Perfherder's Graph View contains graphs and elements which are not screen reader accessible.

Tasks:

Next PR:

(Needs investigation) Show trends and outliers;

Proposed Solution

A button next to "Add test data" was added, to toggle between Graph and Table view.
If there is any highlighted revision, its table row will have a yellow background and an aria-label of "highlighted revision".
Users can show and hide test via Checkbox in Legend Card:

Testing

Compared some data points in the graph to the ones shown in table cells.
Turned highlighted revisions on and off and check if it propagated both in the graph and table.
Still was not able to test with SR. I will edit here once it's done.

codecov-io · 2020-02-13T17:01:19Z

Codecov Report

Merging #6001 into master will increase coverage by 0.7%.
The diff coverage is 74.75%.

@@            Coverage Diff            @@
##           master    #6001     +/-   ##
=========================================
+ Coverage   72.06%   72.76%   +0.7%     
=========================================
  Files         456      458      +2     
  Lines       18905    18988     +83     
  Branches     1534     1554     +20     
=========================================
+ Hits        13623    13817    +194     
+ Misses       4957     4842    -115     
- Partials      325      329      +4

Impacted Files	Coverage Δ
ui/perfherder/graphs/GraphsView.jsx	`0% <0%> (ø)`	⬆️
ui/perfherder/graphs/GraphsViewControls.jsx	`62.96% <40%> (-2.26%)`	⬇️
ui/perfherder/graphs/GraphsContainer.jsx	`63.3% <50%> (-0.2%)`	⬇️
ui/perfherder/graphs/TableView.jsx	`96.92% <96.92%> (ø)`
tests/selenium/pages/log_viewer.py	`100% <0%> (ø)`
treeherder/model/models.py	`91.41% <0%> (+0.6%)`	⬆️
treeherder/webapp/api/refdata.py	`92.59% <0%> (+3.7%)`	⬆️
treeherder/webapp/api/jobs.py	`91.3% <0%> (+3.86%)`	⬆️
treeherder/webapp/api/serializers.py	`96.87% <0%> (+4.16%)`	⬆️
... and 9 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 99d0202...8037617. Read the comment docs.

yogmel · 2020-02-13T17:11:33Z

Hi @camd and @sarah-clements ,
I have some comments:

I separated in commits to facilitate review;
GraphsContainer make it look like a lot of changes were done, but I added only a {!showTable && ()} around the component. The other changes was made by yarn lint I suppose.
I couldn't figure out how to get links and content from GraphTooltip, so I will need some help in this.

Thanks!

camd

This looks good to me. I'd be curious if you could have Marco give it a test drive on prototype. Or perhaps you already did? :)

yogmel · 2020-02-14T03:08:41Z

Hi @camd, I still didn't talk to him about this PR, but I would love to do it.
I will try to schedule a call with him and ask someone to deploy this at prototype, if possible.

MarcoZehe · 2020-02-14T12:18:32Z

Note that from a markup point of view, this looks good to me.

sarah-clements · 2020-02-14T19:45:33Z

Great start! A few comments on the layout/visuals:

I don't think we need the legend cards on the left, since any relevant information is already in the column headers. Whatever is missing can be added (such as the framework, but I don't think we need the signature hash).
We don't need to make this as visually colorful as the graph. To keep it simple - and since the primary target is screen reader users - I think we should omit the colors on the columns used to differentiate the tests.
For the highlighting of revisions, please make it a color that matches with the secondary or info colors (like a light info color that's used for Alerts) and have it go all the way across the column.
For the aria-label of "highlighted revision" will this also convey the revision number too or is it communicated some other way?

ui/css/perf.css

ui/perfherder/graphs/GraphsViewControls.jsx

ui/perfherder/graphs/TableView.jsx

ionutgoldan · 2020-02-17T07:40:06Z

@bebef1987 @marianrai @alexandru-io please do a manual QA check for this PR, so I know you're ok with the changes done in Graphs view.

bebef1987 · 2020-02-17T09:08:45Z

Please add Commit information and a link to treeheder.
The current table format is not usable as the graph view

alexandru-io · 2020-02-17T09:50:03Z

1. I would place the Table/Graph view toggle before the Time range button and eventually search for a more representative toggle button graphic.

2. I would replace the Date column with the Commit one, with a link to pushlog and jobs, just like in the graph view. Eventually include in a tool-tip or parenthesis the date of the revision.

3. I would use another (eye-comforting) color to highlight the revisions and include the first column also in the highlight. I would also improve the number formatting for a better readability and the measurement unity (3151094.00 -> 3,151,094) and cut the zeroes when they contain no data.

4. I would use the empty row in the middle of the data columns on the top to represent to color of the test data and format the signature as in the legend card:

|   color   |
| Test name |
| repo      |
| platform  |

5. The color difference between Previous/Next button when they're available/not is too small when as they they're both available they look much like they are not.
Previous = unavailable, Next = available

Previous = available, Next = available

alexandru-io

This looks like a great work and I didn't find functionality issues yet, but in terms of usability the fixes above seem fair to me.

yogmel · 2020-02-20T13:14:34Z

This last commit adds most of the suggestions made above.

The information from the tooltip was inserted. The "job, compare" links only appear when there is a mouse hover or a keyboard focus.

There are still some additional tasks and tests to be made that I will update on the PR description.

alexandru-io · 2020-02-20T13:59:55Z

@yogmel this looks much better.
I tested around some scenarios and there are few issues:

when i tried to add a new test data in table view I am getting the error: Something went wrong TypeError: original[deltaValueKey] is undefined, but not for all tests. Couldn't identify for what tests. For example I tried to add a talos test on a graph with raptor. But only table view throws this error. After F5 the page is loaded successfully on graph view, but it's thrown back when toggling to table view.
Not sure how the data is sorted, but something doesn't seem right. In the screesnhot below the dates end at Feb 20 (today) and continues from Jan 21 next hour after the previous from Feb 20. According to the timerange filter, the data should indeed stop on Jan 21, but the ascending/descending date sorting should be (Jan 21 ... Feb 20)/(Feb 20 ... Jan 21). Guess you need to change the data type of the filter.
Not sure whether you got to work on this or you're going to, but the information about repo and platform still looks "unfinished".

Overall this is a good piece of work. Few more fixes and we're good to go.

yogmel · 2020-02-27T16:53:17Z

In this latest push:

Add some tests: table_view_text.jsx and I had to create a mock data performance_testData.json.
Jamie's request (Screen-Reader improvements):
- Job and compare links are not SR accessible while hidden. For that reason, I made them appear all the time, but with low opacity. When focused or hovered, the links have full opacity (file: perf.css);
- "Table View" button is not the first element on DOM. The proposed solution here was to insert a sr-only button before the Legend Cards (file: GraphsView.jsx).
Add test Badge, for example "talos", "raptor" (file: TableView.jsx).

In TableView.jsx:

Refactor:
- Separate into different functions;
- Optimized data structure;
Fix date sorting
Fix "add test data" error: @alexandru-io I think that error was triggered when tests with empty cells were chosen. If that's not the case, and the problem still persists, please let me know.

yogmel · 2020-02-27T16:55:43Z

Not sure whether you got to work on this or you're going to, but the information about repo and platform still looks "unfinished".

@alexandru-io Thanks for your input!
Do you mean the style or the functionality? I realized later that users can click on repo and platform names on the Legend Card and it will open the "add test modal". Is that what you meant?

tests/ui/mock/performance_testData.json

ui/perfherder/graphs/GraphsView.jsx

sarah-clements · 2020-02-28T00:40:27Z

ui/perfherder/graphs/TableView.jsx

+    let prevRevision;
+    let pushUrl;
+    if (prevFlotDataPointIndex !== -1) {
+      prevRevision = item.data[prevFlotDataPointIndex].revision;


Some of this logic from the tooltip can be put into a helper function to be used by both components. That way if it needs to be changed in the future, you only have to change it in that helper function.

@yogmel, @sarah-clements just be aware that there's an open PR 6066 that is fixing the empty pushlogs from graph tooltip that appears from the second data-point (first retrigger) of a revision. This is because prevRevision is taking the revision hash from prevFlotDataPointIndex, and if the revision is the same, the pushlog link generated is not correct.

Moving this to a helper function might make fixing this bug harder if it doesn't pass dataPointDetails and testDetails as params. I don't mind either if you include the fix into this PR. Whatever works best for you. Thanks

sarah-clements · 2020-02-28T00:44:42Z

This looks good but let's focus on getting this approved and landed. You should do any follow up improvements/functionality and features in a separate pull request, otherwise this will get too big.

sarah-clements · 2020-02-28T00:48:54Z

ui/perfherder/graphs/TableView.jsx

+              dataIndex,
+              dataPoint,
+              highlightAlerts,
+              highlightedRevisions,


What changed in this file from the last commit?

Some functions were created and I changed the data structured sent to the table. I basically removed this and moved it into an object. I thought it was cleaner.

Before | Current

(there are some lines highlighted, but apparently Github doesn't jump to them when you click the link)

yogmel · 2020-02-28T17:47:52Z

I have implemented @sarah-clements's suggestions.

About the separation of helpers' functions, I can wait for @alexandru-io merge or implement in the follow-up PR.

sarah-clements · 2020-02-28T18:12:16Z

I tested this and I noticed that the 'highlight alerts' button next to the input boxes is controlling the highlighting for the revisions. Those are two separate features (check the implementation in the graphs view). So the toggle for highlight alerts should not control whether the revisions are highlighted. If you want to see a highlighted alert, go to the alerts view and hover over a test - click on the graphs link (the effect on the datapoint is the same). If we don't have the highlight alert functionality implemented in this pr, that button should be hidden until it's implemented.

Also, please file new bugs for the additional features to be added. @yogmel you can assign yourself to them, but we should have bugs filed.

yogmel · 2020-02-29T00:31:51Z

@sarah-clements I have found the piece of code that gets the query params, but I would like to discuss options before making a move. This latest push hides the highlight alerts input in the table view.
I'll file all bugs at the start of next week. :)

ui/perfherder/graphs/GraphsViewControls.jsx

…hanges

…der improvements, refactor, fix errors

yogmel · 2020-03-03T14:27:31Z

@sarah-clements ok, changed to hide only the "Highlight Alerts" button. Input fields are still shown and highlight cells.

sarah-clements

Looks good - thanks for all your hard work on this :)

alexandru-io · 2020-03-04T07:22:35Z

Yep, looks good. Even the platform | repo thing looks better now with that nuance of gray. Good job, @yogmel !

yogmel · 2020-03-04T20:22:00Z

Thank you, everyone :)

…a#6001) Create a table view in graphs for screen reader users; add tests

yogmel requested review from sarah-clements and camd February 13, 2020 16:52

yogmel self-assigned this Feb 13, 2020

yogmel added this to In progress in Outreachy a11y improvements via automation Feb 13, 2020

camd approved these changes Feb 13, 2020

View reviewed changes

sarah-clements reviewed Feb 14, 2020

View reviewed changes

ui/css/perf.css Outdated Show resolved Hide resolved

sarah-clements reviewed Feb 14, 2020

View reviewed changes

ui/perfherder/graphs/GraphsViewControls.jsx Outdated Show resolved Hide resolved

sarah-clements reviewed Feb 14, 2020

View reviewed changes

ui/perfherder/graphs/TableView.jsx Outdated Show resolved Hide resolved

ionutgoldan requested review from bebef1987, marianrai and alexandru-io February 17, 2020 07:38

alexandru-io suggested changes Feb 17, 2020

View reviewed changes

sarah-clements temporarily deployed to treeherder-perfherder-a-1ohv8a February 18, 2020 19:51 Inactive

yogmel force-pushed the perfherder-alternative-graph branch from 1718e10 to 0ec9304 Compare February 19, 2020 20:50

yogmel force-pushed the perfherder-alternative-graph branch from 0ec9304 to 58b09d6 Compare February 20, 2020 13:20

yogmel force-pushed the perfherder-alternative-graph branch 2 times, most recently from e807134 to 2676d24 Compare February 27, 2020 16:42

yogmel requested a review from sarah-clements February 27, 2020 16:56

sarah-clements reviewed Feb 28, 2020

View reviewed changes

tests/ui/mock/performance_testData.json Outdated Show resolved Hide resolved

sarah-clements reviewed Feb 28, 2020

View reviewed changes

ui/perfherder/graphs/GraphsView.jsx Outdated Show resolved Hide resolved

sarah-clements reviewed Feb 28, 2020

View reviewed changes

sarah-clements removed request for bebef1987 and marianrai February 28, 2020 00:45

sarah-clements reviewed Feb 28, 2020

View reviewed changes

yogmel force-pushed the perfherder-alternative-graph branch from 2676d24 to 89d0f2f Compare February 28, 2020 17:39

camd approved these changes Feb 28, 2020

View reviewed changes

sarah-clements reviewed Mar 2, 2020

View reviewed changes

ui/perfherder/graphs/GraphsViewControls.jsx Outdated Show resolved Hide resolved

yogmel added 8 commits March 3, 2020 11:24

Bug 1600759 - Perfherder Alt. Graphs View for Screen Reader

f0a0a6a

Bug 1600759 - add styles

0add6b8

Bug 1600759 - highlight revision

562be6a

Bug 1600759 - changes based on feedback: add tooltip info and other c…

235d16b

…hanges

Bug 1600759 - changes based on feedback: add badge, tests, screen-rea…

7d1c769

…der improvements, refactor, fix errors

Bug 1600759 - changes based on feedback: mock test change

71b315b

Bug 1600759 - changes based on feedback: hide alert inputs

6001473

Bug 1600759 - changes based on feedback: hide only alert button

8037617

yogmel force-pushed the perfherder-alternative-graph branch from 011532e to 8037617 Compare March 3, 2020 14:25

yogmel requested a review from sarah-clements March 3, 2020 14:27

sarah-clements approved these changes Mar 4, 2020

View reviewed changes

sarah-clements merged commit 5a8c470 into mozilla:master Mar 4, 2020

Outreachy a11y improvements automation moved this from In progress to Done Mar 4, 2020

SuyashSalampuria pushed a commit to SuyashSalampuria/treeherder that referenced this pull request May 12, 2020

Bug 1600759 - Perfherder's Graph Alt. View for Screen Readers (mozill…

78ccd08

…a#6001) Create a table view in graphs for screen reader users; add tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1600759 - Perfherder's Graph Alt. View for Screen Readers #6001

Bug 1600759 - Perfherder's Graph Alt. View for Screen Readers #6001

yogmel commented Feb 13, 2020 •

edited

codecov-io commented Feb 13, 2020 •

edited

yogmel commented Feb 13, 2020

camd left a comment

yogmel commented Feb 14, 2020

MarcoZehe commented Feb 14, 2020

sarah-clements commented Feb 14, 2020 •

edited

ionutgoldan commented Feb 17, 2020 •

edited

bebef1987 commented Feb 17, 2020

alexandru-io commented Feb 17, 2020 •

edited

alexandru-io left a comment

yogmel commented Feb 20, 2020

alexandru-io commented Feb 20, 2020 •

edited

yogmel commented Feb 27, 2020

yogmel commented Feb 27, 2020

sarah-clements Feb 28, 2020

alexandru-io Feb 28, 2020

sarah-clements commented Feb 28, 2020 •

edited

sarah-clements Feb 28, 2020

yogmel Feb 28, 2020

yogmel commented Feb 28, 2020

sarah-clements commented Feb 28, 2020 •

edited

yogmel commented Feb 29, 2020

yogmel commented Mar 3, 2020

sarah-clements left a comment

alexandru-io commented Mar 4, 2020

yogmel commented Mar 4, 2020

Bug 1600759 - Perfherder's Graph Alt. View for Screen Readers #6001

Bug 1600759 - Perfherder's Graph Alt. View for Screen Readers #6001

Conversation

yogmel commented Feb 13, 2020 • edited

Description of issue

Proposed Solution

Testing

codecov-io commented Feb 13, 2020 • edited

Codecov Report

yogmel commented Feb 13, 2020

camd left a comment

Choose a reason for hiding this comment

yogmel commented Feb 14, 2020

MarcoZehe commented Feb 14, 2020

sarah-clements commented Feb 14, 2020 • edited

ionutgoldan commented Feb 17, 2020 • edited

bebef1987 commented Feb 17, 2020

alexandru-io commented Feb 17, 2020 • edited

alexandru-io left a comment

Choose a reason for hiding this comment

yogmel commented Feb 20, 2020

alexandru-io commented Feb 20, 2020 • edited

yogmel commented Feb 27, 2020

yogmel commented Feb 27, 2020

sarah-clements Feb 28, 2020

Choose a reason for hiding this comment

alexandru-io Feb 28, 2020

Choose a reason for hiding this comment

sarah-clements commented Feb 28, 2020 • edited

sarah-clements Feb 28, 2020

Choose a reason for hiding this comment

yogmel Feb 28, 2020

Choose a reason for hiding this comment

yogmel commented Feb 28, 2020

sarah-clements commented Feb 28, 2020 • edited

yogmel commented Feb 29, 2020

yogmel commented Mar 3, 2020

sarah-clements left a comment

Choose a reason for hiding this comment

alexandru-io commented Mar 4, 2020

yogmel commented Mar 4, 2020

yogmel commented Feb 13, 2020 •

edited

codecov-io commented Feb 13, 2020 •

edited

sarah-clements commented Feb 14, 2020 •

edited

ionutgoldan commented Feb 17, 2020 •

edited

alexandru-io commented Feb 17, 2020 •

edited

alexandru-io commented Feb 20, 2020 •

edited

sarah-clements commented Feb 28, 2020 •

edited

sarah-clements commented Feb 28, 2020 •

edited