1545399 - "An extension, [...], requires Container Tabs." text will hide itself after brief showing in General tab in Options after landing patch from bug #1534480

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: bug.png

STR:

1. Install Cookie Autodelete extension ( https://addons.mozilla.org/addon/cookie-autodelete/ )
2. Open Firefox Options ( about:preferences )
3. Go to "General" tab ( about:preferences#general )
   and see that "An extension, Cookie AutoDelete, requires Container Tabs." text will hide itself after brief showing

Regression caused by:
Commit message: bug #1534480 - Do not update Fluent DOM attributes if they match current ones. r=smaug
Differential Revision: https://phabricator.services.mozilla.com/D23063

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: no bug.png

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you! Taking.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1545399 - Update the Preferences' setControllingExtensionDescription to only alter the DOM when needed. r?jaws

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ooo, that's an exciting problem on multiple levels!

Summary: The code, written by me during the Preferences+Fluent rewrite, was freely playing with what we now understand to be the skeleton (source) for DOM Overlays and assuming that setting the l10n id/args pair will trigger a recreation of the fragment. The crux was that we were "recreating" the inner structure of the fragment many times, and after my patch from bug 1534480, we didn't trigger Fluent DOM Overlays retranslation becuase id/args didn't change.

This patch fixes that in a pretty reasonable, I believe, way by only altering the source structure if there's a reason.

But it opens up several interesting questions:

1. Gijs, jaws - why do we call this function, at startup, 6 times? Is that a sign of our code being not optimal? Is it worth trying to optimize it? Is there more perf issue because we call the same code 6 times at startup?

2. smaug, Pike, bgrins, jaws, gijs - do we want Fluent to optimize away retranslations? I feel like it goes against how DOM is supposed to work and if we want Fluent DOM to aim to become a Web Standard, we should try to stick close to its logic. If you set attribute multiple times, DOM will trigger multiple Mutations and multiple Custom Element callbacks. I can see why at Mozilla we'd like to avoid paying for suboptimal code, but it leads to issues like this one.

(1) may be worth a separate bug to investigate why are we calling so much repetitive code.
(2) may impact our design decisions for Fluent DOM Overlays rev. 3 (thus asking Pike and smaug), but also has impact on our CE work (bgrins) and Preferences (jaws and Gijs).

Comment 5

[:Gijs (he/him)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)

1. Gijs, jaws - why do we call this function, at startup, 6 times? Is that a sign of our code being not optimal? Is it worth trying to optimize it? Is there more perf issue because we call the same code 6 times at startup?

Are the arguments the same every time? From spending less than a minute looking at the code, it looks to me like we will call setControllingExtensionDescription for different things which can be extension-controlled, like search engine, home page, new tab page, proxy settings, containers, ...

If the arguments aren't different and you're intending to say we call this 6 times for the containers addon, can you copy and share the stacks?

I still don't understand how multiple calls with the same arguments have anything to do with this issue. Surely the issue is that initially, the contents of the node suggest there's an add-on, and we replace the information "at some later point" (presumably once we've talked to the addon manager or some other async API), and the desired behaviour is that we don't indicate either add-on- or non-add-on controlled state until we know either way.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Are the arguments the same every time?

Yes. The call is executed 7 times, once for a different panel, and then 6 times for privacy.containers with the same addon enabled and the same element.

Easy to test:

1. Install the test extension from bug 1534480 STR
2. Set console.log(arguments); at the top of setControllingExtensionDescription
3. Open Firefox Preferences.

I still don't understand how multiple calls with the same arguments have anything to do with this issue.

1. Call setControllingExtensionDescription(elem, {name: "Cookie Addon"}, "privacy.containers");
2. We remove all childNodes from elem, insert <img/> and call document.l10n.setAttributes(elem, "extension-controlled-privacy-containers", {name: addon.name}); - all works fine.
3. We call setControllingExtensionDescription(elem, {name: "Cookie Addon"}, "privacy.containers"); again.
4. We remove all childNodes from elem, insert <img/> and call document.l10n.setAttributes(elem, "extension-controlled-privacy-containers", {name: addon.name}); - here's what bug 1534480 impacts.

Now, with my patch in bug 1534480, in (4), we don't trigger translation again, because id/args don't change. So the element stays empty with <img/> child only and no translation.

Hope that explains what happens better and warrants my two questions.

Comment 7

[Brian Grinstead [:bgrins]]

The main issue that Bug 1534480 solved for us (especially in the context of the <label> Custom Element conversion in Bug 1448213) was that it removed the "removeAttribute" step in the process of a re-localization (see also https://github.com/projectfluent/fluent.js/issues/300).

So what used to happen is basically:

1. <initial localization> label.setAttribute("accessKey", "X");
2. <re-localization>
   2a) label.removeAttribute("accessKey")
   2b) label.setAttribute("accessKey", "X");

2a is the problem for Bug 1534480 because Custom Element <label> is more correct functionally than XBL <label>. The XBL <label> would format the accessKey only at (1) (or if you manually forced it as per https://searchfox.org/mozilla-central/rev/d302c3058330a57f238be4062fddea629311ce66/toolkit/content/customElements.js#387 / https://bugzilla.mozilla.org/show_bug.cgi?id=1532071).

The Custom Element <label> does an accessKey format at (1), then removes the accessKey formatting at (2a), then does another accessKey format at (2b). If (2a) was removed from the process we could easily (and my current patch already does) detect that it hasn't changed between (1) and (2b) and skip the redundant accessKey format.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you Brian! If :smaug and :pike agree, I'll file a bug to restore retranslation on id/args setting without over-optimizing it. But we'll maintain the fix in DOMOverlays to avoid removal of attributes if they're about to be set again.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: stack.txt

Attaching a stack per :Gijs request. There are 7 calls, the first one is for network-proxy-connection-description the next 6 are for privacy.container.

I verified that they all carry the same set of arguments.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

After looking at the stack Gijs pointed out that it's bug 1526274 about calling it 6 times.

In result I'm taking off NI from gijs/jaws, leaving it on smaug/pike to verify the proposal from bgrins and I about what Fluent DOM and DOMOverlays should do when the user requests retranslation with the same id/args.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Filed bug 1545954, will ask for r? from pike and smaug there.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/076077f53326
Update the Preferences' setControllingExtensionDescription to only alter the DOM when needed. r=jaws

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/076077f53326

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 68.0a1 (2019-04-21), so I'm marking this bug as VERIFIED.
Thank you very much! \o/