Adapt time-slice selection in High Contrast Mode. by nchevobbe · Pull Request #5259 · firefox-devtools/profiler

nchevobbe · 2024-12-11T16:31:28Z

Make sure the start and end grips look like button, make the
outbound of the selection even dimmer and add border to make
the selection super obvious.

Using registered property we can get the rgb component from
given values that we can then turn into SVG filters that
we can then apply on top of the icon we use, in CSS.
The time-slice zoom icon takes advantage of this new capability
to adapt to High Contrast Mode.

	dark	light
timeslice selected, end "grip" is hovered
hovered zoom icon
active zoom icon

nchevobbe · 2024-12-11T16:32:04Z

I'm curious about your opinion on the second commit to have more control hover the icon colors

mstange · 2024-12-11T16:48:20Z

I think the filter solution is fine but you could use <feFlood> and <feComposite> so that you don't have to parse the rgba syntax. Example:

<filter id="--button-icon-color" color-interpolation-filters="sRGB">
  <feFlood flood-color="red"/>
  <feComposite operator="in" in2="SourceAlpha"/>
</filter>

mstange · 2024-12-11T18:15:48Z

Oh, I found an even better solution! You can use FillPaint in combination with the fill property! I didn't realize this worked for HTML elements, too.

<filter id="--use-fill-color" color-interpolation-filters="sRGB">
  <feComposite operator="in" in="FillPaint" in2="SourceAlpha"/>
</filter>

fill: green;
filter: url(#--use-fill-color);

nchevobbe · 2024-12-12T08:23:03Z

Oh, I found an even better solution! You can use FillPaint in combination with the fill property! I didn't realize this worked for HTML elements, too.
<filter id="--use-fill-color" color-interpolation-filters="sRGB">
  <feComposite operator="in" in="FillPaint" in2="SourceAlpha"/>
</filter>
fill: green;
filter: url(#--use-fill-color);

Unfortunately this doesn't seem to work in Chromium browser. Would have been nice!

I think the filter solution is fine but you could use <feFlood> and <feComposite> so that you don't have to parse the rgba syntax. Example:
<filter id="--button-icon-color" color-interpolation-filters="sRGB">
  <feFlood flood-color="red"/>
  <feComposite operator="in" in2="SourceAlpha"/>
</filter>

Ah yes that's better, I'll do that

nchevobbe · 2024-12-12T08:32:31Z

I don't really know flow, so not sure what's needed to please it here

julienw

I'm very newbie in terms of SVG filters. I looked at MDN but still I'm not sure I fully understand how filters work, but I'm not opposed.

I was a bit afraid that @property doesn't work in the old ESR, can you double check that the button still works as expected in that version? I don't mind that the forced-colors version doesn't work properly there, but at least the main way of working should work.

Thanks!

nchevobbe · 2025-01-16T14:47:41Z

I'm very newbie in terms of SVG filters. I looked at MDN but still I'm not sure I fully understand how filters work, but I'm not opposed.

I was a bit afraid that @property doesn't work in the old ESR, can you double check that the button still works as expected in that version? I don't mind that the forced-colors version doesn't work properly there, but at least the main way of working should work.

Thanks!

You mean current ESR (128) or the previous one (115) ? @property was added to 128, not sure how things look in 115 though

julienw · 2025-01-27T14:19:15Z

I'm very newbie in terms of SVG filters. I looked at MDN but still I'm not sure I fully understand how filters work, but I'm not opposed.
I was a bit afraid that @property doesn't work in the old ESR, can you double check that the button still works as expected in that version? I don't mind that the forced-colors version doesn't work properly there, but at least the main way of working should work.
Thanks!

You mean current ESR (128) or the previous one (115) ? @property was added to 128, not sure how things look in 115 though

yeah I meant v115, as far as I understand it's still supported.

Make sure the start and end grips look like button, make the outbound of the selection even dimmer and add border to make the selection super obvious.

nchevobbe · 2025-01-28T14:29:56Z

I'm very newbie in terms of SVG filters. I looked at MDN but still I'm not sure I fully understand how filters work, but I'm not opposed.
I was a bit afraid that @property doesn't work in the old ESR, can you double check that the button still works as expected in that version? I don't mind that the forced-colors version doesn't work properly there, but at least the main way of working should work.
Thanks!

You mean current ESR (128) or the previous one (115) ? @property was added to 128, not sure how things look in 115 though

yeah I meant v115, as far as I understand it's still supported.

testing this I realized I don't even need @property as flood-color values can be any type of css color, even the result of a color function (e.g. light-dark(red, blue) works fine). I'll remove the @property rules

nchevobbe · 2025-01-28T15:29:47Z

@julienw I checked on ESR 115 and everything seems to be okay, and I think I addressed all the comments you added, so this should be good for the final review :)

codecov · 2025-01-28T15:30:07Z

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 85.97%. Comparing base (8118f96) to head (a42f516).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/index.js	0.00%	18 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5259      +/-   ##
==========================================
- Coverage   86.04%   85.97%   -0.07%     
==========================================
  Files         311      311              
  Lines       29809    29832      +23     
  Branches     8217     8224       +7     
==========================================
  Hits        25648    25648              
- Misses       3575     3593      +18     
- Partials      586      591       +5

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

nchevobbe · 2025-01-28T15:40:02Z

not sure what changed that makes codecov unhappy now :/

julienw · 2025-01-29T11:03:19Z

don't worry about codecov, it's unhappy because the added code in index.js isn't exercised by the tests. I think that's OK, but if you'd like to exercise it, we would probably need to extract the code to a separate function in a different file (so that we could call it directly), and add something such as https://www.npmjs.com/package/jest-matchmedia-mock.

But again I'm not asking for this here :-)

julienw · 2025-01-29T12:47:57Z

+    --button-icon-hover-color: SelectedItem;
+    --button-icon-active-color: SelectedItem;


BTW the same than usual still happens: SelectedItem is not great for me in light mode. But out of scope here...

yeah, I need to reach to the accessibility team to sort this out

We can use CSS variables whose values are extracted and put into SVG filters that we can then apply on top of the icon we use, in CSS. The time-slice zoom icon takes advantage of this new capability to adapt to High Contrast Mode.

nchevobbe · 2025-01-29T12:59:22Z

don't worry about codecov, it's unhappy because the added code in index.js isn't exercised by the tests. I think that's OK, but if you'd like to exercise it, we would probably need to extract the code to a separate function in a different file (so that we could call it directly), and add something such as https://www.npmjs.com/package/jest-matchmedia-mock.

But again I'm not asking for this here :-)

if that's okay with you, I won't touch it then :)

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

