Bug 167729 - Support string values on list-style-type
Summary: Support string values on list-style-type
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar, WebExposed
Depends on: 203759 203807 217475
Blocks: 202849 203883
  Show dependency treegraph
 
Reported: 2017-02-02 05:09 PST by Ebrahim Byagowi
Modified: 2020-10-08 17:28 PDT (History)
26 users (show)

See Also:


Attachments
Patch (59.37 KB, patch)
2019-10-11 07:27 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (59.29 KB, patch)
2019-10-11 07:56 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (60.24 KB, patch)
2019-10-13 08:41 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (60.96 KB, patch)
2019-10-14 09:39 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (67.06 KB, patch)
2019-10-29 09:26 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (67.76 KB, patch)
2019-10-29 13:03 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (67.51 KB, patch)
2019-10-29 13:18 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (29.61 KB, patch)
2019-11-02 12:05 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (34.19 KB, patch)
2019-11-04 13:35 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (41.90 KB, patch)
2020-10-02 11:51 PDT, Oriol Brufau
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.98 KB, patch)
2020-10-02 18:06 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (46.20 KB, patch)
2020-10-03 07:39 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (46.24 KB, patch)
2020-10-03 15:29 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ebrahim Byagowi 2017-02-02 05:09:19 PST
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
Comment 1 Liam Quin 2018-03-07 20:16:26 PST
See also bug 167645 for more context.
Comment 2 Sierk Bornemann 2019-08-22 12:58:18 PDT
Any progress with fixing this issue?
Comment 3 Oriol Brufau 2019-10-11 06:50:07 PDT
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).
Comment 4 Oriol Brufau 2019-10-11 07:27:35 PDT
Created attachment 380753 [details]
Patch
Comment 5 Oriol Brufau 2019-10-11 07:56:26 PDT
Created attachment 380754 [details]
Patch
Comment 6 Oriol Brufau 2019-10-13 08:41:59 PDT
Created attachment 380844 [details]
Patch
Comment 7 Oriol Brufau 2019-10-14 09:39:59 PDT
Created attachment 380886 [details]
Patch
Comment 8 Oriol Brufau 2019-10-29 09:26:27 PDT
Created attachment 382187 [details]
Patch
Comment 9 Simon Fraser (smfr) 2019-10-29 10:50:55 PDT
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?
Comment 10 Oriol Brufau 2019-10-29 13:03:22 PDT
Created attachment 382211 [details]
Patch
Comment 11 Oriol Brufau 2019-10-29 13:08:13 PDT
(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
Comment 12 Oriol Brufau 2019-10-29 13:18:03 PDT
Created attachment 382213 [details]
Patch
Comment 13 Oriol Brufau 2019-11-02 12:05:12 PDT
Created attachment 382681 [details]
Patch
Comment 14 Oriol Brufau 2019-11-02 12:07:06 PDT
(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.
Comment 15 Oriol Brufau 2019-11-04 13:35:38 PST
Created attachment 382768 [details]
Patch
Comment 16 Oriol Brufau 2019-11-05 01:57:45 PST
I have imported the tests in other bugs, and EWS is green, so it would be great if someone could review.
Comment 17 Antti Koivisto 2019-11-05 12:09:32 PST
Comment on attachment 382768 [details]
Patch

r=me
Comment 18 Simon Fraser (smfr) 2019-11-05 13:37:19 PST
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.
Comment 19 WebKit Commit Bot 2019-11-05 13:41:55 PST
Comment on attachment 382768 [details]
Patch

Clearing flags on attachment: 382768

Committed r252076: <https://trac.webkit.org/changeset/252076>
Comment 20 WebKit Commit Bot 2019-11-05 13:41:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Simon Fraser (smfr) 2019-11-05 13:42:08 PST
Comment on attachment 382768 [details]
Patch

Please fix the enum ordering.
Comment 22 Radar WebKit Bug Importer 2019-11-05 13:42:15 PST
<rdar://problem/56917080>
Comment 23 Matt Lewis 2019-11-11 11:45:42 PST
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>
Comment 24 Simon Fraser (smfr) 2019-11-11 11:48:44 PST
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"
Comment 25 Oriol Brufau 2019-11-11 11:58:14 PST
(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.
Comment 26 Simon Fraser (smfr) 2019-11-11 13:17:38 PST
The padding must have changed somehow. Try playing with ./Tools/Scripts/dump-class-layout on a release build.
Comment 27 Oriol Brufau 2019-11-12 06:34:18 PST
(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);
Comment 28 Oriol Brufau 2020-10-02 11:51:49 PDT
Created attachment 410348 [details]
Patch
Comment 29 Oriol Brufau 2020-10-02 12:00:33 PDT
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?
Comment 30 Oriol Brufau 2020-10-02 18:06:36 PDT
Created attachment 410395 [details]
Patch
Comment 31 EWS Watchlist 2020-10-02 18:07:44 PDT
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
Comment 32 Oriol Brufau 2020-10-03 07:39:32 PDT
Created attachment 410421 [details]
Patch
Comment 33 Darin Adler 2020-10-03 14:08:52 PDT
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.
Comment 34 Oriol Brufau 2020-10-03 15:29:24 PDT
Created attachment 410443 [details]
Patch
Comment 35 Oriol Brufau 2020-10-03 15:38:51 PDT
(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.
Comment 36 EWS 2020-10-03 18:58:55 PDT
Committed r267940: <https://trac.webkit.org/changeset/267940>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410443 [details].
Comment 37 Hector Lopez 2020-10-07 19:11:40 PDT
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
Comment 38 Hector Lopez 2020-10-07 21:57:58 PDT
Rebaseline of test imported/w3c/web-platform-tests/css/css-pseudo/text-selection.html:

https://trac.webkit.org/changeset/268171/webkit
Comment 39 Aakash Jain 2020-10-08 06:37:05 PDT
(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
Comment 40 Oriol Brufau 2020-10-08 07:39:37 PDT
Filed bug 217475 regarding comment 37 and comment 39.

Note the test already had problems before my patch (bug 217161).
Comment 41 Hector Lopez 2020-10-08 09:15:08 PDT
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
Comment 42 Hector Lopez 2020-10-08 17:28:25 PDT
Test result added for Mojave specific result:

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