Closed Bug 1144607 Opened 9 years ago Closed 9 years ago

Support string value on list-style-type


(Core :: Layout, defect)

Not set



Tracking Status
firefox39 --- fixed
relnote-firefox --- 39+


(Reporter: xidorn, Assigned: xidorn)




(Keywords: dev-doc-complete)


(3 files)

CSS List 3 extends the syntax of list-style-type (as well as its shorthand property list-style) to accept a string value. It should be quite easy for use to implement since we have implemented CSS Counter Styles, and the string value is just a special case in cyclic system.
Assignee: nobody → quanxunzhen
Attachment #8579717 - Flags: review?(dbaron)
Attachment #8579721 - Flags: review?(dbaron)
According to Simon Sapin [1] and Tab Atkins Jr. [2], this part of the spec should be stable enough to be implemented. And given we have implemented the Counter Styles module, it doesn't cost much code for us to add support for this feature. For these reasons, I don't think we need any pref for it. I'm not sure whether it is worth a "Intend to ship", but I guess not.

Keywords: dev-doc-needed
Comment on attachment 8579721 [details] [diff] [review]
patch 3 - reftests

Review of attachment 8579721 [details] [diff] [review]:

::: layout/reftests/w3c-css/submitted/lists-3/list-style-type-string-001-ref.html
@@ +6,5 @@
> +  <link rel="author" title="Xidorn Quan" href="">
> +  <style>
> +    ol, ul { list-style: none; }
> +    li::before { content: "# "; }
> +    span { line-height: 0; }

This line is useless. I've removed it locally.
Attachment #8579717 - Flags: review?(dbaron) → review+
Comment on attachment 8579720 [details] [diff] [review]
patch 2 - handle string value on list-style-type

Maybe it's better for AnonymousCounterStyle::GetSuffix to call
IsSingleString() rather than checking mSingleString directly?


>+      nsString content;
>+      typeValue->GetStringValue(content);
>+      list->SetListStyleType(content, new AnonymousCounterStyle(content));

"content" seems like a bad name for this variable; such a name
would usually refer to an nsIContent*.  Maybe just "str"?

Also, I'm curious why you pass content/str as the first argument
to SetListStyleType rather than passing NS_LITERAL_STRING("")
as you do for symbols().  It doesn't look like you depend on it
anywhere, but it seems like it might make more sense to use the
empty string here.

r=dbaron with that
Attachment #8579720 - Flags: review?(dbaron) → review+
Comment on attachment 8579721 [details] [diff] [review]
patch 3 - reftests

I guess it's the span { line-height: 0; } that you said you will remove?

Attachment #8579721 - Flags: review?(dbaron) → review+
Ah... Why does "font-variant-numeric" also affect symbols and space characters...
Adding "font-variant-numeric: tabular-nums;" to the ref makes it pass the test:

But I wonder whether we can add it in a test submitted to W3C. I've sent an email to www-style to propose adding this rule to ::marker by default to make the spec match what we currently do:

If the editors accept this, I'll continue landing the current patches.
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Release Note Request (optional, but appreciated)
[Why is this notable]: A useful layout change for developers
[Suggested wording]:  List-style-type now accepts a string value
[Links (documentation, blog post, etc)]:

(Assuming that link is where developer documentation will be added)
You need to log in before you can comment on or make changes to this bug.