Last Comment Bug 644514 - -moz-box-ordinal-group:0 doesn't work, as opposed to ordinal="0"
: -moz-box-ordinal-group:0 doesn't work, as opposed to ordinal="0"
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: mozilla7
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks: 102440
  Show dependency treegraph
 
Reported: 2011-03-24 01:29 PDT by Dão Gottwald [:dao]
Modified: 2011-06-12 20:18 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: use any values that come from CSS (1.22 KB, patch)
2011-03-26 14:32 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review
patch 2: accept XUL attributes only (3.32 KB, patch)
2011-03-26 14:32 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch 2: accept XUL attributes only (4.55 KB, patch)
2011-03-29 10:03 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review
patch 2: accept XUL attributes only (16.56 KB, patch)
2011-04-19 17:46 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch 2: allow zero (1.63 KB, patch)
2011-05-24 09:59 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review

Description Dão Gottwald [:dao] 2011-03-24 01:29:32 PDT
spinoff from bug 636034
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-24 06:57:02 PDT
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: ⌚️UTC-7 (review requests must explain patch) 2011-03-26 14:19:51 PDT
Should it also accept negative values, or just zero?
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-26 14:32:19 PDT
Created attachment 522140 [details] [diff] [review]
patch 1: use any values that come from CSS
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-26 14:32:49 PDT
Created attachment 522141 [details] [diff] [review]
patch 2: accept XUL attributes only

I should add a test for this too...
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-26 14:33:12 PDT
patch 3 (the nsCSSPropList.h change) is yet to be written, depending on the answer to comment 2.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-26 14:34:33 PDT
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: ⌚️UTC-7 (review requests must explain patch) 2011-03-26 14:34:56 PDT
(And I wonder whether it's safe for nsBoxFrame::RegUnRegAccessKey to do the same.)
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-29 10:03:56 PDT
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.
Comment 9 Dão Gottwald [:dao] 2011-03-29 10:13:58 PDT
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] 2011-03-29 10:14:50 PDT
toolkit/content/widgets/textbox.xml is affected as well.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-05 16:44:52 PDT
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 [:bz] (Out June 25-July 6) 2011-04-05 16:46:31 PDT
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: ⌚️UTC-7 (review requests must explain patch) 2011-04-19 17:38:23 PDT
Dão, Neil, do you have an opinion on comment 2?
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-19 17:46:56 PDT
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.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-19 17:50:51 PDT
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: ⌚️UTC-7 (review requests must explain patch) 2011-04-19 21:26:28 PDT
Landed patch 1 as:
https://hg.mozilla.org/mozilla-central/rev/45b20f137549
Comment 17 Dão Gottwald [:dao] 2011-04-19 22:22:51 PDT
(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: ⌚️UTC-7 (review requests must explain patch) 2011-05-23 20:55:29 PDT
I forgot to do comment 11...
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-24 09:59:41 PDT
Created attachment 534813 [details] [diff] [review]
patch 2: allow zero
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-24 12:50:31 PDT
Comment on attachment 534813 [details] [diff] [review]
patch 2: allow zero

This still doesn't do comment 11...
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-24 12:59:11 PDT
Yeah, I stuck that in my queue as a separate patch. :-)
Comment 22 Neil Deakin 2011-06-01 07:50:25 PDT
(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: ⌚️UTC-7 (review requests must explain patch) 2011-06-12 20:18:42 PDT
comment 11:
https://hg.mozilla.org/mozilla-central/rev/d0b3065150f7

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

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