Last Comment Bug 601912 - HTML 5's <ol> reversed attribute not supported
: HTML 5's <ol> reversed attribute not supported
Status: RESOLVED FIXED
[parity-webkit]
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: mozilla18
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: html5 html5test
  Show dependency treegraph
 
Reported: 2010-10-05 05:20 PDT by Nuno Lopes
Modified: 2012-10-17 16:51 PDT (History)
20 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case: OL lists with start, value, and reversed attributes (1.37 KB, text/html)
2011-02-02 05:39 PST, Florens Verschelde
no flags Details
Test case: changing start value or count order with CSS (2.69 KB, text/html)
2011-02-02 06:48 PST, Florens Verschelde
no flags Details
Implement <ol reversed>. (15.32 KB, patch)
2012-06-28 08:51 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
With a test for appending (15.79 KB, patch)
2012-06-28 09:55 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
With some assertions also fixed so this passes tests (17.34 KB, patch)
2012-07-08 11:30 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Implement <ol reversed>. (15.81 KB, patch)
2012-07-14 00:10 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
With the various mass-changes applied (15.81 KB, patch)
2012-09-07 14:49 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
dholbert: review+
Details | Diff | Review

Description Nuno Lopes 2010-10-05 05:20:37 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10

HTML 5's <li> reversed attribute is ignored. It was supposed to reverse the list numbering.

Reproducible: Always
Comment 1 Matthias Versen [:Matti] 2010-10-05 07:04:12 PDT
I hope that you have tested this with FF4beta and not only with 3.6.x ?
Please attach a testcase if you get this with FF4.6beta6 or later
Comment 2 Nuno Lopes 2010-10-05 07:32:39 PDT
<html><body>
<ol reversed>
<li>3</li>
<li>2</li>
<li>1</li>
</ol>
</body></html>

prints:
1. 3
2. 2
3. 1

but it should print:
3. 3
2. 2
1. 1
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-10-05 08:11:18 PDT
I'm not crazy about this feature; we'd need to extend the CSS counter model to do this, and it's complicated enough already.
Comment 4 Florens Verschelde 2011-02-02 05:36:05 PST
Confirming: not implemented in FF4b10.
Comment 5 Florens Verschelde 2011-02-02 05:39:07 PST
Created attachment 509082 [details]
Test case: OL lists with start, value, and reversed attributes

Firefox 4 implements:
- <ol start=integer> (with positive and negative integers),
- <li value=integer> (likewise).
Doesn't implement:
- <ol reversed>
Comment 6 Florens Verschelde 2011-02-02 06:48:00 PST
Created attachment 509097 [details]
Test case: changing start value or count order with CSS

