Stop using the :not(:target) selector in DevTools SVG icons

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 1 bug, {perf})

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

10 months ago
As part of fixing bug 1358998 we should stop using the :not(:target) selector in DevTools SVG icons.
(Assignee)

Comment 1

10 months ago
Created attachment 8861253 [details] [diff] [review]
patch for select-arrow.svg
Assignee: nobody → jwatt
Attachment #8861253 - Flags: review?(jryans)
(Assignee)

Comment 2

10 months ago
Created attachment 8861254 [details] [diff] [review]
patch for performance-icons.svg

Note than performance-overview-markers.svg and performance-overview-frames.svg are unused. I included them for completeness, but probably they should just be removed. Let me know.
Attachment #8861254 - Flags: review?(jryans)
(Assignee)

Comment 3

10 months ago
Created attachment 8861255 [details] [diff] [review]
patch for sort-arrows.svg
Attachment #8861255 - Flags: review?(jryans)
(Assignee)

Comment 4

10 months ago
The only other file under devtools that uses the :not(:target) selector is devtools/client/themes/images/breakpoint.svg. That file seems to be used in:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/codemirror/old-debugger.css

but the "old" in that file's name implies it is going away, so I didn't fix up this case.

That said, breakpoint.svg is also referenced in *comments* in:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/new/debugger.css
https://dxr.mozilla.org/mozilla-central/source/layout/style/test/gtest/example.css

so maybe breakpoint.svg does need fixed up if there's an intention to reference that file from those CSS files at some point in the future.
(Assignee)

Comment 5

10 months ago
(In reply to Jonathan Watt [:jwatt] from comment #1)
> Created attachment 8861253 [details] [diff] [review]
> patch for select-arrow.svg

This patch may require a bit of explanation.

The new SVG file uses 'context-fill' to get the fill color from its context (the embedding element).

The embedding elements happen to have the same value specified on them for 'color' as was specified for the 'fill' values for the individual icons in the old file. This allows us to set 'fill' to 'currentColor' on the embedding element to pick up the correct color as the context fill for each image instance.
(Assignee)

Updated

10 months ago
Keywords: perf
Comment on attachment 8861254 [details] [diff] [review]
patch for performance-icons.svg

Review of attachment 8861254 [details] [diff] [review]:
-----------------------------------------------------------------

Needs a commit message, and please remove the unused images.  Thanks! :)

::: devtools/client/jar.mn
@@ +122,5 @@
>      skin/images/pseudo-class.svg (themes/images/pseudo-class.svg)
>      skin/images/controls.png (themes/images/controls.png)
>      skin/images/controls@2x.png (themes/images/controls@2x.png)
>      skin/images/animation-fast-track.svg (themes/images/animation-fast-track.svg)
> +    skin/images/performance-overview-markers.svg (themes/images/performance-overview-markers.svg)

Hmm, it looks like these 2 overview images are unused?  We should probably just remove them then.
Attachment #8861254 - Flags: review?(jryans) → review+
Comment on attachment 8861253 [details] [diff] [review]
patch for select-arrow.svg

Review of attachment 8861253 [details] [diff] [review]:
-----------------------------------------------------------------

This one doesn't seem to have worked as expected.

STR:

1. Open some web site
2. Tools menu -> Web Developer -> Responsive Design Mode

ER:

The device selector, throttling selector, and DPR selector should have arrows next to them.

AR:

No arrows are shown for any of them.
Attachment #8861253 - Flags: review?(jryans) → review-
Created attachment 8861425 [details]
rdm-select-arrow.png

Here's an example of what the RDM select arrows should look like when working.
Comment on attachment 8861255 [details] [diff] [review]
patch for sort-arrows.svg

Review of attachment 8861255 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!  Please add a real commit message. :)
Attachment #8861255 - Flags: review?(jryans) → review+
:jwatt says I need a more up to date build with bug 1358690 for the first patch to work, so rebuilding now.
(Assignee)

Comment 11

10 months ago
Turns out the patch for select-arrow.svg doesn't work without the pref svg.context-properties.content.enabled flipped to true. I guess since select-arrow.svg isn't loaded via a chrome:// URL it doesn't count as chrome. I'll land the other two patches and work on figuring out how to enable context paint in this scenario without also enabling it for web content.

Comment 12

10 months ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31d8050e8e17
Split up DevTools' performance-icons.svg to improve performance. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e44294b9f5c
Split up DevTools' sort-arrows.svg to improve performance. r=jryans
(Assignee)

Updated

10 months ago
Keywords: leave-open
(Assignee)

Updated

10 months ago
Depends on: 1359486
Comment on attachment 8861253 [details] [diff] [review]
patch for select-arrow.svg

Review of attachment 8861253 [details] [diff] [review]:
-----------------------------------------------------------------

I can confirm it now works with the extra change from bug 1359486.  Thanks! :)

(Also needs real commit message.)
Attachment #8861253 - Flags: review- → review+

Comment 15

10 months ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/280c2dc0de7a
Convert DevTools' select-arrow.svg to use context paint to improve performance. r=jryans
(Assignee)

Updated

10 months ago
Keywords: leave-open

Comment 16

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/280c2dc0de7a
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.