644514 - -moz-box-ordinal-group:0 doesn't work, as opposed to ordinal="0"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Dão Gottwald [:dao]]

spinoff from bug 636034

Comment 1

[Boris Zbarsky [:bzbarsky]]

Specifically, the code looks like this:

519   PRUint32 ordinal = DEFAULT_ORDINAL_GROUP;
....
526     content->GetAttr(kNameSpaceID_None, nsGkAtoms::ordinal, value);
527     if (!value.IsEmpty()) {
528       ordinal = value.ToInteger(&error);
529     }
530     else {
531       // No attribute value.  Check CSS.
532       const nsStyleXUL* boxInfo = GetStyleXUL();
533       if (boxInfo->mBoxOrdinal > 1) {
534         // The ordinal group was defined in CSS.
535         ordinal = (nscoord)boxInfo->mBoxOrdinal;
536       }
537     }

DEFAULT_ORDINAL_GROUP is 1.

I think the right thing to do is to just take out that |if| test.  The default value of mBoxOrdinal is 1 already, I would assume (and if it's not we should make it so), thus assigning to |ordinal| would only have an effect if it's set to a non-default value anyway, right?

Comment 2

[David Baron :dbaron:]

Should it also accept negative values, or just zero?

Comment 3

[David Baron :dbaron:]

Attachment: patch 1: use any values that come from CSS

Comment 4

[David Baron :dbaron:]

Attachment: patch 2: accept XUL attributes only

I should add a test for this too...

Comment 5

[David Baron :dbaron:]

patch 3 (the nsCSSPropList.h change) is yet to be written, depending on the answer to comment 2.

Comment 6

[David Baron :dbaron:]

Regarding patch 2:  I'd note nsIBox::AddCSSPrefSize already checks IsXUL.  This is making the other functions do the same.

Comment 7

[David Baron :dbaron:]

(And I wonder whether it's safe for nsBoxFrame::RegUnRegAccessKey to do the same.)

Comment 8

[David Baron :dbaron:]

Attachment: patch 2: accept XUL attributes only

The fact that I needed one change, though, suggests there might be more.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 522720 [details] [diff] [review]
patch 2: accept XUL attributes only

>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -59,17 +59,17 @@
>           <xul:image class="autocomplete-icon" allowevents="true"/>
>         </children>
>         <xul:hbox anonid="textbox-input-box"
>                   class="textbox-input-box urlbar-input-box"
>                   flex="1" xbl:inherits="tooltiptext=inputtooltiptext">
>           <children/>
>           <html:input anonid="input"
>                       class="autocomplete-textbox urlbar-input textbox-input uri-element-right-align"
>-                      flex="1" allowevents="true"
>+                      style="-moz-box-flex: 1" allowevents="true"
>                       xbl:inherits="tooltiptext=inputtooltiptext,onfocus,onblur,value,type,maxlength,disabled,size,readonly,placeholder,tabindex,accesskey"/>
>         </xul:hbox>

toolkit/content/widgets/autocomplete.xml has the same markup

Comment 10

[Dão Gottwald [:dao]]

toolkit/content/widgets/textbox.xml is affected as well.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522140 [details] [diff] [review]
patch 1: use any values that come from CSS

r=me, but this removes the only consumer of DEFAULT_ORDINAL_GROUP.  Can you remove the macro too?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522720 [details] [diff] [review]
patch 2: accept XUL attributes only

r=me.  This looks like it fixes bug 102440 too, right?

Comment 13

[David Baron :dbaron:]

Dão, Neil, do you have an opinion on comment 2?

Comment 14

[David Baron :dbaron:]

Attachment: patch 2: accept XUL attributes only

I audited all "html:input" and "html:textarea" in the tree, which while in theory isn't sufficient, in practice I suspect it is.

Comment 15

[David Baron :dbaron:]

Comment on attachment 527167 [details] [diff] [review]
patch 2: accept XUL attributes only

Actually, moved patch 2 to bug 102440.

Comment 16

[David Baron :dbaron:]

Landed patch 1 as:
https://hg.mozilla.org/mozilla-central/rev/45b20f137549

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Should it also accept negative values, or just zero?

I can't say I see a striking need for negative values, but I don't see a downside of accepting them either...

Comment 18

[David Baron :dbaron:]

I forgot to do comment 11...

Comment 19

[David Baron :dbaron:]

Attachment: patch 2: allow zero

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534813 [details] [diff] [review]
patch 2: allow zero

This still doesn't do comment 11...

Comment 21

[David Baron :dbaron:]

Yeah, I stuck that in my queue as a separate patch. :-)

Comment 22

[Neil Deakin]

(In reply to comment #13)
> Dão, Neil, do you have an opinion on comment 2?

Negative values shouldn't be supported.

Comment 23

[David Baron :dbaron:]

comment 11:
https://hg.mozilla.org/mozilla-central/rev/d0b3065150f7

patch 2:
http://hg.mozilla.org/mozilla-central/rev/db0bcc6ab526