Skip to content

Adapt Tracks to High Contrast Mode.#5252

Merged
julienw merged 2 commits into
firefox-devtools:mainfrom
nchevobbe:hcm-selected-track
Jan 27, 2025
Merged

Adapt Tracks to High Contrast Mode.#5252
julienw merged 2 commits into
firefox-devtools:mainfrom
nchevobbe:hcm-selected-track

Conversation

@nchevobbe

@nchevobbe nchevobbe commented Dec 10, 2024

Copy link
Copy Markdown
Member

This patch makes the selected track use system colors for selected and hovered style.
It also switches from using a box-shadow to a before pseudo element to draw the left selected indicator so it's easier to handle.
We also add a few CSS variables simplify computation.

Here Screenshots track is hovered, GPU Process is selected

Dark Light
before image image
after image image

Comment thread src/components/timeline/Track.css Outdated
@codecov

codecov Bot commented Dec 10, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.08%. Comparing base (9175e24) to head (92cac31).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5252   +/-   ##
=======================================
  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.

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

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the way you changed the implementation brings some subtle changes:

Adjacent selected tracks now have the track border "spilling" over into the selected border:
image

Also for local tracks:
image

Also global tracks now have their bottom box shadow spill over into the grey gutter at the right:
image

So I believe it would be best to revert the structural changes and only include the changes needed for the high contrast mode to work.

@nchevobbe

Copy link
Copy Markdown
Member Author

Actually the way you changed the implementation brings some subtle changes:

Adjacent selected tracks now have the track border "spilling" over into the selected border: image

Also for local tracks: image

Also global tracks now have their bottom box shadow spill over into the grey gutter at the right: image

So I believe it would be best to revert the structural changes and only include the changes needed for the high contrast mode to work.

ah right, sorry about that. I reverted my changes and only added the bits needed for HCM, let me know how that looks for you

@nchevobbe nchevobbe requested a review from julienw January 16, 2025 14:13
@julienw

julienw commented Jan 17, 2025

Copy link
Copy Markdown
Contributor

The hover on a track spans the entire track, do you think you could make it span just the title box?
In dark mode, this makes IMO the hover too visible for me on Linux (where it's the same as in light mode).

image

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the previous comment

This patch makes the tracks and their "hide" button
use system colors for selected and hovered style.
@nchevobbe

Copy link
Copy Markdown
Member Author

The hover on a track spans the entire track, do you think you could make it span just the title box? In dark mode, this makes IMO the hover too visible for me on Linux (where it's the same as in light mode).

image

sure, it should be good now

@nchevobbe nchevobbe requested a review from julienw January 24, 2025 15:03

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

The only thing that could be better, at least on my computer, is the hover style for the close button in light mode: it doesn't standout properly.
Also in dark mode: I feel like it's backwards: the border is white when not hovering, and dark when hovering (making it nearly disappear).

But they are small things and I'm fine merging it like that. But I'd be curious to know how it is on MacOS though.

@julienw julienw enabled auto-merge (squash) January 27, 2025 16:16
@julienw julienw merged commit 97d5830 into firefox-devtools:main Jan 27, 2025
@canova canova mentioned this pull request Jan 30, 2025
canova added a commit that referenced this pull request Jan 30, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility Related to making the profiler UI accessible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants