The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla7

Status

()

Core
CSS Parsing and Computation
P4
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dao, Assigned: dbaron)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
spinoff from bug 636034
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?
(Assignee)

Comment 2

6 years ago
Should it also accept negative values, or just zero?
(Assignee)

Comment 3

6 years ago
Created attachment 522140 [details] [diff] [review]
patch 1: use any values that come from CSS
Attachment #522140 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

6 years ago
Created attachment 522141 [details] [diff] [review]
patch 2: accept XUL attributes only

I should add a test for this too...
Attachment #522141 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

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

Comment 6

6 years ago
Regarding patch 2:  I'd note nsIBox::AddCSSPrefSize already checks IsXUL.  This is making the other functions do the same.
(Assignee)

Comment 7

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

Comment 8

6 years ago
Created attachment 522720 [details] [diff] [review]
patch 2: accept XUL attributes only

The fact that I needed one change, though, suggests there might be more.
Attachment #522141 - Attachment is obsolete: true
Attachment #522141 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #522720 - Flags: review?(bzbarsky)
(Reporter)

Comment 9

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

Comment 10

6 years ago
toolkit/content/widgets/textbox.xml is affected as well.
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?
Attachment #522140 - Flags: review?(bzbarsky) → review+
Comment on attachment 522720 [details] [diff] [review]
patch 2: accept XUL attributes only

r=me.  This looks like it fixes bug 102440 too, right?
Attachment #522720 - Flags: review?(bzbarsky) → review+
Blocks: 102440
(Assignee)

Comment 13

6 years ago
Dão, Neil, do you have an opinion on comment 2?
(Assignee)

Comment 14

6 years ago
Created attachment 527167 [details] [diff] [review]
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.
Attachment #527167 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #522720 - Attachment is obsolete: true
(Assignee)

Comment 15

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

Actually, moved patch 2 to bug 102440.
Attachment #527167 - Attachment is obsolete: true
Attachment #527167 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

6 years ago
Landed patch 1 as:
https://hg.mozilla.org/mozilla-central/rev/45b20f137549
(Reporter)

Comment 17

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

Comment 18

6 years ago
I forgot to do comment 11...
(Assignee)

Comment 19

6 years ago
Created attachment 534813 [details] [diff] [review]
patch 2: allow zero
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #534813 - Flags: review?(bzbarsky)
Comment on attachment 534813 [details] [diff] [review]
patch 2: allow zero

This still doesn't do comment 11...
Attachment #534813 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 21

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

Comment 22

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

Negative values shouldn't be supported.
(Assignee)

Comment 23

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

patch 2:
http://hg.mozilla.org/mozilla-central/rev/db0bcc6ab526
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Priority: -- → P4
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.