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

RESOLVED FIXED in mozilla0.9.5

Status

()

Core
XUL
P2
normal
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: David Hyatt, Assigned: Joe Hewitt (gone))

Tracking

Trunk
mozilla0.9.5
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [xul1.0-layout-box])

Attachments

(4 attachments)

(Reporter)

Description

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

Updated

17 years ago
Blocks: 70753
Whiteboard: [xul1.0-layout-box]

Comment 1

17 years ago
[How] does this relate to position="..."?
(Assignee)

Updated

17 years ago
Blocks: 9656
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5
(Assignee)

Comment 2

17 years ago
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.

(Assignee)

Comment 3

17 years ago
Created attachment 46860 [details] [diff] [review]
patch
(Assignee)

Comment 4

17 years ago
Created attachment 46861 [details]
xul test case
(Reporter)

Comment 5

17 years ago
Looks good.  We need to add the CSS property as well. :)
(Assignee)

Comment 6

17 years ago
Created attachment 47634 [details] [diff] [review]
this oughta do the trick
(Reporter)

Comment 7

17 years ago
(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.  

(Assignee)

Comment 8

17 years ago
Created attachment 47739 [details] [diff] [review]
new patch including hyatt's feedback
(Reporter)

Comment 9

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

sr=hyatt
Attachment #47739 - Flags: superreview+
(Reporter)

Updated

17 years ago
Attachment #46861 - Attachment mime type: text/xul → application/vnd.mozilla.xul+xml
(Assignee)

Updated

17 years ago
Priority: -- → P2

Comment 10

17 years ago
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...
(Assignee)

Comment 11

17 years ago
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?
(Reporter)

Comment 12

17 years ago
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.

(Assignee)

Comment 13

17 years ago
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

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

r=waterson if you fix the comments in nsCSSPropList.h
Attachment #47739 - Flags: review+

Comment 15

17 years ago
I think it's probably fine as is; we probably ought to file another bug to
ensure that NS_QuickSort() is stable.
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
(Assignee)

Comment 17

17 years ago
fixed.  The XUL 1.0 box model is done.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 18

17 years ago
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). 
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.
(Assignee)

Comment 20

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

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.