Bug 167729 – Support string values on list-style-type

• Component: CSS
• Status: RESOLVED FIXED

[Ebrahim Byagowi]

Testcase:
data:text/html,<ul style="list-style-type: '-'"><li></li><li></li</ul>

Expected:
Like Firefox,
 -
 -

Actual:
 *
 *

It seems list-style-type doesn't support string values, which per http://crbug.com/687225#c5 seems needed for better Russian support

[Liam Quin]

See also bug 167645 for more context.

[Sierk Bornemann]

Any progress with fixing this issue?

[Oriol Brufau]

I have implemented this in Blink, I can port it.
However, I haven't added support for mixed-bidi in markers (in Blink it only works thanks to LayoutNG).

[Oriol Brufau]

Created attachment 380753 [details]
Patch

[Oriol Brufau]

Created attachment 380754 [details]
Patch

[Oriol Brufau]

Created attachment 380844 [details]
Patch

[Oriol Brufau]

Created attachment 380886 [details]
Patch

[Oriol Brufau]

Created attachment 382187 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 382187 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382187&action=review

> Source/WebCore/ChangeLog:21
> +        Tests: imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-002.html
> +               imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-003.html
> +               imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-004.html
> +               imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-005a.html
> +               imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-005b.html
> +               imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-006.html
> +               imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-007.html

Maybe update the css-lists directory wholesale in a first patch, then do the bug fix?

[Oriol Brufau]

Created attachment 382211 [details]
Patch

[Oriol Brufau]

