1144607 - Support string value on list-style-type

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - remove BuildCounterStyle for anonymous counter styles

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - handle string value on list-style-type

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - reftests

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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.

[1] https://lists.w3.org/Archives/Public/www-style/2015Mar/0290.html
[2] https://lists.w3.org/Archives/Public/www-style/2015Mar/0304.html

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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="mailto:[e-mail]">
> +  <style>
> +    ol, ul { list-style: none; }
> +    li::before { content: "# "; }
> +    span { line-height: 0; }

This line is useless. I've removed it locally.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

nsRuleNode.cpp:

>+      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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

r=dbaron

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=412eece8f3e7

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Ah... Why does "font-variant-numeric" also affect symbols and space characters...

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Adding "font-variant-numeric: tabular-nums;" to the ref makes it pass the test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45fd90e05b89

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: https://lists.w3.org/Archives/Public/www-style/2015Mar/0389.html

If the editors accept this, I'll continue landing the current patches.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00b852893926
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3c37c82aa0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8e79ee4339

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00b852893926
https://hg.mozilla.org/mozilla-central/rev/0a3c37c82aa0
https://hg.mozilla.org/mozilla-central/rev/3b8e79ee4339

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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)]: https://developer.mozilla.org/en-US/docs/Web/CSS/list-style-type

(Assuming that link is where developer documentation will be added)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://test.csswg.org/shepherd/testcase/list-style-type-string-001a/
http://test.csswg.org/shepherd/testcase/list-style-type-string-001b/
have errors because of "ref" rather than "href".

Linked from either:
http://test.csswg.org/shepherd/search/owner/dbaron/status/issue/
http://test.csswg.org/shepherd/search/author/upsuper/status/issue/

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/605e54c0e2c3

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/605e54c0e2c3

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.csswg.org/test/rev/6a0119d22f8f

Comment 18

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/39
and
https://developer.mozilla.org/en-US/docs/Web/CSS/list-style-type