This test case illustrates that you can, with CSS:
- start an ordered list with a specific value (same as HTML start attribute),
- count in bigger increments (no HTML equivalent that I know of),
- count backwards (i.e. in negative increments).
But you can't:
- use CSS to count backwards (countdown style) unless you already know the number of items and you hardcode that value in your CSS.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-28 08:17:29 PDT
David, if we don't think this should be in the spec we should push back on the spec.  Letting it stay in the spec but not implementing is the worst of both worlds...
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-28 08:51:52 PDT
Created attachment 637531 [details] [diff] [review]
Implement <ol reversed>.
Comment 9 Mounir Lamouri (:mounir) 2012-06-28 09:05:16 PDT
Boris, could you update content/html/content/test/test_ol_attributes_reflection.html?
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-28 09:20:37 PDT
It's in the diff!
Comment 11 Mounir Lamouri (:mounir) 2012-06-28 09:35:43 PDT
(In reply to Boris Zbarsky (:bz) from comment #10)
> It's in the diff!

Damn! I checked but haven't seen it :( I will double-check next time!
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-28 09:38:33 PDT
What handles renumbering when new items are appended?  Do you have tests for that?
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-28 09:53:02 PDT
There's a RenumberLists() call in nsBlockFrame::Reflow that handles it.

I'll add a test for this.
Comment 14 Gordon P. Hemsley [:GPHemsley] 2012-06-28 09:55:06 PDT
> layout/reftests/bugs/601912-2-ref.html
> ...
> <li value="3"og.>Three</li>

Is this a typo?
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-28 09:55:14 PDT
Created attachment 637559 [details] [diff] [review]
With a test for appending
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-28 10:06:40 PDT
> Is this a typo?

Yes, fixed.  Doesn't affect anything, of course.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-08 11:30:27 PDT
Created attachment 640083 [details] [diff] [review]
With some assertions also fixed so this passes tests
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-14 00:10:23 PDT
Created attachment 642184 [details] [diff] [review]
Implement <ol reversed>.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-29 18:58:06 PDT
Comment on attachment 642184 [details] [diff] [review]
Implement <ol reversed>.

Hoping it's ok if I transfer this review request to you; if not, send it back.
Comment 20 Daniel Holbert [:dholbert] 2012-09-07 14:41:42 PDT
bz, would you mind posting an unbitrotted version of this?

(Planning to review this in the next couple of days -- ideally, I'd prefer to review & try running with an up-to-date version.  The posted patch was from before the stdint & nullptr switchovers, I believe, so it doesn't apply at the moment. Not sure if there's other bitrot beyond that, as well.)
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-07 14:49:42 PDT
Created attachment 659366 [details] [diff] [review]
With the various mass-changes applied

Sure thing!
Comment 22 Daniel Holbert [:dholbert] 2012-09-07 15:27:29 PDT
Comment on attachment 659366 [details] [diff] [review]
With the various mass-changes applied

>+++ b/layout/generic/nsBlockFrame.cpp
>@@ -2782,17 +2782,17 @@ nsBlockFrame::AttributeChanged(int32_t  
>                                int32_t         aModType)
> {
[...]
>-  if (nsGkAtoms::start == aAttribute) {
>+  if (nsGkAtoms::start == aAttribute || nsGkAtoms::reversed == aAttribute) {

This probably wants to check Tag(), too, something like this:
  if (nsGkAtoms::start == aAttribute || 
      (nsGkAtoms::reversed == aAttribute && nsGkAtoms::ol == mContent->Tag()) {

since reversed only has an effect on <ol> elements -- right?  (Later in this patch, you check Tag() in the same spot where you check for reversed)

(I don't imagine this will affect behavior, but it should just save us some unnecessary notifications)

>@@ -2782,17 +2782,17 @@ nsBlockFrame::AttributeChanged(int32_t  
>                                int32_t         aModType)
>   nsGenericHTMLElement *hc = nsGenericHTMLElement::FromContent(mContent);
>-
>-  if (hc) {
>-    const nsAttrValue* attr = hc->GetParsedAttr(nsGkAtoms::start);
[...]
>+  // Must be non-null, since FrameStartsCounterScope only returns true
>+  // for HTML elements.
>+  const nsAttrValue* attr = hc->GetParsedAttr(nsGkAtoms::start);

Maybe add a MOZ_ASSERT(hc, ...) there to document what that comment's claiming?  Then if FrameStartsCounterScope() gets a bug at some point, we'll get a helpful assertion-failure instead of a mysterious segfault.

>+  if (attr && attr->Type() == nsAttrValue::eInteger) {
>+    ordinal = attr->GetIntegerValue();
>+  } else if (increment == -1) {
>+    // <ol reversed> case: count up the child lis

s/lis/list/

>+    ordinal = 0;
>+    for (nsIContent* kid = mContent->GetFirstChild(); kid;
>+         kid = kid->GetNextSibling()) {
>+      if (kid->IsHTML(nsGkAtoms::li)) {
>+        ++ordinal;

I wonder if we might even want to make this "ordinal -= increment" instead of "++ordinal"? (and check "increment < 0" instead of "== -1" in the "else if" check above it)

That way, abritrary increment values should Just Work (e.g. if the WHATWG introduces "<ol increment="3" reversed>" at some point in the future)

That could be premature generalization, though -- the existing code is good too.

> int32_t
> nsBulletFrame::SetListItemOrdinal(int32_t aNextOrdinal,
[...]
>-  return mOrdinal + 1;
>+  return mOrdinal + aIncrement;
> }

Might be worth asserting that aIncrement is either 1 or -1 here, just as a sanity-check, since (for now) those are the only values that we're expecting to add.

>+++ b/layout/reftests/bugs/601912-2.html
>@@ -0,0 +1,6 @@
>+<!DOCTYPE html>
>+<ol reversed start="5">
>+  <li>Five</li>
>+  <li>Four</li>
>+  <li>Three</li>
>+</ol>

It's probably worth having a test where we wrap past 0 into negative territory, too -- like this test, but with start="1" maybe?

Also, since this is actual feature-work (rather than a random-bugfix), these tests probably deserve "real" names instead of "601912-*", and they probably want to live in layout/reftests/list-item/ instead of layout/reftests/bugs.

Also: I noticed that http://blog.whatwg.org/reverse-ordered-lists mentions that XHTML needs this to be specified as <ol reversed="reversed"> -- does that work right now?  (and do we reject other values for the "reversed" attribute in XHTML, as it sounds like we should?)

It'd probably be good to add XHTML reftests showing that reversed="reversed" works and reversed="anything_else" is ignored.

r=me with the above addressed.
Comment 23 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-07 19:13:34 PDT
> mentions that XHTML needs this to be specified as <ol reversed="reversed">

That's an authoring conformance requirement because <ol reversed> is not valid XML.

Technically, in HTML (non-XHTML) there are only two valid ways of writing this: <ol reversed> and <ol reversed="reversed">.  Again, as an authoring conformance requirement.

The UA conformance requirement is the same for HTML and XHTML, and is given at http://www.whatwg.org/specs/web-apps/current-work/multipage/grouping-content.html#attr-ol-reversed and following.  Note that all that text talks just about whether the attribute is set at all, not what values it's set to.  That's just how boolean attributes work in HTML.

I addressed the rest of the comments.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-07 19:31:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f083c0a666d3
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-09-08 08:34:02 PDT
https://hg.mozilla.org/mozilla-central/rev/f083c0a666d3
Comment 27 :Ms2ger 2012-09-15 12:40:57 PDT
(In reply to Boris Zbarsky (:bz) from comment #23)
> > mentions that XHTML needs this to be specified as <ol reversed="reversed">
> 
> That's an authoring conformance requirement because <ol reversed> is not
> valid XML.
> 
> Technically, in HTML (non-XHTML) there are only two valid ways of writing
> this: <ol reversed> and <ol reversed="reversed">.  Again, as an authoring
> conformance requirement.

(To be pedantic: <ol reversed=''> is valid in both HTML and XML.)

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