989289 - Australis: Toolbar seems broken after launch Firefox28 -> Firefox29 -> Firefox28(Icons and Text) -> Firefox29, And "Restore Defaults" does not fix the broken

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Attachment: screenshot

Steps to Reproduce:
1. Start Firefox29

2. Start Firefox28 with same profile
3. Select "Icons and text" in Toolbar Customization
4. Exit Browser

5. Start Firefox29 with same profile
   ---- Observe how toolbar broken

6. "Restore Defaults" in in Toolbar Customization
   ---- Observe how toolbar broken

Actual Results:
  Toolbar seems broken.
  "Restore Defaults" does not fix the broken.

Expected Results:
  Toolbar should be Icon only.
  "Restore Defaults" should reset icon mode too.

Comment 1

[Alice0775 White]

At lease, "Restore Defaults" should work.

Comment 2

[:Gijs (he/him)]

This is because of how our migration code works. It's always been like that and its effects can in some cases be felt in different ways as well. I don't think we really want to fix this right now.

Comment 3

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #1)
> At lease, "Restore Defaults" should work.

I disagree. Making restore defaults do that kind of work for what is an edge case of an edge case doesn't seem worth it to me.

Comment 4

[Justin Dolske [:Dolske]]

The upgrade -> downgrade -> upgrade scenario is not terribly uncommon (especially with a big UI change), and we've worked to support it in the past. Although the definition of "support" can vary -- I certainly wouldn't spend time making, say, the "icons&text" preference be retained upon downgrade (assuming it isn't, but that's not really relevant here). But getting into a broken state that can't easily be fixed is not swell.

We could make "Restore Defaults" take care of this, if there's no feasible alternative, but I think the right fix should really be to make this not happen. Can we make 29 just always forcibly clear this, or make it harder to accidentally show labels?

I'd actually make this P2 but for the fact that the user needs to reenable icons+text.

Would reconsider needing a fix if this is too difficult / risky.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Justin Dolske [:Dolske] from comment #4)
> The upgrade -> downgrade -> upgrade scenario is not terribly uncommon
> (especially with a big UI change), and we've worked to support it in the
> past. Although the definition of "support" can vary -- I certainly wouldn't
> spend time making, say, the "icons&text" preference be retained upon
> downgrade (assuming it isn't, but that's not really relevant here). But
> getting into a broken state that can't easily be fixed is not swell.
> 
> We could make "Restore Defaults" take care of this, if there's no feasible
> alternative, but I think the right fix should really be to make this not
> happen. Can we make 29 just always forcibly clear this, or make it harder to
> accidentally show labels?
> 

Forcibly clearing sounds possible. We could get the toolbar binding to remove that attribute on construction, or CustomizableUI to remove it on toolbar registration.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I'll see if I can crank this out this morning.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

The good news is that, at least for Classic Theme Restorer, wiping out the mode="full" attribute from the toolbars will not affect its text+icons support.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test - Patch v1

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Pretty simple - just force the mode attribute on construction.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

In our add-on compat document, we wrote:

"Support for small icons mode as well as text and icons mode have been removed."

I _think_ that means we can force this attribute without anybody claiming it as a surprise.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8399457 [details] [diff] [review]
Patch v1

How's my thinking on this? Do we have add-on compat issues to think about here, or am I right in thinking that we're OK to force this attribute value?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8399456 [details] [diff] [review]
Regression test - Patch v1

Actually, y'know what, I'm going to withhold asking for review on this until I'm certain this is the right approach.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8399457 [details] [diff] [review]
Patch v1

Review of attachment 8399457 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message should be changed to say "Bug 989282 - Forcibly set the 'mode' attribute to 'icons' on toolbar contruction. r=jaws'

::: browser/components/customizableui/content/toolbar.xml
@@ +24,5 @@
>              this.addEventListener("overflow", this);
>              this.addEventListener("underflow", this);
>            }
>  
> +          this.setAttribute("mode", "icons");

Please move this to _init

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8399456 [details] [diff] [review]
Regression test - Patch v1