(In reply to Simon Fraser (smfr) from comment #9)
> Maybe update the css-lists directory wholesale in a first patch, then do the
> bug fix?

Seems reasonable, but before splitting I want to ensure that EWS are completely green

[Oriol Brufau]

Created attachment 382213 [details]
Patch

[Oriol Brufau]

Created attachment 382681 [details]
Patch

[Oriol Brufau]

(In reply to Simon Fraser (smfr) from comment #9)
> Maybe update the css-lists directory wholesale in a first patch, then do the
> bug fix?

Done. The tests will be imported in bug 203759.

[Oriol Brufau]

Created attachment 382768 [details]
Patch

[Oriol Brufau]

I have imported the tests in other bugs, and EWS is green, so it would be great if someone could review.

[Antti Koivisto]

Comment on attachment 382768 [details]
Patch

r=me

[Simon Fraser (smfr)]

Comment on attachment 382768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382768&action=review

> Source/WebCore/rendering/style/RenderStyleConstants.h:635
> +    None,
> +    String

Seems odd not to keep None at the end.

[WebKit Commit Bot]

Comment on attachment 382768 [details]
Patch

Clearing flags on attachment: 382768

Committed r252076: <https://trac.webkit.org/changeset/252076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 382768 [details]
Patch

Please fix the enum ordering.

[Radar WebKit Bug Importer]

<rdar://problem/56917080>

[Matt Lewis]

Reverted r252076 for reason:

This broke internal builds and tests. For more information discuss with your reviewers.

Committed r252336: <https://trac.webkit.org/changeset/252336>

[Simon Fraser (smfr)]

Error is:
./rendering/style/StyleRareInheritedData.cpp:74:1: error: static_assert failed due to requirement 'sizeof(WebCore::StyleRareInheritedData) <= sizeof(WebCore::GreaterThanOrSameSizeAsStyleRareInheritedData)' "StyleRareInheritedData_should_bit_pack"

[Oriol Brufau]

(In reply to Simon Fraser (smfr) from comment #24)
> Error is:
> ./rendering/style/StyleRareInheritedData.cpp:74:1: error: static_assert
> failed due to requirement 'sizeof(WebCore::StyleRareInheritedData) <=
> sizeof(WebCore::GreaterThanOrSameSizeAsStyleRareInheritedData)'
> "StyleRareInheritedData_should_bit_pack"

Strange. I got this error on Windows for an early version of the patch, but after I increased the number of AtomString in GreaterThanOrSameSizeAsStyleRareInheritedData, EWS was green.
For the final version of the patch, the win bot had unrelated failures, but the patch compiled fine. So the static_assert passed.

[Simon Fraser (smfr)]

The padding must have changed somehow. Try playing with ./Tools/Scripts/dump-class-layout on a release build.

[Oriol Brufau]

(In reply to Simon Fraser (smfr) from comment #26)
> The padding must have changed somehow. Try playing with
> ./Tools/Scripts/dump-class-layout on a release build.

When I run
> ./Tools/Scripts/dump-class-layout WebCore StyleRareInheritedData
I get an error:
> Failed to make target for /home/oriol/src/WebKit/WebKitBuild/Release/WebCore.framework/WebCore
> Failed to get first module in /home/oriol/src/WebKit/WebKitBuild/Release/WebCore.framework/WebCore

Anyways, I have printed
> sizeof(StyleRareInheritedData)
> sizeof(GreaterThanOrSameSizeAsStyleRareInheritedData)

Before reverting my patch, the values were 280 and 288.
After reverting my patch, they became 272 and 280.

This is expected since I added an AtomString to both of them.

So I don't understand how the assert failed
> COMPILE_ASSERT(sizeof(StyleRareInheritedData) <= sizeof(GreaterThanOrSameSizeAsStyleRareInheritedData), StyleRareInheritedData_should_bit_pack);

[Oriol Brufau]

Created attachment 410348 [details]
Patch

[Oriol Brufau]

This is basically a rebase of the patch that I landed the other time (plus bug 203883).

It was reverted because a padding problem in GreaterThanOrSameSizeAsStyleRareInheritedData that made an static_assert fail.

I haven't addressed that directly, but the bitfields in GreaterThanOrSameSizeAsStyleRareInheritedData have been modified e.g. in bug 212030, and I hope this might avoid the problem now.

Does it sound good to try to land this again?

[Oriol Brufau]

Created attachment 410395 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Oriol Brufau]

Created attachment 410421 [details]
Patch

[Darin Adler]

Comment on attachment 410421 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410421&action=review

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3884
> +        // Some properties need to fallback onto the regular parser.

The verb "fall back" is two words.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4343
> +        // NOTE: All the keyword values for the list-style-type property are handled by the CSSParserFastPaths.

Leave out "NOTE:" unless there is some reason to include it. What comment is not a note?

> Source/WebCore/rendering/RenderListMarker.cpp:1803
> +        return ""_str;

emptyString(), not ""_str.

> Source/WebCore/rendering/style/RenderStyle.h:1002
> +    void setListStyleStringValue(const AtomString& s) { SET_VAR(m_rareInheritedData, listStyleStringValue, s); }

How about the word 'value" instead of the letter "s"?

> LayoutTests/platform/mac/TestExpectations:2234
> +# These tests fail in Mac due to subpixel differences for the marker position
> +imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-002.html [ ImageOnlyFailure ]
> +imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-004.html [ ImageOnlyFailure ]

We should find a way to resolve this. It’s unfortunate to not have test coverage.

[Oriol Brufau]

Created attachment 410443 [details]
Patch

[Oriol Brufau]

(In reply to Darin Adler from comment #33)
> The verb "fall back" is two words.

Done.

> Leave out "NOTE:" unless there is some reason to include it. What comment is
> not a note?

You are right, removed. I just copied the comment that Chromium had in the display property.

> emptyString(), not ""_str.

Done.

> How about the word 'value" instead of the letter "s"?

Various functions around there use a single letter, but OK, done.

> > LayoutTests/platform/mac/TestExpectations:2234
> > +# These tests fail in Mac due to subpixel differences for the marker position
> > +imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-002.html [ ImageOnlyFailure ]
> > +imported/w3c/web-platform-tests/css/css-lists/list-style-type-string-004.html [ ImageOnlyFailure ]
> 
> We should find a way to resolve this. It’s unfortunate to not have test
> coverage.

Note that when I noticed this problem, I added list-style-type-string-007.html, which uses the Ahem to ensure things align properly.
I will file a follow-up bug, though.

[EWS]

Committed r267940: <https://trac.webkit.org/changeset/267940>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410443 [details].

[Hector Lopez]

REGRESSION(r267940): [ macOS Catalina ] imported/w3c/web-platform-tests/css/css-pseudo/text-selection.html is a constant failure

Test is a constant failure according to history on macOS Catalina testers. First occurrence of failures at r267940.

History:

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-pseudo%2Ftext-selection.html&version_name=Catalina

Diff:

--- /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/css/css-pseudo/text-selection-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/css/css-pseudo/text-selection-actual.txt
@@ -7,5 +7,5 @@
 FAIL Selection ending in ::marker assert_equals: toString expected "hello" but got ""
 PASS Selection contained in ::marker
 FAIL Selection ending in ::before-marker assert_equals: toString expected "hello" but got ""
-FAIL Selection contained in ::before-marker assert_equals: toString expected "" but got "h"
+PASS Selection contained in ::before-marker
 

Was able to reproduce on r267940 with:

run-webkit-tests imported/w3c/web-platform-tests/css/css-pseudo/text-selection.html --iterations 1000 --exit-after-n-failures 3

Test expectation while investigated:
https://trac.webkit.org/changeset/268168/webkit

[Hector Lopez]

Rebaseline of test imported/w3c/web-platform-tests/css/css-pseudo/text-selection.html:

https://trac.webkit.org/changeset/268171/webkit

[Aakash Jain]

(In reply to Hector Lopez from comment #38)
> Rebaseline of test
> imported/w3c/web-platform-tests/css/css-pseudo/text-selection.html:
> 
> https://trac.webkit.org/changeset/268171/webkit
This seems to have broken this test on mac-debug-wk1 (and is affecting that EWS queue)
History: https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-pseudo%2Ftext-selection.html

[Oriol Brufau]

Filed bug 217475 regarding comment 37 and comment 39.

Note the test already had problems before my patch (bug 217161).

[Hector Lopez]

imported/w3c/web-platform-tests/css/css-pseudo/text-selection.html

Test is failing on Mojave testers after re-baseline according to history. Will change test expectation while being investigated.

History:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-pseudo%2Ftext-selection.html&platform=mac

Test expectation correction:

https://trac.webkit.org/changeset/268184/webkit

[Hector Lopez]

Test result added for Mojave specific result:

https://trac.webkit.org/changeset/268233/webkit

[Oriol Brufau]

Marking as fixed again since the patch landed and has not been reverted.
Problems with the text-selection.html test, which was already flaky before this, can be addressed in bug 217161.