Closed Bug 601912 Opened 14 years ago Closed 12 years ago

HTML 5's <ol> reversed attribute not supported

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: nunoplopes, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, html5, Whiteboard: [parity-webkit])

Attachments

(3 files, 4 obsolete files)

Test case: OL lists with start, value, and reversed attributes
1.37 KB, text/html
Details
Test case: changing start value or count order with CSS
2.69 KB, text/html
Details
With the various mass-changes applied
15.81 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
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
<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
Confirming: not implemented in FF4b10.
Firefox 4 implements: - <ol start=integer> (with positive and negative integers), - <li value=integer> (likewise). Doesn't implement: - <ol reversed>
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.
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...
Attached patch Implement <ol reversed>. (obsolete) — — Splinter Review
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?
Attached patch With a test for appending (obsolete) — — Splinter Review
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.
Attachment #640083 - Flags: review?(dbaron)
Attachment #637559 - Attachment is obsolete: true
Attachment #637559 - Flags: review?(dbaron)
Blocks: html
Attached patch Implement <ol reversed>. (obsolete) — — Splinter Review
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.)
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
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
(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.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: