Last Comment Bug 575500 - -moz-box-ordinal-group doesn't preserve secondary sort (by content order) under mutation
: -moz-box-ordinal-group doesn't preserve secondary sort (by content order) und...
Status: NEW
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 10 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: abp
  Show dependency treegraph
 
Reported: 2010-06-28 21:14 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2014-01-21 17:45 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (1.20 KB, text/html)
2012-06-27 03:50 PDT, Wladimir Palant
no flags Details

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-06-28 21:14:17 PDT
This is a followup to bug 555987.

Handling of dynamic changes to -moz-box-ordinal-group (and the XUL ordinal attribute) now correctly handles the primary sort key (the ordinal number) following mutation of the ordinals.  However, it does not handle the secondary sort key (sort by content order) correctly, since, following a mutation, the code does not re-sort by content order (only by ordinal); instead it inserts the element with the changed ordinal after its previous sibling and then does a sort by ordinal that is otherwise a stable sort.

I believe this is why the reftest layout/reftests/box-ordinal/dynamic-1-remove-to-one-grouped-2.xul fails.

Additionally, the tests in layout/reftests/box-ordinal/ are only testing very simple cases and probably ought to be expanded a bit.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-06-28 21:15:26 PDT
In bug 555987 comment 10, I wrote:
> We could make nsLayoutUtils::CompareTreePosition part of the sort algorithm.
...
> (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 2 Nochum Sossonko [:Natch] 2010-06-29 05:34:08 PDT
Also see bug 102054, which has now been fixed besides for this...
Comment 3 Wladimir Palant 2012-06-27 03:50:55 PDT
Created attachment 637063 [details]
Testcase

Steps to reproduce:

1. Open testcase, observe "div1" text being displayed before "div2".
2. Click "Remove div1 and insert it into the same place" button.
3. Use Inspector tool or something comparable to verify that step 2 didn't change the DOM - the element was put into the same place where it was before.

Expected results:

Display doesn't change, div1 is still displayed before div2 (it works this way in Chrome 20).

Actual results:

The elements change positions, now div2 is displayed before div1 (reproduced in Firefox 13.0.1 and 16.0a1 nightly). This is due to an invisible element with -moz-box-ordinal-group CSS property, without this element things work correctly.

This affects Adblock Plus because the add-on bar in Firefox has the status bar as one of its children - and it has an orient="1000" attribute. So Adblock Plus will insert its icon into the correct position (according to user's customization) but it might be displayed elsewhere.
Comment 4 Markus Popp 2012-06-29 10:59:16 PDT
Maybe this is helpful:

before the update to Adblock Plus 2.1 I had the icons:

About Info - Separator - Firebug - Adblock Plus - Separator - other icons (FoxClocks, Rikaichan, Enable/Disable Flash from the QuickJava addon)

after the update I had:

About Info - Separator - Separator - Firebug - other icons - Adblock Plus

Not only moving the Adblock Plus icon back to its original position failed, also to move the Firebug icon in between the 2 Separators didn't work (I then removed one of the 2 separators).

Maybe also worth mentioning that I have the QuickJava and the Toolbar Buttons addons installed, which provide additional icons which are not available in Firefox out of the box.
Comment 5 Alex 2012-10-16 13:57:17 PDT
Can a developer please look at this bug?
Thank you!

More info here:
https://adblockplus.org/forum/viewtopic.php?f=11&t=10493&p=60997
Comment 6 Danial Horton 2013-03-01 18:40:51 PST
This affects session manager as well now that it has gone restartless.

Note You need to log in before you can comment on or make changes to this bug.