Cool, thanks Jared. Mind reviewing the little test too?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1 (r+'d by jaws)

Addressed feedback, updated comment, and fixed the bug number while I was at it. 

Also added a small bit of documentation to accompany the change.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Folded and landed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/2f22e878159e

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f22e878159e

Comment 18

[:Gijs (he/him)]

Mike/Jared, do we want to uplift this, too? I'd actually argue that because it'll most affect 29, it's quite important that we get it there...

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Yes, we should uplift it.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8399508 [details] [diff] [review]
Patch v1.1 (r+'d by jaws)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / upgrade/downgrades
User impact if declined: after certain upgrade/downgrade sequences, the toolbar layout might be (unrecoverably) messed up
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 21

[:Gijs (he/him)]

Comment on attachment 8399456 [details] [diff] [review]
Regression test - Patch v1

[Approval Request Comment]
See comment #20

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/45c0ceb1180e

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/85d2c5b844bc

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Per our IRC conversation, I had to skip the newly-added test on Linux debug due to the permaleaksit perturbed mochitest-bc-1 into hitting.
https://hg.mozilla.org/releases/mozilla-aurora/rev/4f6268f94b1d

The leak:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37477352&tree=Mozilla-Aurora

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_635418.js | leaked until shutdown [nsGlobalWindow #7109 chrome://browser/content/tabview.html]
TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_635418.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/tabview.html]

Comment 24

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/1244d500650c


Backed out of beta per IRC discussion with mconley because the current fix would also force the icons attribute on add-on toolbars.

Comment 25

[:Gijs (he/him)]

Attachment: use nsBrowserGlue to migrate users,

I'm using the pref as a proxy for 'has this profile been on Australis before'. I believe this works because we remove this pref in pre-Australis versions like 28: https://mxr.mozilla.org/mozilla-release/source/browser/components/nsBrowserGlue.js#1291 using very similar code. The only unfortunate thing is that if you have a pristine non-customized-at-all profile, the pref won't get set at all, and so the code needlessly runs. That's suboptimal, but I don't see a way around it, and it doesn't seem any worse than the code that ran before (on every toolbar creation). Mike, does this make sense?

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8404292 [details] [diff] [review]
use nsBrowserGlue to migrate users,

Review of attachment 8404292 [details] [diff] [review]:
-----------------------------------------------------------------

Tentative r- because I don't think this takes care of all cases.

::: browser/components/nsBrowserGlue.js
@@ +1316,5 @@
>    _migrateUI: function BG__migrateUI() {
>      const UI_VERSION = 22;
>      const BROWSER_DOCURL = "chrome://browser/content/browser.xul#";
>      let currentUIVersion = 0;
> +    let alreadyOnAustralis = Services.prefs.prefHasUserValue("browser.uiCustomization.state");

I don't think we can rely on this. In the event that the user has gone from 28, to 29, to 27, back to 29, they _will_ have a browser.uiCustomization.state, and we're back to square one.

I'm starting to wonder if perhaps a migration isn't the right way of doing this - but making it so that Resetting Defaults removes those old attributes is the better way forward.

Comment 27

[:Gijs (he/him)]

Attachment: only migrate builtin toolbars, also migrate toolbox,

OK, how is this? I included the toolbox because without that, going back to 28 confuses our dialogs because they look at the toolbox properties and assume they're always in sync with the toolbars. I imagine some addons might also check for iconsize/mode on the toolbox rather than the toolbar, and I figured... better safe than sorry?

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8404315 [details] [diff] [review]
only migrate builtin toolbars, also migrate toolbox,

Review of attachment 8404315 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I have a few requests:

1) Please file a bug to consider exposing the list of default toolbars through CustomizableUI
2) Please consider my simplification of the setAttribute thing you do

Thanks Gijs!

::: browser/components/customizableui/content/toolbar.xml
@@ +14,5 @@
>      </resources>
>      <implementation>
>        <field name="overflowedDuringConstruction">null</field>
> +      <field name="_builtinToolbars" readonly="true">
> +        new Set(["nav-bar", "PersonalToolbar", "addon-bar", "TabsToolbar", "toolbar-menubar"])

:(

I wonder if down the line we should try to expose this set through CustomizableUI instead. This is probably fine for now though.

@@ +51,5 @@
>            // Bug 989289: Forcibly set the now unsupported "mode" attribute, just
>            // in case it gets accidentally restored from persistence from a user
>            // that's been upgrading and downgrading.
> +          if (this._builtinToolbars.has(this.id)) {
> +            if (this.getAttribute("mode") != "icons") {

We can probably make this a bit less wordy by having something like:

const kDefaultAttributes = {
  "mode": "icons",
  "iconsize": "small",
}

for (let attr in kDefaultAttributes) {
  let value = kDefaultAttributes[value]; // Or use an Iterator, or whatever.
  if (this.getAttribute(attr) != value) {
    this.setAttribute(attr, value);
    document.persist(this.id, attr);
  }
  if (this.toolbox && this.toolbox.getAttribute(attr) != value) {
    this.toolbox.setAttribute(attr, value);
    document.persist(this.toolbox.id, attr);
  }
}

Comment 29

[:Gijs (he/him)]

Attachment: only migrate builtin toolbars, also migrate toolbox,

Plonk.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8404887 [details] [diff] [review]
only migrate builtin toolbars, also migrate toolbox,

Review of attachment 8404887 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with my nits fixed, and let's not forget to dev-doc-needed for the new method for CustomizableUI.

Great work!

::: browser/components/customizableui/content/toolbar.xml
@@ +13,5 @@
>        <stylesheet src="chrome://global/skin/toolbar.css"/>
>      </resources>
>      <implementation>
>        <field name="overflowedDuringConstruction">null</field>
> +      <field name="_builtinToolbars" readonly="true">

This can go now.

@@ +51,5 @@
> +          // Bug 989289: Forcibly set the now unsupported "mode" and "iconsize"
> +          // attributes, just in case they accidentally get restored from
> +          // persistence from a user that's been upgrading and downgrading.
> +          if (CustomizableUI.isBuiltinToolbar(this.id)) {
> +            const attributes = new Map([["mode", "icons"], ["iconsize", "small"]]);

Nit: kAttributes or ATTRIBUTES. I prefer the former, because then it doesn't look like I'm YELLING. :)

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8404887 [details] [diff] [review]
only migrate builtin toolbars, also migrate toolbox,

Review of attachment 8404887 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +245,5 @@
>      }, true);
>    },
>  
> +  get _builtinToolbars() {
> +    return new Set([

It's too bad we can't make the returned set read-only... that way, we wouldn't have to recreate it each time. :/

Comment 32

[:Gijs (he/him)]

Attachment: only migrate builtin toolbars, also migrate toolbox,

Comment 33

[:Gijs (he/him)]

Comment on attachment 8405047 [details] [diff] [review]
only migrate builtin toolbars, also migrate toolbox,

Carrying over r+, hoping someone has a chance to land this between now and tomorrow.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/794e7aec5d7a

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/794e7aec5d7a

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8405047 [details] [diff] [review]
only migrate builtin toolbars, also migrate toolbox,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

The original patch for this bug, believe it or not. The original patch was already backed out of beta.


User impact if declined: 

Users who use add-ons that inject toolbars with different modes and iconsizes will be forced to have mode="icons" and iconsize="small". This patch makes it so that we only force that for built-in toolbars.


Testing completed (on m-c, etc.): 

A few days to bake on m-c.


Risk to taking this patch (and alternatives if risky): 

Low risk.


String or IDL/UUID changes made by this patch:

None.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

The automated test provided with the original patch has been excised, since we didn't think it was worth it to try to fake out one of the built-in toolbars.

Comment 38

[:Gijs (he/him)]

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm$compare?to=549831&from=540831

Comment 39

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc14179fc49c

Comment 40

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e946bc71ae2b

Comment 41

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using old Nightly build and verified that the issue is fixed on Firefox 29 beta 8, latest Aurora and latest Nightly.