56088 - [FIX]ordered lists can't start below 1

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Simon Montagu :smontagu]

From Bugzilla Helper:
User-Agent: Mozilla/4.75 [en] (WinNT; U)
BuildID:    2000092008

Ordered lists with <ol start=n> with n <= 1 default to 1.
I can't find any restriction on the starting value of a list in the HTML or CSS 
specs, and IE correctly displays start=0 or even negative values

Reproducible: Always
Steps to Reproduce:
<ol start=0>
  <li>Get up</li>
  <li>Eat breakfast</li>
  <li>Go to work</li>
<ol>

Actual Results:  
1. Get up
2. Eat breakfast
3. Go to work                   

Expected Results:  
0. Get up
1. Eat breakfast
2. Go to work                            

(A potential problem with fixing this would be the possibilities for 
"list-style-type" (e.g roman and alpha) which are undefined below 1.)

The CSS spec at http://www.w3.org/TR/CSS2/generate.html is rather 
self-contradictory: section 12.6.2 seems to imply that all numbering systems 
start at 1, but section 12.5 gives examples of counters with zero and negative 
values.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Reading over the spec, it seems that the simplest interpretation is that
counters can take on any values but the behavior of non-numeric counters for
values outside a certain range is not defined and it is the content provider's
responsibility to avoid such counters.  The spec explicitly says that the
behavior of the a,b,c,... counter is undefined after one reaches 26, for example.

On the other hand, numeric counters, for the most part, also do not handle
negative values.  how would one write -9 in Roman numerals, for example?

Other systems, such as TeX, will simply give an error when a counter is out of
range for the formatting method.  I guess that's not really the best option here...

Comment 2

[Loco]

Actually, 0 is also undefined in Roman numerals.  That was one fo the reasons why 
the Roman empire fell (so I heard).  I can't see any instance where a list *
should* start at 0 or negative numbers.  Yes, there are "Zeroeth" Laws (say, of 
thermodynamics, for example) but in ordinary usage its not numbered.  I'd be 
inclined to allow zeroes, but not negative numbers.  Othewise, I'd mark it as 
INVALID.  Any thoughts?

Comment 3

[Loco]

Attachment: Simple testcase from [e-mail] in a file.

Comment 4

[Loco]

Confirmed on Mac 10-11-08.

Comment 5

[Loco]

Boy, I keep forgetting to do this all in one step.  My apologies for the spam.  
(Changing platform/OS to ALL, priority to P4, and severity to trivial)

Comment 6

[Hixie (not reading bugmail)]

Per CSS2 (http://www.w3.org/TR/REC-CSS2/generate.html#counters):  
#   Zero and negative integers are allowed. 

Severity back to normal, this is a standards compliance issue.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  The line about zero and negative integers being allowed refers to the
counter-increment property, not the actual values of the counters.

Furthermore, the spec says that:

"By default, counters are formatted with decimal numbers, but all the styles
available for the 'list-style-type' property are also available for counters."

The specification of the 'list-style-type' property (at
http://www.w3.org/TR/REC-CSS2/generate.html#propdef-list-style-type) defines
"decimal" as:

"Decimal numbers, beginning with 1."

And gives similar definitions for the other values of the property.  Implicitly,
it seems to be ruling out zero or negative numbers as values of a counter that
can be meaningfully formatted. (Note that until you actually output some text, a
counter can really have any value you want.  It would make a lot of sense, for
example, to set the 'chapter' counter to 0 at the beginning of a document and
increment by 1 for each H1 encountered.  If the 'chapter' counter's value is
only formatted when an H1 is encountered, the 0 would never be formatted.)

This seems to be an issue that could use some clarification from the
spec-writers.....

Comment 8

[Hixie (not reading bugmail)]

First of all, if counter-increment can be negative and the initial counter is 1,
then it is clear that counters can reach negative numbers very easily.

Secondly, counter-reset sets the counter, and that property takes an <integer>
as the value, and that is defined as being either positive or negative. 

Thirdly, HTML defines 'start' as taking an integer, which mathematically may
be negative too.

So whichever way you look at it, there is no reason to think that the numbers
should not be negative. So if we don't allow that then we are wrong.

Comment 9

[Boris Zbarsky [:bzbarsky]]

There are two separate issues here:

1)  What values of the counter does mozilla allow?

This should be any integer (positive, zero, or negative)

2)  What does mozilla display for a given value of the counter?

This depends on the list-style-type and for most of the types there are values
of the counter that cannot be displayed.

So... looks like we should allow negative counters but come up with a decent way
to fail when someone wants to format -1 as a Roman numeral.

Comment 10

[karnaze (gone)]

Reassigning to Buster and marking future.

Comment 11

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 12

[bsharma]

qa contact updated.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 98486 has been marked as a duplicate of this bug. ***

Comment 15

[Kevin McCluskey (gone)]

Build reassigning Buster's bugs to Marc.

Comment 16

[Bill Weinman]

The HTML 4.01 DTD entry for the START attribute to the OL element lists the
attribute type as "number". Number is further defined as a value consisting of
"at least one digit [0-9]". That means that zero is explicitly a valid value. 

http://www.w3.org/TR/html4/struct/lists.html#adef-start
http://www.w3.org/TR/html4/types.html#type-number

The same rule should be applied to the VALUE attribute with an LI element.

Comment 17

[Mats Palmgren (inactive)]

*** Bug 118620 has been marked as a duplicate of this bug. ***

Comment 18

[Scott Gifford (old account)]

Here is a Web site which makes legitimate use of <ol start=0>:

    http://htmlhelp.org/faq/cgifaq.html

It uses it for a preamble to the document itself.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 149314 has been marked as a duplicate of this bug. ***

Comment 20

[Boris Zbarsky [:bzbarsky]]

OK.  Is the CSS3 spec likely to clarify this at all?  Or should we just treat
any list-style-type other than "decimal" that attempts to use a non-positive
value as an error and just deal somehow?

Comment 21

[Hixie (not reading bugmail)]

What is there to clarify?

Comment 22

[Boris Zbarsky [:bzbarsky]]

<ol start="-10" style="list-style-type: hiragana">

Should we just make something up for the -10 ... 0 items?

Comment 23

[Hixie (not reading bugmail)]

Oh, right. Yeah, CSS3 will clarify that.

Comment 24

[Hixie (not reading bugmail)]

(CSS3 will say "fallback to decimal")

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: more interesting testcase

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

This fixes all the numbering systems we support to do the right thing with 0
and negative numbers.... It doesn't really address other issues some of those
algorithms have.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Taking.  Ian, what do you think?

Comment 28

[Hixie (not reading bugmail)]

Seems ok. This will all have to be refactored to do counter() anyway.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Yes, of course.  I just needed an algorithmic sanity check before bothering
people about code reviews.  ;)

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 103433 [details] [diff] [review]
Patch

- In static PRBool HebrewToText(PRInt32 ordinal, nsString& result)

>+#ifdef IBMBIDI
>+    static const PRUnichar hebrewZero[] = { 0x05D0, 0x05E4, 0x05E1 };
>+#else
>+    static const PRUnichar hebrewZero[] = { 0x05E1, 0x05E4, 0x05D0 };
>+#endif // IBMBIDI

I don't claim to be an expert in hebrew or bidi anything, but don't you want to
check if we are in ltr or rtl if we're in bidi?


- In ArmenianToText() and GeorgianToText() could you remove the else after
return?  The other FooToText impls don't have this else after return and I
would argue that is a good thing.

- Do you think it would be a good idea to combine the impls of ParseValue to
take a |PRInt32 aMin = PR_INT32_MIN, PRInt32 aMax = PR_INT32_MAX| as the last
args?

- Not your fault, but while looking at nsGenericHTMLElement.h I noticed this:
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsGenericHTMLEle
ment.h#381  If you decide to do the above point in a separate bug (which I
think is probably prudent), could you at least fix that comment to also talk
about max values and fix the @param for the max, since you're touching related
stuff?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> but don't you want to check if we are in ltr or rtl

Nope.  I should just put the codepoints in there and the BIDI stuff should
handle the direction itself.

Good catch on the else after return.

I was sorta trying to avoid modifying all the callers of ParseValue()...  If you
feel I should just bite the bullet and do that, I ca, but as you say a separate
bug would be preferable.

I'll fix the comment to correspond to reality.

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

Please explain the bidi thing in more detail, on IRC if you like.  In any case,
I feel a comment is needed here.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: fix the nits

Comment 34

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 104720 [details] [diff] [review]
fix the nits


>+#ifdef IBMBIDI
>+    static const PRUnichar hebrewZero[] = { 0x05D0, 0x05E4, 0x05E1 };
>+#else
>+    static const PRUnichar hebrewZero[] = { 0x05E1, 0x05E4, 0x05D0 };
>+#endif // IBMBIDI

Okay.  Please get someone to verify that this does the right thing if someone
tries to force the direction to ltr while in bidi, though.  Also maybe consider

      static const PRUnichar hebrewZero[] =
#ifdef IBMBIDI
	  { 0x05D0, 0x05E4, 0x05E1 }
#else
	  { 0x05E1, 0x05E4, 0x05D0 }
#endif
      ;


>+static PRBool ArmenianToText(PRInt32 ordinal, nsString& result)
> {
>-  if((0 == ordinal) || (ordinal > 9999)) { // zero or reach the limit of Armenain numbering system
>+  // XXXbz this system goes out to a lot further than 9999... we should fix
>+  // that.  This algorithm seems broken in general.  There's this business of
>+  // "7000" being special and then there's the combining accent we're supposed
>+  // to be using...
>+  if (ordinal <= 0 || ordinal > 9999) { // zero or reach the limit of Armenain numbering system

Is 0 really a special case?  Looks like you're only using it because of the
code's heritage.  But it seems to me on reading this again that it would be
clearer if you used 1 here as the boundary since this and other systems systems
such as Roman, etc. are defined from 1...n; in the case of Armenian, n = 9999.
(they cannot possibly be defined from "things greater than 0"...n because they
have no notion of 0). Can you please update the code for all relevant numbering
systems?  Also, isn't Armenian misspelled in the comment?


>     DecimalToText(ordinal, result);
>-    return;
>-  } else {
>-    PRUnichar buf[NUM_BUF_SIZE];
>-    PRInt32 idx = NUM_BUF_SIZE;
>-    PRInt32 d = 0;
>-    do {
>-      PRInt32 cur = ordinal % 10;
>-      if( cur > 0)
>-      {
>-        PRUnichar u = 0x0530 + (d * 9) + cur;
>-        buf[--idx] = u;
>-      }
>-      d++;
>-      ordinal /= 10;
>-    } while ( ordinal > 0);
>-    result.Append(buf+idx,NUM_BUF_SIZE-idx);
>+    return PR_FALSE;
>   }
>+
>+  PRUnichar buf[NUM_BUF_SIZE];
>+  PRInt32 idx = NUM_BUF_SIZE;
>+  PRInt32 d = 0;
>+  do {
>+    PRInt32 cur = ordinal % 10;
>+    if( cur > 0)

Wanna fix spacing with the braces?

>+    {
>+      PRUnichar u = 0x0530 + (d * 9) + cur;
>+      buf[--idx] = u;
>+    }
>+    d++;

++d

>+    ordinal /= 10;
>+  } while ( ordinal > 0);

And here, too?

>+  result.Append(buf+idx,NUM_BUF_SIZE-idx);

Add spaces here between operators, and after the comma?

And the last 4 nits apply to GeorgianToText, as well....

With the above changes, and if someone more knowledgable in bidi signs off on
the one case we discussed, then r=caillon.

Also, please make sure there is a bug filed on combining the ParseValue impls.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: fix the uber-nits

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 105570 [details] [diff] [review]
fix the uber-nits

sr=roc+moz

Couldn't you take the DecimalToText out of all the failing cases of *ToText,
and put it at the bottom of GetListItemText? That's optional though.

One day someone should go and rip out all the #ifdef IBMBIDIs. We're never
going to turn it off.

Comment 37

[Simon Montagu :smontagu]

Attachment: Testcase for Bidi (logical Hebrew)

The Hebrew numbering has to work with all combinations of --disable-bidi on or
off (at least for now), operating systems with and without native bidi support
and pages in Visual and Logical Hebrew. I'm not sure that this patch will do
that.

Comment 38

[Simon Montagu :smontagu]

Attachment: Testcase for Bidi (visual Hebrew)

Comment 39

[Simon Montagu :smontagu]

Comment on attachment 105570 [details] [diff] [review]
fix the uber-nits

Also, shouldn't there be a change in nsBulletFrame.h?

- void GetListItemText...
+ PRBool GetListItemText...

Comment 40

[Simon Montagu :smontagu]

Attachment: Testcase for Bidi (logical Hebrew)

On testing with the patch, I see that this old testcase is slightly broken.

Comment 41

[Simon Montagu :smontagu]

Attachment: Testcase for Bidi (visual Hebrew)

Comment 42

[Boris Zbarsky [:bzbarsky]]

Simon, what exactly is broken?  Could you be more specific?

Comment 43

[Simon Montagu :smontagu]

Comment on attachment 105570 [details] [diff] [review]
fix the uber-nits

OK, with bidi enabled, all the other combinations work (at least there are no
new problems introduced: there are some other smaller and greater issues with
the Hebrew numbers which should be fixed, but that can and should be a separate
bug).

I suspect that the combination which will break is bidi disabled on an OS with
Bidi support, but I don't think we need to bend over backwards to support that
situation. If anybody complains, we can release-note it, or we could remove
list-style-type: hebrew altogether when bidi is disabled, as you suggested on
IRC.

So, r=smontagu for Bidi, BUT the patch as-is didn't build for me anywhere until
I made the change to nsBulletFrame.h in comment 39, and still didn't build on
Windows until I removed the |inline| from |ParseValue()| in
nsGenericHTMLElement.cpp, so please don't check in without addressing those
issues.

Comment 44

[Simon Montagu :smontagu]

I meant that the version I originally attached was broken. The second pair of
attachments are OK.

Comment 45

[Boris Zbarsky [:bzbarsky]]

> put it at the bottom of GetListItemText

I could, but this keeps it a little more explicit, especially because we're
likely to start reusing those functions for general counter support, not just
list items at some point..

Simon, thanks for pointing out the build problems.  I had simply forgotten to
diff the .h and the inline stuff was indeed an issue.

Patch checked in with the addition of nsBulletFrame.h and moving the impls of
the inline versions of ParseValue() into the header to unconfuse the windows
compiler.

Comment 46

[Stefan [:stefanh]]

*** Bug 212311 has been marked as a duplicate of this bug. ***