HTML 5's <ol> reversed attribute not supported

RESOLVED FIXED in mozilla18

Status

()

Core
Layout
--
enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Nuno Lopes, Assigned: bz)

Tracking

(Blocks: 2 bugs, {dev-doc-complete, html5})

Trunk
mozilla18
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-webkit])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 2

7 years ago
<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
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.
Summary: HTML 5's <li> reversed attribute ignored → HTML 5's <li> reversed attribute not supported

Comment 4

6 years ago
Confirming: not implemented in FF4b10.

Comment 5

6 years ago
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

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

Updated

6 years ago
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: html5
Summary: HTML 5's <li> reversed attribute not supported → HTML 5's <ol> reversed attribute not supported
Version: unspecified → Trunk
Whiteboard: [parity-webkit]
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...
Created attachment 637531 [details] [diff] [review]
Implement <ol reversed>.
Attachment #637531 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Whiteboard: [parity-webkit] → [need review][parity-webkit]
Boris, could you update content/html/content/test/test_ol_attributes_reflection.html?
It's in the diff!
(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!
What handles renumbering when new items are appended?  Do you have tests for that?
There's a RenumberLists() call in nsBlockFrame::Reflow that handles it.

I'll add a test for this.
> layout/reftests/bugs/601912-2-ref.html
> ...
> <li value="3"og.>Three</li>

Is this a typo?
Created attachment 637559 [details] [diff] [review]
With a test for appending
Attachment #637559 - Flags: review?(dbaron)
Attachment #637531 - Attachment is obsolete: true
Attachment #637531 - Flags: review?(dbaron)
> Is this a typo?

Yes, fixed.  Doesn't affect anything, of course.
Created attachment 640083 [details] [diff] [review]
With some assertions also fixed so this passes tests
Attachment #640083 - Flags: review?(dbaron)
Attachment #637559 - Attachment is obsolete: true
Attachment #637559 - Flags: review?(dbaron)

Updated

5 years ago
Blocks: 568516
Created attachment 642184 [details] [diff] [review]
Implement <ol reversed>.
Attachment #642184 - Flags: review?(dbaron)
Attachment #640083 - Attachment is obsolete: true
Attachment #640083 - Flags: review?(dbaron)
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.
Attachment #642184 - Flags: review?(dbaron) → review?(dholbert)
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.)
Created attachment 659366 [details] [diff] [review]
With the various mass-changes applied

Sure thing!
Attachment #659366 - Flags: review?(dholbert)
Attachment #642184 - Attachment is obsolete: true
Attachment #642184 - Flags: review?(dholbert)
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.
Attachment #659366 - Flags: review?(dholbert) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f083c0a666d3
Flags: in-testsuite+
Whiteboard: [need review][parity-webkit] → [parity-webkit]
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/f083c0a666d3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: dev-doc-needed
Updated:
https://developer.mozilla.org/en-US/docs/HTML/Element/ol
and
https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers
Keywords: dev-doc-needed → dev-doc-complete
(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.)
Blocks: 802882
You need to log in before you can comment on or make changes to this bug.