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

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Jonathan Watt [:jwatt]]

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

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch for select-arrow.svg

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: 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.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch for sort-arrows.svg

Comment 4

[Jonathan Watt [:jwatt]]

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.

Comment 5

[Jonathan Watt [:jwatt]]

(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.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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/[e-mail] (themes/images/[e-mail])
>      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.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: rdm-select-arrow.png

Here's an example of what the RDM select arrows should look like when working.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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. :)

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:jwatt says I need a more up to date build with bug 1358690 for the first patch to work, so rebuilding now.

Comment 11

[Jonathan Watt [:jwatt]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.)

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31d8050e8e17
https://hg.mozilla.org/mozilla-central/rev/2e44294b9f5c

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/280c2dc0de7a
Convert DevTools' select-arrow.svg to use context paint to improve performance. r=jryans

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/280c2dc0de7a