Closed Bug 966599 Opened 7 years ago Closed 7 years ago

Restore Defaults should collapse non-default-visible toolbars

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch WIP Patch (obsolete) — Splinter Review
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.
Attachment #8370976 - Flags: feedback?(gijskruitbosch+bugs)
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
}
Attachment #8370976 - Flags: feedback?(gijskruitbosch+bugs)
Attached patch WIP Patch v2 (obsolete) — Splinter Review
Attachment #8370976 - Attachment is obsolete: true
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.
Attached patch WIP Patch v3 (obsolete) — Splinter Review
Pushed to try server, https://tbpl.mozilla.org/?tree=Try&rev=515353d33e2d
Attachment #8371085 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
This is green on try (pending 10.8bc), https://tbpl.mozilla.org/?tree=Try&rev=ff7fbac92eea
Attachment #8371828 - Attachment is obsolete: true
Attachment #8372088 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8372088 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Flags: in-testsuite+
Depends on: 969473
Attached patch Patch v2Splinter Review
Now with fixed MENUBAR_CAN_AUTOHIDE.
Attachment #8372088 - Attachment is obsolete: true
Attachment #8372504 - Flags: review?(gijskruitbosch+bugs)
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. :-(
Attachment #8372504 - Flags: review?(gijskruitbosch+bugs) → review+
Depends on: 969766
https://hg.mozilla.org/mozilla-central/rev/83accd4cace2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Any reason the 'title bar' option does not get reset?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
(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.
Blocks: 971626
(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
No longer blocks: 971626
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.
Attachment #8372504 - Flags: approval-mozilla-aurora?
Attachment #8372504 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 974819
Status: RESOLVED → VERIFIED
Depends on: 996364
You need to log in before you can comment on or make changes to this bug.