Closed
Bug 555987
Opened 15 years ago
Closed 14 years ago
{inc}Dynamically changing -moz-box-ordinal-group doesn't work
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta1+ |
People
(Reporter: dao, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: testcase)
Attachments
(2 files)
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
In the attached testcase, each click on the colored boxes should make them swap positions. This works with the 'ordinal' attribute (green boxes) but not with -moz-box-ordinal-group (red boxes). Setting/removing a class or an attribute that -moz-box-ordinal-group depends on doesn't work either.
I also noticed that when on this page: http://hacks.mozilla.org/wp-content/uploads/2010/04/exemple-blog.html when I use Firebug to turn off the "-moz-box-ordinal-group" style property for
#content .topgroup
it correctly places the ARTICLE back in it's natural HTML flow order. However, when I re-enable this style, it does not switch back to the top.
Comment 2•15 years ago
|
||
nsStyleXUL::CalcDifference does a reframe on the element whose -moz-box-ordinal-group changed.
nsBoxFrame::AttributeChanged calls parent->RelayoutChildAtOrdinal when ordinal changes.
So, presumably we also have buggy handling when dynamically inserting a node with an "unusual" ordinal, and fixing that would fix this bug? But we could also improve things by adding a new change hint for this case.
Updated•15 years ago
|
Summary: Dynamically changing -moz-box-ordinal-group doesn't work → {inc}Dynamically changing -moz-box-ordinal-group doesn't work
Updated•14 years ago
|
Updated•14 years ago
|
Component: Style System (CSS) → Layout
QA Contact: style-system → layout
Assignee | ||
Comment 3•14 years ago
|
||
So... CheckBoxOrder takes an nsBoxLayoutState... but the only caller is nsBoxFrame::SetInitialChildList.
We used to call it in nsContainerBox::Prepend, nsContainerBox::Append, nsContainerBox::InsertAfter (in addition to nsContainerBox::InitChildren) before bug 258513 landed, but nothing replaced those callsites.
Comment 4•14 years ago
|
||
This is causing tabs on top to switch to tabs on bottom when a persona is applied (bug 574739) which is blocking the release of the first beta of Firefox 4.
blocking2.0: --- → beta1+
Assignee | ||
Comment 5•14 years ago
|
||
If someone has time to write tests, that would rock.
I should note that this patch breaks dynamically changing the "tabs on top" setting via the menu, but due to what looks like a bug in front-end code. The front-end sets a moz-box-ordinal-group on the #navigator-toolbox when that menuitem is used, which causes it to sort below everything else. So you end up with, going top to bottom, content area, status bar, menu, then tabs and toolbars (in the order determined by the current pref value). We probably need a bug on fixing that front-end bit.
Attachment #454542 -
Flags: review?(dbaron)
Comment 6•14 years ago
|
||
Comment on attachment 454542 [details] [diff] [review]
Obvious fix
I'm worried that this might just change us from one wrong behavior to another: does CheckBoxOrder and the mergesort algorithm it uses actually do something to sort starting from the original order? It looks more like it starts from the current state, which seems like it would do the wrong thing when frames whose -moz-box-ordinal-group was previously different change to having the same ordinal group.
Assignee | ||
Comment 7•14 years ago
|
||
> It looks more like it starts from the current state
That's what it does, yes.
> which seems like it would do the wrong thing when frames whose
> -moz-box-ordinal-group was previously different change to having the same
> ordinal group.
When that happens, we will reframe them, right? So let's say we have A and B. A has ordinal 1, B has ordinal 2. B comes before A in the DOM, but after in the frame tree.
We change the ordinal of A and B to 3. We process the two style changes. For now, we end up with two separate reframes. Whichever one we reframe second will put itself in the right place, no? Even if we coalesced the reframes (via lazy frameconst), they would end up in the right order, I think.
Comment 8•14 years ago
|
||
What says that they're *both* getting reframed? And what says the InsertFrames call put the new frames in any sensible place given the other reordering that already happened?
Assignee | ||
Comment 9•14 years ago
|
||
> And what says the InsertFrames call put the new frames in any sensible place
Hmm... InsertFrames will put it after the DOM previous sibling.
So I guess you're worried about a situation like this:
<box ordinal="5"/>
<box ordinal="1"/>
And someone inserting another ordinal="1" box between them. We'll end up getting the order of the two ordinal="1" boxes wrong in that case, I think....
I agree that's a problem. Short of reframing the parent on ordinal changes, I don't see a good fix.
Updated•14 years ago
|
Attachment #454542 -
Flags: review?(dbaron) → review+
Comment 10•14 years ago
|
||
Comment on attachment 454542 [details] [diff] [review]
Obvious fix
We could make nsLayoutUtils::CompareTreePosition part of the sort algorithm. I'd be ok with doing that later, though, so r=dbaron on this.
(We could also make ordinal changes a separate style hint so that we almost never have to deal with handling them, which would also make them not require a reframe.)
Comment 11•14 years ago
|
||
We can haz checkin?
Assignee | ||
Comment 12•14 years ago
|
||
If you're ok with checkin without tests thus far... Otherwise, once we have tests.
Comment 13•14 years ago
|
||
I'm OK to check in without tests if they're coming. Really want to start on builds.
Comment 14•14 years ago
|
||
I wrote 6 basic tests:
http://hg.mozilla.org/mozilla-central/rev/c51bb774fb71
(before the patch, we failed 3 of 6; with it we fail 1 of 6)
and landed the patch:
http://hg.mozilla.org/mozilla-central/rev/ce2f6b5e0525
However, before starting on builds, I think we also need to remove a workaround for this from browser code that bz mentioned on IRC earlier today.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 15•14 years ago
|
||
(In reply to comment #14)
> However, before starting on builds, I think we also need to remove a workaround
> for this from browser code that bz mentioned on IRC earlier today.
16:58 < firebot> Check-in: http://hg.mozilla.org/mozilla-central/rev/00bf38db60e2 - Gavin Sharp - Remove bogus workaround for bug 555987 now that it's fixed
Seems you got that done, too!
Updated•14 years ago
|
Assignee: nobody → bzbarsky
Reporter | ||
Comment 17•14 years ago
|
||
> http://hg.mozilla.org/mozilla-central/rev/00bf38db60e2 - Gavin Sharp - Remove
> bogus workaround for bug 555987 now that it's fixed
That wasn't bogus, fwiw. It worked without this fix and broke with this fix expectedly, as the comment indicated.
Comment 18•14 years ago
|
||
(In reply to comment #17)
> That wasn't bogus, fwiw. It worked without this fix and broke with this fix
> expectedly, as the comment indicated.
It was relying on the bug to not break the browser's appearance (setting ordinal-group on gNavToolbox makes it appear below content). Surely there were other ways to trigger a "reframe" that didn't involve setting that bogus style?
Reporter | ||
Comment 19•14 years ago
|
||
Yes, it relied on the bug, hence the need to remove it when that bug was fixed. I phrased that comment very carefully. :) I don't know if there would have been other ways to implement the workaround (I tried a few things that didn't work), but we would have wanted to remove it either way with the underlying bug being fixed.
Assignee | ||
Comment 20•14 years ago
|
||
David, thanks for writing those tests! And thank you both for pushing stuff. I'm sorry I had to disappear for a bit there. :(
Comment 21•14 years ago
|
||
I filed bug 575500 to follow up on the remaining issues.
You need to log in
before you can comment on or make changes to this bug.
Description
•