Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 9 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 4 • 9 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 5 • 9 years ago

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:quanxunzhen@gmail.com">
> +  <style>
> +    ol, ul { list-style: none; }
> +    li::before { content: "# "; }
> +    span { line-height: 0; }

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 6 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 7 • 9 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 8 • 9 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 9 • 9 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 10 • 9 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 11 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 9 years ago

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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 13 • 9 years ago

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)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 14 • 9 years ago

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/

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 15 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 16 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 17 • 9 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 18 • 9 years ago

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