data-identity-color CSS selectors are inefficient

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
10 months ago
a month ago

People

(Reporter: florian, Assigned: jkt, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed, firefox68 fixed)

Details

(Whiteboard: [domsecurity-backlog2])

Attachments

(1 attachment)

Reporter

Description

10 months ago
Mentioned in bug 1260036 comment 15 and 19:

(Emilio Cobos Álvarez (:emilio) from bug 1260036 comment #19)
> (In reply to Florian Quèze [:florian] from comment #18)
> > (In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

> > > Or all the [data-identity-color] attributes:
> > > 
> > >  
> > > https://searchfox.org/mozilla-central/rev/
> > > d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/
> > > contextualidentity/content/usercontext.css
> > > 
> > > We may want to be more granular at that, but that may very well end up being
> > > a performance regression for elements with a lot of attributes, so it's more
> > > unclear it's worth doing.
> > 
> > Is it bad enough that it would make sense to add a class to all the
> > attributes that can ever have this data-identity-color attribute? Would this
> > help?
> 
> Whenever an attribute changes anywhere in the page we go through these,
> yeah. Probably not terrible, since we avoid looking for attribute changes
> whose attribute doesn't appear in any selector, but it may add up. Classes
> are definitely faster.
Priority: -- → P3
Whiteboard: [domsecurity-backlog2]
Jonathan, could this be a good first bug? Would you be interested in mentoring this?
Flags: needinfo?(jkt)
Assignee

Comment 2

10 months ago
Yeah and I would be happy to mentor too :)
Flags: needinfo?(jkt)
Keywords: good-first-bug
Mentor: jkt

Comment 3

3 months ago

Hi. I want to work on this bug. Can someone please help me reproducing it?

I think the idea is replacing something like .setAttribute("data-identity-color", identity.color); with .className = "identity-color-" + identity.color;

But I'll leave the rest to jkt :)

Flags: needinfo?(jkt)
Assignee

Comment 5

3 months ago

So the idea is that we currently use data attributes for the CSS here: https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/browser/components/contextualidentity/content/usercontext.css#1-39

We should replace these instead with classes, then we need to replace all the ways we set those data attributes:
https://searchfox.org/mozilla-central/search?q=%22data-identity-color%22&case=false&regexp=false&path=

To reproduce if you weren't aware you need to install a container addon or use Firefox Nightly to get containers on the browser you are working on. You then will need to follow the build steps to modify firefox source please reach out to me if you get stuck or manage to make some progress.

Flags: needinfo?(jkt) → needinfo?(khyati.agarwalss)

Comment 6

3 months ago

I have installed and built Firefox Nightly (non-artifact mode) on my laptop. Please guide me what to do next.

Flags: needinfo?(khyati.agarwalss)
Assignee

Comment 7

3 months ago

So from there you will see the code I linked to in 'searchfox' exists in your directory too. Try changing the colours in the file browser/components/contextualidentity/content/usercontext.css for example and then try running firefox again.

Also look at the Browser Toolbox which allows you to debug the interface with the inspector much like the normal inspector for web pages. You can edit browser CSS and watch the result without running Firefox again.

So the task then is to take the existing CSS and replace it with classes rather than attribute selectors. This requires manipulating the CSS file and also the JS that sets the attributes, this can be changes just to set classes.

Flags: needinfo?(khyati.agarwalss)

Comment 8

3 months ago

I am done experimenting with 'searchfox' and Browser Toolbox. Moving towards the next step of replacing attribute selectors with classes. Is "usercontext.css" the only file which need to be changed?

Flags: needinfo?(khyati.agarwalss) → needinfo?(jkt)
Assignee

Comment 9

3 months ago

So usercontext.css is the only CSS file that is needed to be changed. The patch will also need to change the setting of the attribute selectors.

If you look at the searchfox result here you can see there are a few places we are setting this in the JavaScript.

So as :johannh said in Comment 4 the task is to use .className = "identity-color-" + identity.color;

However one thing to note is that we need to keep the data attribute to store what the current colour is as the current code tracks the state of the current colour through the data attribute. So clearing the colour currently is just a case of removing the attribute, however moving this to classes the code either needs to understand the previous colour to clear it or loop through the colour list (looping through all the colours seems a bad idea).

So a solution like the folloing will need to be used:

let classPrefix = "identity-color-";
let currentColor = el.getAttribute("data-identity-color");
el.classList.removeClass(classPrefix + currentColor);
el.classList.addClass(classPrefix + newColor);
el.setAttribute("data-identity-color", newColor);
Flags: needinfo?(jkt) → needinfo?(khyati.agarwalss)
Assignee

Updated

3 months ago
Blocks: 1532746
Assignee

Updated

2 months ago
Assignee: nobody → jkt
Assignee

Updated

2 months ago
Flags: needinfo?(khyati.agarwalss)
Assignee

Comment 11

a month ago

Did you get chance to review again? (I'm not sure if I need to reflag you on phab somehow).

Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)

Comment 12

a month ago
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45e56d6e9dd0
Changing icons and colors for containers to be class based. r=dao

Comment 13

a month ago
bugherder
Status: NEW → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee

Comment 14

a month ago

Comment on attachment 9053644 [details]
Bug 1479433 - Changing icons and colors for containers to be class based.

Beta/Release Uplift Approval Request

  • User impact if declined: Firefox is a little slower
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1532746
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change only impacts CSS for the containers and has been verified in Nightly as working for 9 days. The changes in Bug 1532746 just add an icon but they depend on this CSS change. The impact to not landing that is the facebook container addon won't be able to use the correct icon.
  • String changes made/needed: N/A, Bug 1532746 has two container strings in it though "containers-icon-fence.label" and "containers-color-toolbar.label"
Attachment #9053644 - Flags: approval-mozilla-beta?

Comment on attachment 9053644 [details]
Bug 1479433 - Changing icons and colors for containers to be class based.

Looks reasonably safe and has baked on nightly for >1 week, uplift accepted for 67 beta 13, thanks.

Attachment #9053644 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.