Closed Bug 900593 Opened 11 years ago Closed 11 years ago

Australis: Fix flex updates to remove flex when items get removed / remove flex setting

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P4][Australis:M9])

Attachments

(1 file)

When discussing bug 899576 with mconley, I realized we currently don't reduce the parent flex if we remove items that have flex from an area. Either that should be fixed, or, really preferably, we'd get rid of this flex-tracking-code. Jared, bug 860535 comment 2 said you talked about this, can you give some background about why it's necessary and whether we can get rid of it? Doing so would clean up code a little bit. Alternatively, we should probably ensure that removing nodes with flex removes the flex from the parent as well - right now removing the search box would leave the parent with too high a flex number.
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] from bug 899576 comment #19) > (In reply to Mike Conley (:mconley) from bug 899576 comment #10) > > > @@ -308,5 @@ > > > > > > > > let currentNode = container.firstChild; > > > > for (let id of aPlacements) { > > > > if (currentNode && currentNode.id == id) { > > > > - this._addParentFlex(currentNode); > > > > > > Why do/did we need to do this (and, presumably, not m-c)? > > > > Unsure! It's possible that the parent flex stuff we're doing is no longer > > necessary now that the customization target is not restricted to a little > > nubbin on the toolbar. I'd want to needinfo jaws to find out if the parent > > flex stuff is still required. > > The parent flex is needed because we want the customize-target to have the > same flex as it would have had prior to Australis, in case something else > gets placed in the toolbar and the author had set a flex relative to the > previous values. Setting the parent flex here would set the flex on the > customization-target, satisfying this invariant. So, I don't think we should worry about this case, as the customize target is now basically the entire toolbar minus the overflow button minus the panel menu button. We don't want anything to appear outside of the customize target anyway, so we should just clean up this code.
Attached patch Remove parent flex stuff, (deleted) — Splinter Review
Attachment #819654 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 819654 [details] [diff] [review] Remove parent flex stuff, Review of attachment 819654 [details] [diff] [review]: ----------------------------------------------------------------- This does mean though that any add-on that subverts our new API and does an appendChild on the #nav-bar will probably have too much flex assigned to it.
Attachment #819654 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] from comment #4) > Comment on attachment 819654 [details] [diff] [review] > Remove parent flex stuff, > > Review of attachment 819654 [details] [diff] [review]: > ----------------------------------------------------------------- > > This does mean though that any add-on that subverts our new API and does an > appendChild on the #nav-bar will probably have too much flex assigned to it. Yes. Those add-ons should be fixed, so I'm OK with their breakage becoming more noticeable. :-)
Whiteboard: [Australis:P4] → [Australis:P4][Australis:M9][fixed-in-ux]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][Australis:M9][fixed-in-ux] → [Australis:P4][Australis:M9]
Target Milestone: --- → Firefox 28
Depends on: 964433
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: