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

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: alice0775, Assigned: Gijs)

Tracking

(Blocks 1 bug, {dev-doc-complete})

29 Branch
Firefox 31
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29+ verified, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3+])

Attachments

(2 attachments, 6 obsolete attachments)

Reporter

Description

5 years ago
Posted image 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.
Reporter

Comment 1

5 years ago
At lease, "Restore Defaults" should work.
Assignee

Comment 2

5 years ago
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.
Assignee

Comment 3

5 years ago
(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.
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.
Whiteboard: [Australis:P3+]
(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.
I'll see if I can crank this out this morning.
Assignee: nobody → 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.
Status: NEW → ASSIGNED
Posted patch Patch v1 (obsolete) — Splinter Review
Pretty simple - just force the mode attribute on construction.
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 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?
Attachment #8399457 - Flags: review?(jaws)
Attachment #8399456 - Flags: review?(jaws)
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.
Attachment #8399456 - Flags: review?(jaws)
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
Attachment #8399457 - Flags: review?(jaws) → review+
Comment on attachment 8399456 [details] [diff] [review]
Regression test - Patch v1

Cool, thanks Jared. Mind reviewing the little test too?
Attachment #8399456 - Flags: review?(jaws)
Posted patch Patch v1.1 (r+'d by jaws) (obsolete) — Splinter Review
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.
Attachment #8399457 - Attachment is obsolete: true
Attachment #8399508 - Flags: review+
Attachment #8399456 - Flags: review?(jaws) → review+
Folded and landed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/2f22e878159e
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/2f22e878159e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31
Assignee

Comment 18

5 years ago
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...
Flags: needinfo?(mconley)
Flags: needinfo?(jaws)
Yes, we should uplift it.
Flags: needinfo?(jaws)
Assignee

Comment 20

5 years ago
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
Attachment #8399508 - Flags: approval-mozilla-beta?
Attachment #8399508 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mconley)
Assignee

Comment 21

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

[Approval Request Comment]
See comment #20
Attachment #8399456 - Flags: approval-mozilla-beta?
Attachment #8399456 - Flags: approval-mozilla-aurora?
Attachment #8399508 - Flags: approval-mozilla-beta?
Attachment #8399508 - Flags: approval-mozilla-beta+
Attachment #8399508 - Flags: approval-mozilla-aurora?
Attachment #8399508 - Flags: approval-mozilla-aurora+
Attachment #8399456 - Flags: approval-mozilla-beta?
Attachment #8399456 - Flags: approval-mozilla-beta+
Attachment #8399456 - Flags: approval-mozilla-aurora?
Attachment #8399456 - Flags: approval-mozilla-aurora+
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]
Assignee

Comment 24

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 25

5 years ago
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?
Attachment #8404292 - Flags: review?(mconley)
Assignee

Updated

5 years ago
Assignee: mconley → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
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.
Attachment #8404292 - Flags: review?(mconley) → review-
Assignee

Comment 27

5 years ago
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?
Attachment #8404315 - Flags: review?(mconley)
Assignee

Updated

5 years ago
Attachment #8404292 - Attachment is obsolete: true
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);
  }
}
Attachment #8404315 - Flags: review?(mconley) → review+
Assignee

Comment 29

5 years ago
Plonk.
Attachment #8404887 - Flags: review?(mconley)
Assignee

Updated

5 years ago
Attachment #8404315 - Attachment is obsolete: true
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. :)
Attachment #8404887 - Flags: review?(mconley) → review+
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. :/
Assignee

Updated

5 years ago
Attachment #8404887 - Attachment is obsolete: true
Assignee

Comment 33

5 years ago
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.
Attachment #8405047 - Flags: review+
Attachment #8405047 - Flags: checkin?
Assignee

Updated

5 years ago
Keywords: dev-doc-needed
Keywords: verifyme
remote:   https://hg.mozilla.org/integration/fx-team/rev/794e7aec5d7a
Flags: in-testsuite+
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/794e7aec5d7a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
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.
Attachment #8405047 - Flags: checkin?
Attachment #8405047 - Flags: checkin+
Attachment #8405047 - Flags: approval-mozilla-beta?
Attachment #8405047 - Flags: approval-mozilla-aurora?
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.
Flags: in-testsuite+
Attachment #8405047 - Flags: approval-mozilla-beta?
Attachment #8405047 - Flags: approval-mozilla-beta+
Attachment #8405047 - Flags: approval-mozilla-aurora?
Attachment #8405047 - Flags: approval-mozilla-aurora+
Attachment #8399456 - Attachment is obsolete: true
Attachment #8399508 - Attachment is obsolete: true
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.
You need to log in before you can comment on or make changes to this bug.