Edit and zoom controls don't go back to the menupanel after a reset

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M7])

Attachments

(1 attachment, 1 obsolete attachment)

2.87 KB, patch
mconley
: review+
Unfocused
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
STR:

1. Customize and move edit + zoom controls to the navbar
2. Close customize, reopen customize
3. Click restore defaults
4. Close customization mode.
5. Close and reopen the browser (alternatively, look at about:config and notice the pref got changed again / didn't get reset?)

Expected result:
Everything looks like the default

Actual result:

My zoom/edit buttons are in the navbar again.



(If you need to get out of this state to re-test, use about:config to reset the customization pref)


Mike, any idea why this is happening? I haven't seen this with any other controls...
(Assignee)

Comment 1

5 years ago
Created attachment 762583 [details] [diff] [review]
Patch

Turns out this was pretty trivial to fix.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #762583 - Flags: review?(mdeboer)
Comment on attachment 762583 [details] [diff] [review]
Patch

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

Trivial indeed! :)
Attachment #762583 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 3

5 years ago
Pushed: https://hg.mozilla.org/projects/ux/rev/a462ae7a8657
Whiteboard: [Australis:M?] → [Australis:M7][fixed-in-ux]
(Assignee)

Comment 4

5 years ago
Backed out for test failures, https://hg.mozilla.org/projects/ux/rev/833523565041 . Not as trivial as I thought!
Whiteboard: [Australis:M7][fixed-in-ux] → [Australis:M7]
(Assignee)

Updated

5 years ago
Attachment #762583 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 762774 [details] [diff] [review]
Patch

Soooo this also works and doesn't break tests. However, I'm somewhat inclined to still also add the removable attribute to the node either in the edit/zoom controls themselves and/or after onBuild runs in buildWidget, mostly so we can select for it with CSS and use that to give visual feedback about items which aren't removable. Whatever we do, though, the widget property should stay...

How do you feel about this, Mike?
Attachment #762774 - Flags: review?(mconley)
Comment on attachment 762774 [details] [diff] [review]
Patch

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

In theory this is fine (so long as that comment I noted is corrected), but I think I'd want Blair's thumbs-up on this, since he did the removable stuff.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +299,5 @@
>        if (currentNode && currentNode.id == id) {
>          this._addParentFlex(currentNode);
>          this.setLocationAttributes(currentNode, container, aArea);
>  
>          // Normalize removable attribute. It defaults to false if the widget is

"It defaults to false if the widget is originally defined as a child of a build area."

I don't think that's true anymore. I'm not sure if there's a good reason for those to default to false...maybe Blair knows?
Attachment #762774 - Flags: review?(mconley)
Attachment #762774 - Flags: review?(bmcbride)
Attachment #762774 - Flags: review+
(In reply to Mike Conley (:mconley) from comment #6)
> "It defaults to false if the widget is originally defined as a child of a
> build area."
> 
> I don't think that's true anymore. I'm not sure if there's a good reason for
> those to default to false...maybe Blair knows?

It is indeed still true. You're thinking of bug 875809, which was WONTFIXed as it would break backwards compatibility.
Attachment #762774 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 8

5 years ago
Pushed: https://hg.mozilla.org/projects/ux/rev/d326eb8c292c

Thanks!
Whiteboard: [Australis:M7] → [Australis:M7][fixed-in-ux]
(Assignee)

Comment 9

5 years ago
Filed bug 884204 about making this more sane.
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d326eb8c292c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M7][fixed-in-ux] → [Australis:M7]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.