1358998 - [meta] Stop using the :not(:target) selector in SVG for the Firefox UI

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P3)

Description

[Jonathan Watt [:jwatt]]

I mentioned this already in bug 1337492 comment 6, but apparently I didn't file a bug.

Right now we are using :not(:target) a lot in the SVG that we use in the UI:

https://dxr.mozilla.org/mozilla-central/search?limit=10000&q=not(%3Atarget

I remember suggesting a long time ago that this way of grouping icons in a single file *could* perhaps be optimized to make improve performance over having them in individual files. (The idea was that we could optimize the implementation internally so that a single SVG document would be shared between different icons.) However, this optimization does not currently exist (and would require a fair bit of work).

What currently happens when we use this mechanism is that imagelib creates a new VectorImage (and therefore SVG document instance) for each URL with a different hash (see bug 1027106). So by using the |use:not(:target)| trick we end up creating a new document instance for each icon in the file that is displayed, PLUS each document instance has to pay the parsing, DOM construction, styling, etc. overhead for all the other icons in the combined file. In other words there is only downside to this approach over having each icon in its own file.

I'd strongly suggest switching to individual files, or else using the -moz-image-rect/-moz-image-region mechanism we use elsewhere.

Comment 1

[Johann Hofmann [:johannh]]

Thanks! That doesn't sound fun but it looks like we might want do it as part of Photon. :)

- Isn't this a dupe of bug 1337492?

- Shouldn't we turn this into a meta-bug and fix occurrences in individual bugs? We could use bug 1337492 to fix the identity icon (I'm happy to do that myself). It's really noticeable there.

Comment 2

[Jonathan Watt [:jwatt]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> - Isn't this a dupe of bug 1337492?

That bug would seem to be about a specific set of icons. There are lots more though.

> - Shouldn't we turn this into a meta-bug and fix occurrences in individual
> bugs? We could use bug 1337492 to fix the identity icon (I'm happy to do
> that myself).

In my head this was already a meta bug, and I was leaving it up to you guys to decide how you want to split (or not) the work that would fix this.

Comment 5

[Tim Nguyen :ntim]

It might be worth updating https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines

to include this (and new stuff like context-fill) as well.

Comment 6

[Dão Gottwald [:dao]]

I'm fixing caption-buttons.svg in bug 1367384.

Comment 7

[Dão Gottwald [:dao]]

remaining cases:

browser//extensions/pocket/content/panels/img/pocket.svg:7:    use:not(:target) {
browser//themes/shared/controlcenter/connection.svg:9:    svg > rect:not(:target) {
browser//themes/shared/controlcenter/tracking-protection.svg:9:    g:not(:target) {
browser//themes/shared/drm-icon.svg:16:    use:not(:target) {
browser//themes/shared/identity-block/tracking-protection-16.svg:8:    g:not(:target) {
browser//themes/shared/incontentprefs/icons.svg:7:    use:not(:target) {
browser//themes/shared/incontentprefs-old/icons.svg:7:    use:not(:target) {
browser//themes/shared/notification-icons.svg:7:    :root > use:not(:target),
browser//themes/shared/notification-icons.svg:8:    :root > g:not(:target),
browser//themes/shared/tabbrowser/tab-audio-small.svg:7:    .icon:not(:target) {
toolkit//themes/osx/global/tree/arrow-disclosure.svg:7:    .icon:not(:target) {
toolkit//themes/windows/global/tree/twisty-preWin10.svg:7:    use:not(:target) {
toolkit//themes/windows/global/tree/twisty.svg:7:    use:not(:target) {

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I think we can close out this metabug, since the dependencies are all closed. Sound okay to you, cmkm?

Comment 10

[Tim Nguyen :ntim]

https://searchfox.org/mozilla-central/search?q=%3Anot%28%3Atarget%29&path=&case=false&regexp=false

Comment 11

[Cieara Meador [:cmkm]]

Confirmed with the themes group this can be closed out.