93519 - Implement box-ordinal-group (and the ordinal attribute)

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[David Hyatt]

See the spec at http://www.mozilla.org/projects/xul/layout.html for details on 
how this property is supposed to work.

Comment 1

[[e-mail]]

[How] does this relate to position="..."?

Comment 2

[Joe Hewitt (gone)]

I'd like to get some feedback on my progress so far, so I'll post a patch with
my changes.  I have not yet added support for the -moz-box-ordinal-group CSS
property, so this patch only includes support for the ordinal attribute.

I should mention that I have also made some changes to outliner to allow it to
support box-ordinal-group, but I'll post them later on.

Comment 3

[Joe Hewitt (gone)]

Attachment: patch

Comment 4

[Joe Hewitt (gone)]

Attachment: xul test case

Comment 5

[David Hyatt]

Looks good.  We need to add the CSS property as well. :)

Comment 6

[Joe Hewitt (gone)]

Attachment: this oughta do the trick

Comment 7

[David Hyatt]

(1) Fix the bad indentation in AddCSSOrdinal. 
(2) Remove the printf from AddCSSOrdinal.
(3) In the box object code that gets ordinal siblings, you QI the parent frame 
to a box and then walk the box list, but then convert back to frames.  Just 
walk the frame list.  The code will simpler I think.
(4) You didn't increment the total # of properties in the CheckXULProperties 
function.  You need to increment it now that you added a 6th property.  This 
can cause bad style glitches to occur.  

Comment 8

[Joe Hewitt (gone)]

Attachment: new patch including hyatt's feedback

Comment 9

[David Hyatt]

Comment on attachment 47739 [details] [diff] [review]
new patch including hyatt's feedback

sr=hyatt

Comment 10

[Chris Waterson]

Looks like the XXX comment in nsCSSPropList.h is not appropriate.

Also, I'm not sure that NS_QuickSort() is stable, so...how do new ordinals get
assigned? It looks like you might end up with a whole lot of boxes with the same
ordinal (namely, one), and these could be randomly re-ordered if NS_QuickSort()
is unstable...

Comment 11

[Joe Hewitt (gone)]

The only case in which the boxes would be sorted is if at least one box has an
ordinal value specified on it.  In this case, there are currently no rules
specified in the xul spec for where the non-ordinal-specified boxes would be
painted.  Therefore, I think it is fair for these boxes to be ordered randomly.
 I am not confident that it would be a valuable service to try and keep these
boxes in their natural dom order.  The developer should specify an ordinal for
all boxes, or none at all.  

So, if NS_QuickSort() is not stable, that's fine by me.  Hyatt, what do you think?

Comment 12

[David Hyatt]

It is possible to assign the same ordinal to multiple boxes (e.g., by default
all boxes have an ordinal group of 1), and if you do this, then those boxes
should be in element order (i.e., in the order they appear in the content
model).  If quicksort does not guarantee this, then there is a bug here.

Also, it is specified that boxes by default belong to ordinal group 1, so it is
well-known where those boxes should appear.

There should be no randomness here.  Everything should be completely deterministic.

Comment 13

[Joe Hewitt (gone)]

I can easily fix it so that I sort manually instead of using NS_QuickSort so
that we can guarantee dom element order for boxes with the same order.  However,
in the case where box ordinals are being changed dynamically.  It would be a
considerable amount of extra work to try and guarantee that boxes with the same
order appear in  dom element order all the time.  

With the current patch, as boxes are moved from one ordinal group into another,
they will be painted in the order of their arrival in the group from earliest to
latest.  I did it this way primarily for efficiency.  Were I to gather the
element order of all boxes in a group, and then re-sort them each time a box
changes it's ordinal, that could conceivably slow the process down a bit, even
for cases where such ordering is unnecessary.

Comment 14

[Chris Waterson]

Comment on attachment 47739 [details] [diff] [review]
new patch including hyatt's feedback

r=waterson if you fix the comments in nsCSSPropList.h

Comment 15

[Chris Waterson]

I think it's probably fine as is; we probably ought to file another bug to
ensure that NS_QuickSort() is stable.

Comment 16

[Brendan Eich [:brendan]]

If you can factor entry order into the key, you can make an unstable sort be
stable.  Given a "compare" callback that takes pointers to entry storage, it's
easy to break "ties" based on address order.  So I'm not convinced NS_QuickSort
should be changed.

/be

Comment 17

[Joe Hewitt (gone)]

fixed.  The XUL 1.0 box model is done.

Comment 18

[rbs]

Maybe you could have used an nsAutoVoidArray boxes (this way, there wouldn't be 
an effective |new| if there are only few elements -- which is what might happen 
in practice), and from there an in-place boxes.Sort(BoxOrderSortComparison, 
(void*)&aState) would have produced the same effect (c.f. bug 88344). 

Comment 19

[Bradley Baetz (:bbaetz)]

Index: mozilla/content/base/src/nsStyleContext.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsStyleContext.cpp,v
retrieving revision 3.173
diff -u -r3.173 nsStyleContext.cpp
--- nsStyleContext.cpp	2001/08/02 00:06:59	3.173
+++ nsStyleContext.cpp	2001/08/30 23:51:01
@@ -1129,7 +1129,8 @@
     (int)xul->mBoxDirection,
     (int)xul->mBoxFlex,
     (int)xul->mBoxOrient,
-    (int)xul->mBoxPack);
+    (int)xul->mBoxPack,
+    (int)xul->mBoxOrdinal);
   fprintf(out, "\" />\n");
 #endif
   //#insert new style structs here#

You changed the number of things printed out by printf, but didn't update the
format string.

Comment 20

[Joe Hewitt (gone)]

whoops, thanks for pointing that out bradley.  fix checked in