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)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: Gijs, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Australis:P4][Australis:M9])
Attachments
(1 file)
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•11 years ago
|
||
Flags: needinfo?(jaws)
Assignee | ||
Comment 2•11 years ago
|
||
(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.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #819654 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment 4•11 years ago
|
||
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+
Assignee | ||
Comment 5•11 years ago
|
||
(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. :-)
Assignee | ||
Comment 6•11 years ago
|
||
Whiteboard: [Australis:P4] → [Australis:P4][Australis:M9][fixed-in-ux]
Assignee | ||
Comment 7•11 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•