Adapt FilterNavigatorBar to High Contrast Mode. by nchevobbe · Pull Request #5257 · firefox-devtools/profiler

nchevobbe · 2024-12-11T10:00:28Z

This ensures the different items are properly rendered in HCM:

the items gets a button color, and have a distinct hovered style
the selected item looks selected
the separator are visible (a dark version of the existing image was added)
the disabled items have full opacity and use a gray text

	dark	light
single profile navigator button
single profile navigator button hovered
with a few items (second one hovered, last one is the "preview")

codecov · 2024-12-11T11:18:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.08%. Comparing base (9175e24) to head (033a323).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5257   +/-   ##
=======================================
  Coverage   86.08%   86.08%           
=======================================
  Files         311      311           
  Lines       29678    29678           
  Branches     8190     8190           
=======================================
  Hits        25548    25548           
  Misses       3548     3548           
  Partials      582      582

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

julienw

Thanks!
I only have these small comments, tell me what you think and rerequest review when you're ready :-)

julienw · 2025-01-13T14:02:59Z

+@media (forced-colors: active) {
+  .filterNavigatorBar {
+    --internal-background-color: ButtonFace;
+    --internal-hover-background-color: SelectedItemText;


This one doesn't work very well for me on Linux when using the Light theme: it's white, so it doesn't well stand out with the white background around. Surprisingly with the dark theme this works fine, because SelectedItem and SelectedItemText stay the same (!) and therefore stand ou just fine!

It would be good to fix that but if that not easy (and you consider it's more a bug in Linux) I'm also fine with keeping it.

Maybe using ButtonFace/ButtonText would make sense?

Could you post a screenshot of this please (with and without :hover)? So basically the issue is that SelectedItemText has the same/similar value as Canvas ? Could you check the computed values of those?
I'm using what's recommended by the accessibility team, but here the design isn't exactly the same (e.g. we don't have block borders). We could switch to using ButtonText bg and ButtonFace color, but that would make it different from other items we already have the SelectedItemText?SelectedItem pair for hover style :/

Thinking about it more, I think I'd prefer to use the same selected values: that is SelectedItem for background and SelectedItemText for text, on hover. They'd stand out more. Currently I don't think they're very "high contrast" when hovering, not even in your screenshots.
(I'm fine to swap them for the "active" state though).

What do you think?

Screenshots with the current version:
light mode:

dark mode:

It may also be that there's a deeper issue in Firefox, where SelectedItem/SelectedItemText don't change with the dark mode (at least for me on Linux).

Thinking about it more, I think I'd prefer to use the same selected values: that is SelectedItem for background and SelectedItemText for text, on hover. They'd stand out more. Currently I don't think they're very "high contrast" when hovering, not even in your screenshots. (I'm fine to swap them for the "active" state though).

What do you think?

That would make it hard to differentiate between hovered and selected, which isn't ideal.
At least in the screenshot you're providing the color of the text is updated, so that's something.
If we want to go bold, we could have a ButtonText background with a ButtonFace color, but it looked a tad brutal when I tried (but I think that was something Nathan suggested to me in some HCM review in DevTools, so that could work).
Would you mind trying this combination and telling me how it looks for you?

(The borders look a bit weird on your screenshots, not sure what this is about)

Thinking about it more, I think I'd prefer to use the same selected values: that is SelectedItem for background and SelectedItemText for text, on hover. They'd stand out more. Currently I don't think they're very "high contrast" when hovering, not even in your screenshots. (I'm fine to swap them for the "active" state though).
What do you think?

That would make it hard to differentiate between hovered and selected, which isn't ideal.

But to me that makes it obvious what a click operation will do: this will make the current hovered one selected.

At least in the screenshot you're providing the color of the text is updated, so that's something. If we want to go bold, we could have a ButtonText background with a ButtonFace color, but it looked a tad brutal when I tried (but I think that was something Nathan suggested to me in some HCM review in DevTools, so that could work). Would you mind trying this combination and telling me how it looks for you?

I can have a look yes.

(The borders look a bit weird on your screenshots, not sure what this is about)

This happens at some zoom level because of rounding differences (I think) between borders (sticking to integer number of pixels) and other things (not sticking to integer value of pixels). I never really bothered trying to fix this.

Thinking about it more, I think I'd prefer to use the same selected values: that is SelectedItem for background and SelectedItemText for text, on hover. They'd stand out more. Currently I don't think they're very "high contrast" when hovering, not even in your screenshots. (I'm fine to swap them for the "active" state though).
What do you think?

That would make it hard to differentiate between hovered and selected, which isn't ideal.

But to me that makes it obvious what a click operation will do: this will make the current hovered one selected.

Well, it kind of does, but you can't really have the same style for 2 different states, "to be selected" and "selected" are different. Would you be okay o land this as is for now? I already used it for a few items in the profiler, as well as in DevTools, and this is the combination that is recommended by the a11y team. maybe we could report the issue on Linux to them so they would come up with something better?

(The borders look a bit weird on your screenshots, not sure what this is about)

This happens at some zoom level because of rounding differences (I think) between borders (sticking to integer number of pixels) and other things (not sticking to integer value of pixels). I never really bothered trying to fix this.

Alright, not my patch then :)

Well, it kind of does, but you can't really have the same style for 2 different states, "to be selected" and "selected" are different.

I don't see why not, because one of them is transient while the other one is fixed.

Would you be okay o land this as is for now? I already used it for a few items in the profiler, as well as in DevTools, and this is the combination that is recommended by the a11y team. maybe we could report the issue on Linux to them so they would come up with something better?

I'm fine with landing it as is though, this is still a huge improvement! And this concern is a bit nitpicking :-)

This ensures the different items are properly rendered in HCM: - the items gets a button color, and have a distinct hovered style - the selected item looks selected - the separator are visible (light and dark HCM-specific version of the existing image were added) - the disabled items have full opacity and use a gray text

julienw

thanks!

Updates: [Julien Wajsberg] Some more small refactorings (#5320) [Markus Stange] Pass the correct sample index offset to getTimingsForCallNodeIndex for the flame graph tooltip. (#5328) [Nisarg Jhaveri] Update docs to include Android Studio/Simpleperf trace file support (#5309) [Markus Stange] Don't pass the preview filtered thread to getTimingsForPath/CallNodeIndex. (#5329) [Nazım Can Altınova] Add a "Sample timestamp" field to the sample tooltip in timeline (#5322) [Markus Stange] Reduce confusion between call tree summary strategy aware samples and regular samples (#5330) [Markus Stange] Rename this getCounter selector to getCounters. (#5337) [Markus Stange] Make sample indexes compatible between the unfiltered and (preview) filtered call tree summary strategy samples when using an allocation strat> [Markus Stange] Remove some code that uses the preview filtered thread (#5336) [Markus Stange] Remove getMarkerSchemaName special cases - look up marker schemas from data.type and nothing else (#5293) [Markus Stange] Remove the makeProfileSerializable step - make the raw in-memory profile match the format that's stored in the file (#5287) [Nicolas Chevobbe] Adapt FilterNavigatorBar to High Contrast Mode. (#5257) [Nicolas Chevobbe] Adapt Tracks to High Contrast Mode. (#5252) [Markus Stange] Adjust string index fields in markers when merging threads (#5344) [Theodoros Nikolaou] Localize title and aria label in ProfileName (#5345) [Julien Wajsberg] Adapt time-slice selection in High Contrast Mode. (#5259) [Markus Stange] Make stackTable (sub)category derived data (#5342) [Markus Stange] Compute cpuRatio values when computing the derived thread (#5288) [Nazım Can Altınova] Add a context menu item to open the JS scripts in DevTools debugger (#5295) Also thanks to our localizers: el: Jim Spentzos fr: Théo Chevalier it: Francesco Lodolo [:flod] zh-TW: Pin-guang Chen

nchevobbe force-pushed the hcm-navigator-bar branch from cec6825 to 85a71a4 Compare December 11, 2024 11:11

nchevobbe added the accessibility Related to making the profiler UI accessible label Dec 11, 2024

julienw requested changes Jan 13, 2025

View reviewed changes

nchevobbe force-pushed the hcm-navigator-bar branch from 85a71a4 to 033a323 Compare January 16, 2025 14:41

julienw approved these changes Jan 27, 2025

View reviewed changes

Merge branch 'main' into hcm-navigator-bar

7f54da0

julienw enabled auto-merge (squash) January 27, 2025 16:05

julienw merged commit ab68bda into firefox-devtools:main Jan 27, 2025

nchevobbe mentioned this pull request Jan 28, 2025

UI could look better in High Contrast Mode #5218

Open

44 tasks

canova mentioned this pull request Jan 30, 2025

Deploy Jan 30, 2025 #5348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adapt FilterNavigatorBar to High Contrast Mode.#5257

Adapt FilterNavigatorBar to High Contrast Mode.#5257
julienw merged 2 commits into
firefox-devtools:mainfrom
nchevobbe:hcm-navigator-bar

nchevobbe commented Dec 11, 2024 •

edited

Loading

Uh oh!

codecov Bot commented Dec 11, 2024 •

edited

Loading

Uh oh!

julienw left a comment

Uh oh!

Uh oh!

julienw Jan 13, 2025

Uh oh!

julienw Jan 13, 2025

Uh oh!

nchevobbe Jan 16, 2025

Uh oh!

julienw Jan 17, 2025 •

edited

Loading

Uh oh!

nchevobbe Jan 17, 2025

Uh oh!

julienw Jan 17, 2025

Uh oh!

nchevobbe Jan 27, 2025

Uh oh!

julienw Jan 27, 2025 •

edited

Loading

Uh oh!

Uh oh!

julienw left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nchevobbe commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julienw Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

julienw Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

nchevobbe Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

julienw Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchevobbe Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

julienw Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

nchevobbe Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

julienw Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nchevobbe commented Dec 11, 2024 •

edited

Loading

codecov Bot commented Dec 11, 2024 •

edited

Loading

julienw Jan 17, 2025 •

edited

Loading

julienw Jan 27, 2025 •

edited

Loading