966599 - Restore Defaults should collapse non-default-visible toolbars

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

STR:
Enter customization mode
Show the bookmarks toolbar
Click on the Restore Defaults button

ER:
The bookmarks toolbar is hidden

AR:
The bookmarks toolbar remains visible

It doesn't make sense that "restore default" only restores some of the visual state of Firefox to its out-of-box state, but doesn't restore toolbar visibility.

Comment 1

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

Attachment: WIP Patch

When running the accompanied tests with this patch, there is an exception with aWrapper being null in unwrapToolbarItem that is coming from the PersonalToolbar's Bookmark Toolbar Items' wrapper having a null parentNode. I suspect this has something to do with the binding changing, but maybe something can be changed about the ordering of this code to avoid this.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8370976 [details] [diff] [review]
WIP Patch

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

Some stuff below. Checking what's up now.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +185,5 @@
>          "downloads-button",
>          "home-button",
>          "social-share-button",
> +      ],
> +      defaultVisible: true,

Nit: you should document this value (both in the JSM and on MDN's doc for CustomizableUI.jsm) and pick a sensible default value for it (probably false).

@@ +2187,5 @@
>              return itemNode && removableOrDefault(itemNode || item);
>            });
>          }
> +
> +        let autohide = container.getAttribute("autohide") == "true";

Sooooooo....

let attribute = (areaId == AREA_MENUBAR) ? "autohide" : "collapsed";
let defaultVisible = "" + props.get("defaultVisible");
if (props.get("type") == CustomizableUI.TYPE_TOOLBAR &&
    container.getAttribute(attribute) == defaultVisible) {
 // whatever
}

Comment 3

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

Attachment: WIP Patch v2

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8371085 [details] [diff] [review]
WIP Patch v2

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +196,5 @@
>          "menubar-items",
> +      ],
> +      get defaultVisible() {
> +        return Services.appinfo.OS == "WINNT" &&
> +               Services.sysinfo.getProperty("version") == "5.1";

I don't like that we're duplicating this logic from browser/base/jar.mn for win6BrowserOverlay.xul. You should at least add a comment about where this came from and it would be nice if it mentioned "win6BrowserOverlay.xul" so searches to remove that in the future would find this code.

I had hoped we don't need the new defaultVisible properties at all as the browser has the data we need already, it's just matter of getting access to it. Aren't we still using XUL persistence, so can we remove the persisted value and somehow figure out the default value? It's in the document at startup. Maybe we don't have APIs for that? Just a thought to reduce duplication.

@@ +2064,5 @@
> +            hidden = areaNode.getAttribute("autohide") == "false" &&
> +                     area.get("defaultVisible") == false;
> +            areaNode.setAttribute("autohide", !area.get("defaultVisible"));
> +          } else {
> +            areaNode.collapsed = !area.get("defaultVisible");

Nit: if we called the property "defaultCollapsed" it would align with the attribute better and be easier to find in search related to collapsed toolbars.

Comment 5

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

Attachment: WIP Patch v3

Pushed to try server, https://tbpl.mozilla.org/?tree=Try&rev=515353d33e2d

Comment 6

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

Attachment: Patch

This is green on try (pending 10.8bc), https://tbpl.mozilla.org/?tree=Try&rev=ff7fbac92eea

Comment 7

[:Gijs (he/him)]

Comment on attachment 8372088 [details] [diff] [review]
Patch

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

r=me but please note the notes, especially about add-ons and the async-ness of .reset().

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +291,5 @@
>        props.set("type", CustomizableUI.TYPE_TOOLBAR);
>      }
> +    if (props.get("type") == CustomizableUI.TYPE_TOOLBAR) {
> +      if (!props.has("defaultCollapsed")) {
> +        props.set("defaultCollapsed", true);

Now add-ons can set this. Do we want to allow that? I thought you said we didn't?

::: browser/components/customizableui/test/browser_877178_unregisterArea.js
@@ +43,3 @@
>    removeCustomToolbars(); // Will call unregisterArea for us
>    ok(CustomizableUI.inDefaultState, "When the toolbar is unregistered, " +
>       "everything should still be in a default state.");

Nit: update this message.

::: browser/components/customizableui/test/browser_938980_navbar_collapsed.js
@@ +34,5 @@
> +  is(navbar.getBoundingClientRect().height, 1, "navbar should have a height=1 (due to border)");
> +  is(CustomizableUI.inDefaultState, false, "Should no longer be in default state");
> +
> +  yield startCustomizing();
> +  gCustomizeMode.reset();

Please can you file a followup bug for making this (and if necessary the CUI equivalent) return a promise for when it finishes, and update the docs to make it clear which of them are async?

Comment 8

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

Pushed to fx-team,
https://hg.mozilla.org/integration/fx-team/rev/0a8f966b8332

Filed bug 969443 to update the CustomizableUI tests to yield on gCustomizeMode.reset instead of gCustomizeMode.resetting.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/6330fb0cb998

https://tbpl.mozilla.org/php/getParsedLog.php?id=34301900&tree=Fx-Team

Comment 10

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

Attachment: Patch v2

Now with fixed MENUBAR_CAN_AUTOHIDE.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8372504 [details] [diff] [review]
Patch v2

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

This looks nice! Please test on both Linux and Windows if all CUI tests + the social API test that failed before, pass.

Assuming they do (which is what I'd expect), r=me

PS: don't forget to update CustomizableUI.jsm documentation on MDN (yes, we have to sync things manually. :-(

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/83accd4cace2

Comment 13

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

Updated the documentation on MDN, https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm$revision/520565

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/83accd4cace2

Comment 15

[Peter Retzer (:pretzer)]

Any reason the 'title bar' option does not get reset?

Comment 16

[:Gijs (he/him)]

(In reply to Peter Retzer (:pretzer) from comment #15)
> Any reason the 'title bar' option does not get reset?

It isn't a toolbar. You could file a separate bug, but I seriously don't think it's something we should work on right now, considering the rest of our backlog.

Comment 17

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

(In reply to Peter Retzer (:pretzer) from comment #15)
> Any reason the 'title bar' option does not get reset?

Yes, just out of scope for this bug. I wish I had thought of it when writing this patch though. A new bug will be appreciated.

Comment 18

[Peter Retzer (:pretzer)]

(In reply to Jared Wein [:jaws] from comment #17)
> Yes, just out of scope for this bug. I wish I had thought of it when writing
> this patch though. A new bug will be appreciated.

Done. Bug 971626

Comment 19

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

Comment on attachment 8372504 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature for Australis
User impact if declined: restore defaults doesn't fulfill what it claims to do
Testing completed (on m-c, etc.): on m-c for a while, including tests
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

note: needs to land with the patch for bug 969766.

Comment 20

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

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