Closed Bug 555987 Opened 14 years ago Closed 14 years ago

{inc}Dynamically changing -moz-box-ordinal-group doesn't work

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: dao, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files)

Attached file testcase
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.
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.
Summary: Dynamically changing -moz-box-ordinal-group doesn't work → {inc}Dynamically changing -moz-box-ordinal-group doesn't work
Component: Style System (CSS) → Layout
QA Contact: style-system → layout
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.
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+
Attached patch Obvious fixSplinter Review
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 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.
> 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.
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?
> 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.
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.)
We can haz checkin?
If you're ok with checkin without tests thus far...  Otherwise, once we have tests.
I'm OK to check in without tests if they're coming. Really want to start on builds.
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
(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!
Assignee: nobody → bzbarsky
> 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.
(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?
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.
David, thanks for writing those tests!  And thank you both for pushing stuff.  I'm sorry I had to disappear for a bit there.  :(
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.