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

NEW
Unassigned

Status

()

Firefox
Theme
P3
normal
7 months ago
25 days ago

People

(Reporter: jwatt, Unassigned)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {meta, perf})

unspecified
meta, perf
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-performance])

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

Updated

7 months ago
Keywords: perf
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.
Flags: needinfo?(jwatt)
(Reporter)

Comment 2

7 months ago
(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.
Depends on: 1337492
Flags: needinfo?(jwatt)
Blocks: 1348289
Summary: Stop using the :not(:target) selector in SVG for the Firefox UI → [meta] Stop using the :not(:target) selector in SVG for the Firefox UI

Updated

7 months ago
Flags: qe-verify-
Keywords: meta
Whiteboard: [photon-performance]
(Reporter)

Updated

7 months ago
Depends on: 1359273
(Reporter)

Updated

7 months ago
Depends on: 1359631
Comment hidden (spam)
Comment hidden (spam)

Comment 5

7 months ago
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.
(Reporter)

Updated

7 months ago
Depends on: 1362890
(Reporter)

Updated

7 months ago
Depends on: 1363075
Blocks: 1363777
No longer blocks: 1348289

Comment 6

6 months ago
I'm fixing caption-buttons.svg in bug 1367384.
Depends on: 1367384
(Reporter)

Updated

6 months ago
Depends on: 1368411
(Reporter)

Updated

6 months ago
Depends on: 1368414
(Reporter)

Updated

6 months ago
Depends on: 1368417
(Reporter)

Updated

6 months ago
Depends on: 1368425
(Reporter)

Updated

6 months ago
Depends on: 1368428
Depends on: 1371230
(Reporter)

Updated

5 months ago
Depends on: 1374099
(Reporter)

Updated

5 months ago
Depends on: 1374101
(Reporter)

Updated

5 months ago
Depends on: 1374104
(Reporter)

Updated

5 months ago
Depends on: 1374105
(Reporter)

Updated

5 months ago
Depends on: 1374106
(Reporter)

Updated

5 months ago
Depends on: 1374107
(Reporter)

Updated

5 months ago
Depends on: 1374108

Comment 7

5 months ago
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) {

Updated

3 months ago
Depends on: 1394609

Updated

a month ago
Priority: -- → P3
Depends on: 1411606
You need to log in before you can comment on or make changes to this bug.