mstange reviewed Dec 11, 2024

View reviewed changes

Comment thread src/components/timeline/Selection.css Outdated

nchevobbe force-pushed the hcm-time-selection branch from 6fd0211 to 5e53575 Compare December 11, 2024 16:45

nchevobbe force-pushed the hcm-time-selection branch from 5e53575 to 5c75e6b Compare December 12, 2024 08:31

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

julienw approved these changes Jan 13, 2025

View reviewed changes

Comment thread src/components/timeline/Selection.css Outdated

Comment thread src/index.js

Comment thread src/index.js Outdated

julienw reviewed Jan 27, 2025

View reviewed changes

Comment thread src/index.js

Adapt time-slice selection in High Contrast Mode.

b3ea366

Make sure the start and end grips look like button, make the outbound of the selection even dimmer and add border to make the selection super obvious.

nchevobbe force-pushed the hcm-time-selection branch 3 times, most recently from 3aecec9 to a32e0fa Compare January 28, 2025 15:23

nchevobbe requested a review from julienw January 28, 2025 15:28

nchevobbe commented Jan 28, 2025

View reviewed changes

Comment thread src/index.js Outdated

Comment thread src/index.js

julienw reviewed Jan 29, 2025

View reviewed changes

Add SVG filter to be able to control icons color in CSS

a42f516

We can use CSS variables whose values are extracted and put into SVG filters that we can then apply on top of the icon we use, in CSS. The time-slice zoom icon takes advantage of this new capability to adapt to High Contrast Mode.

nchevobbe force-pushed the hcm-time-selection branch from a32e0fa to a42f516 Compare January 29, 2025 12:58

Merge branch 'main' into hcm-time-selection

505a2a0

julienw enabled auto-merge January 29, 2025 13:57

julienw merged commit 34710b2 into firefox-devtools:main Jan 29, 2025

nchevobbe mentioned this pull request Jan 29, 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

		--button-icon-hover-color: SelectedItem;
		--button-icon-active-color: SelectedItem;

Uh oh!

Conversation

nchevobbe commented Dec 11, 2024

Uh oh!

nchevobbe commented Dec 11, 2024

Uh oh!

Uh oh!

mstange commented Dec 11, 2024

Uh oh!

mstange commented Dec 11, 2024

Uh oh!

nchevobbe commented Dec 12, 2024

Uh oh!

nchevobbe commented Dec 12, 2024

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nchevobbe commented Jan 16, 2025

Uh oh!

julienw commented Jan 27, 2025

Uh oh!

Uh oh!

nchevobbe commented Jan 28, 2025

Uh oh!

nchevobbe commented Jan 28, 2025

Uh oh!

codecov Bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

nchevobbe commented Jan 28, 2025

Uh oh!

julienw commented Jan 29, 2025

Uh oh!

julienw Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

nchevobbe Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

nchevobbe commented Jan 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jan 28, 2025 •

edited

Loading