1479433 - data-identity-color CSS selectors are inefficient

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Johann Hofmann [:johannh]]

Jonathan, could this be a good first bug? Would you be interested in mentoring this?

Comment 2

[Jonathan Kingston [:jkt] he/him]

Yeah and I would be happy to mentor too :)

Comment 3

[khyati.agarwalss]

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

Comment 4

[Johann Hofmann [:johannh]]

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

Comment 5

[Jonathan Kingston [:jkt] he/him]

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.

Comment 6

[khyati.agarwalss]

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

Comment 7

[Jonathan Kingston [:jkt] he/him]

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.

Comment 8

[khyati.agarwalss]

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?

Comment 9

[Jonathan Kingston [:jkt] he/him]

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

Comment 10

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1479433 - Changing icons and colors for containers to be class based.

Comment 11

[Jonathan Kingston [:jkt] he/him]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45e56d6e9dd0
Changing icons and colors for containers to be class based. r=dao

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/45e56d6e9dd0

Comment 14

[Jonathan Kingston [:jkt] he/him]

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"

Comment 15

[Pascal Chevrel:pascalc]

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.

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12d959489400