Closed Bug 1573162 Opened 5 years ago Closed 5 years ago

Graph page on Perfherder could load infinitely and doesn't display any output

Categories

(Tree Management :: Perfherder, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: sclements)

References

()

Details

Attachments

(1 file)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:70.0) Gecko/20100101 Firefox/70.0 ID:20190804214813

I have a bookmark as set a couple of weeks ago for the graph page which includes measurements of the Raptor video playback performance tests. When I try to open it today, I can see the selected graphs for a fraction of a second on the left side. Then they disappear again, and a loading indicator stays around forever.

Not a single graph is shown.

To reproduce open the URL as set in the URL field.

As said it is also in the above URL field right next to the whiteboard. :)

The reason it isn't working is because the color scheme changed to support 7 colors (it shouldn't actually break the page though, so apologies for that). That's in alignment with the Test Data Modal warning that would display when more than 6 tests were selected at once - "Displaying more than 6 graphs at a time is not supported in the UI." This message was from the old angular code - it was not a new addition and I had assumed it was for performance reasons. I hadn't realized people were trying to display this many tests at once (12). I can change the color scheme back to what it was previously, however that only supports 10 tests so the last two tests will show black for their legend colors.

The question now is how many visible tests should Perfherder graphs support showing at once (so how many legend colors)? Regardless of what is decided, that modal warning should be supplemented with a message in the UI for when more tests are added gradually but not enough at once to show the modal warning message.

Dave, can I get your input on this?

Flags: needinfo?(dave.hunt)

Henrik, do you always view all results on the graph if there is data (for all 10-12 results) or do you keep 12 there and then toggle the visibility of the graphs to view different tests as needed (so for example, keeping 3 or 4 visible and turning visibility off for others)?

Assignee: nobody → sclements
Status: NEW → ASSIGNED
Priority: -- → P1

I don't really use it anymore. It was just a bookmark I wanted to visit today. If something has changed now, I'm happy to recreate the setup with all platforms included, and update the bookmark. From my side keeping backward compatibility isn't a must have, especially because it is about more than 6 graphs included which is/was unsupported.

Well, even if we stick with the 6 colors I'll still create a patch so that it doesn't break the UI if there are more tests than there are colors for. No need to update the url.

Regarding the amount of colors, I do think keeping support to something small like 6 is reasonable... the higher amount of datapoints in the graph, the slower the rendering will be with regards to zooming/selection.

(In reply to Sarah Clements [:sclements] from comment #3)

The reason it isn't working is because the color scheme changed to support 7 colors (it shouldn't actually break the page though, so apologies for that). That's in alignment with the Test Data Modal warning that would display when more than 6 tests were selected at once - "Displaying more than 6 graphs at a time is not supported in the UI." This message was from the old angular code - it was not a new addition and I had assumed it was for performance reasons. I hadn't realized people were trying to display this many tests at once (12). I can change the color scheme back to what it was previously, however that only supports 10 tests so the last two tests will show black for their legend colors.

The question now is how many visible tests should Perfherder graphs support showing at once (so how many legend colors)? Regardless of what is decided, that modal warning should be supplemented with a message in the UI for when more tests are added gradually but not enough at once to show the modal warning message.

Dave, can I get your input on this?

We should show as many as we can before it becomes hard to distinguish, or performance becomes a concern. If we supported 10 before, it would be great to not reduce that. Instead of any further series being black, could we cycle through the colours?

Whatever the number of distinct colours is, we should display a warning that readability and performance may be affected, but allow the user to continue anyway. Maybe instead of a modal, we could display a banner above the graph and in the test picker?

Flags: needinfo?(dave.hunt)

Partly unrelated but something I would be interested to know is what method is being used to calculate the colors, and specifically the contrast between them.

We should show as many as we can before it becomes hard to distinguish, or performance becomes a concern.

Yes, and that depends on the time range selected also. I think it'd be a good idea to put the question to Ionut when he's testing the new graph library since performance with that library might be a bit different.

If we supported 10 before, it would be great to not reduce that. Instead of any further series being black, could we cycle through the colours?

I'm not sure what you mean by cycling through the colors - we'd want to avoid duplicating colors. But any tests in excess of the colors supported can be shown in grey with visibility defaulted to false. A user can toggle it to visible up to a max of the supported colors (I've implemented this in my most recent graphs conversion pr but with the 6 colors there to try it out).

A thought I had about stating that the graphs only support x num of tests, but providing colors in excess of that seems a little inconsistent. However, if you think that's the best call I'll make the change.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #8)

Partly unrelated but something I would be interested to know is what method is being used to calculate the colors, and specifically the contrast between them.

They aren't generated by the graph libraries. I'm not sure how the previously used colors were chosen. For the new color scheme, I attempted to select colors that would go with the the new layout and still be distinctive, but there's definite room for improvement (especially where accessibility is concerned). Do you find they don't have enough contrast?

(In reply to Sarah Clements [:sclements] from comment #10)

They aren't generated by the graph libraries. I'm not sure how the previously used colors were chosen. For the new color scheme, I attempted to select colors that would go with the the new layout and still be distinctive, but there's definite room for improvement (especially where accessibility is concerned). Do you find they don't have enough contrast?

I haven't tested it myself. But be aware of the contrast between those colors. Just using different colors only because they look different isn't enough. The inspector inside the dev tools has a nice new feature to tell about that. But I think that also a lot of sites are present in the internet which can guide you to find better values. Otherwise James will also be able to give you a good amount of information.

Flags: needinfo?(jteh)
Blocks: 1572722

As mentioned, I'm well aware it needs to be improved :) I've already been in discussions with James and Marco about accessibility improvements.

The inspector inside the dev tools has a nice new feature to tell about that

What's the feature and where can I find out more about it?

No longer blocks: 1572722

(removed the block since the this original issue doesn't have anything to do with accessibility)

As noted in comment 12, we've (very recently) begun discussions about an accessibility review and improvements for Perfherder. That said, thanks for thinking of this and flagging it, Henrik!

Since it was mentioned here, there are two contrast related features in Dev Tools worth being aware of:

  1. The Accessibility Inspector has tools for checking contrast. See https://developer.mozilla.org/en-US/docs/Tools/Accessibility_inspector#Find_contrast_issues_in_the_page and https://developer.mozilla.org/en-US/docs/Tools/Accessibility_inspector#Highlighting_of_UI_items .
  2. The colour picker in Nightly now provides colour contrast information, though I believe there are still some improvements coming. See bug 1478156.

That said, we should take any further a11y discussion to a separate bug/thread.

Flags: needinfo?(jteh)

Thanks James! I'll definitely check those out.

I've merged a patch to prevent the UI from breaking here: https://github.com/mozilla/treeherder/commit/0fb09c85b1cb1b378f4f4e97d49ead8fbd38545e

It should get pushed to production on Tuesday. More improvements to come in bug 1519995

I'll mark this as resolved. Dave, we can continue the conversation about the colors per my comment 9 here or in the final graphs review pr you were @mentioned on - whichever you prefer.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: