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
Depends on: 203759 203807
Blocks: 202849 203883
  Show dependency treegraph
 
Reported: 2017-02-02 05:09 PST by Ebrahim Byagowi
Modified: 2019-11-12 06:34 PST (History)
19 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

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);