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

NEW
Unassigned

Status

()

enhancement
P3
normal
2 years ago
10 months ago

People

(Reporter: jwatt, Unassigned)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-performance] [fxperf])

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.
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)
(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)
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
Flags: qe-verify-
Keywords: meta
Whiteboard: [photon-performance]
Depends on: 1359273
Depends on: 1359631
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.
Depends on: 1362890
Depends on: 1363075
I'm fixing caption-buttons.svg in bug 1367384.
Depends on: 1367384
Depends on: 1368411
Depends on: 1368414
Depends on: 1368417
Depends on: 1368425
Depends on: 1368428
Depends on: 1371230
Depends on: 1374099
Depends on: 1374101
Depends on: 1374104
Depends on: 1374105
Depends on: 1374106
Depends on: 1374107
Depends on: 1374108
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) {
Depends on: 1394609
Priority: -- → P3
Depends on: 1411606
Whiteboard: [photon-performance] → [photon-performance] [fxperf]
Depends on: 1455138
Depends on: 1456639
Depends on: 1466826
Depends on: 1488617
No longer depends on: 1456639
You need to log in before you can comment on or make changes to this bug.