Bug 229083

<rdar://problem/82176885>

It looks like it's hidden behind ENABLE(CSS3_TEXT), so not sure it's even available for most people?

Created attachment 446814 [details] Patch

Comment on attachment 446814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446814&action=review Do we have any actual functional tests, above and beyond these tests that checking parsing/serialization? > LayoutTests/imported/w3c/web-platform-tests/css/css-text/parsing/text-align-last-computed-expected.txt:9 > +FAIL Property text-align-last value 'match-parent' assert_equals: expected "match-parent" but got "auto" This seems bad? Is this just serialisation, or do we at least get the semantics right?

Comment on attachment 446814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446814&action=review >> LayoutTests/imported/w3c/web-platform-tests/css/css-text/parsing/text-align-last-computed-expected.txt:9 >> +FAIL Property text-align-last value 'match-parent' assert_equals: expected "match-parent" but got "auto" > > This seems bad? Is this just serialisation, or do we at least get the semantics right? I'm not sure if the test is correct. The spec says that 'match-parent' should match the immediate parent (which we don't have in this test), or the initial containing block which does not have a value specified for the direction value. That makes it sound like it should either be 'left', or maybe the default "auto" value. I don't think the computed value should ever be "match-parent".

Comment on attachment 446814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446814&action=review >>> LayoutTests/imported/w3c/web-platform-tests/css/css-text/parsing/text-align-last-computed-expected.txt:9 >>> +FAIL Property text-align-last value 'match-parent' assert_equals: expected "match-parent" but got "auto" >> >> This seems bad? Is this just serialisation, or do we at least get the semantics right? > > I'm not sure if the test is correct. The spec says that 'match-parent' should match the immediate parent (which we don't have in this test), or the initial containing block which does not have a value specified for the direction value. That makes it sound like it should either be 'left', or maybe the default "auto" value. I don't think the computed value should ever be "match-parent". See https://github.com/w3c/csswg-drafts/issues/2577 I don't think the outcome was particularly clear unfortunately. Might be worth pinging frivoal or fantasai.

Comment on attachment 446814 [details] Patch Do those WPTs start passing: https://webkit-search.igalia.com/webkit/rev/c7aee49ebf38fcbd12c681b28cc2e877d356a1e9/LayoutTests/TestExpectations#2671-2673 ?

Comment on attachment 446814 [details] Patch Can you unskip https://webkit-search.igalia.com/webkit/rev/c7aee49ebf38fcbd12c681b28cc2e877d356a1e9/LayoutTests/TestExpectations#2956 as well?

(In reply to Tim Nguyen (:ntim) from comment #7) > Comment on attachment 446814 [details] > Patch > > Do those WPTs start passing: > https://webkit-search.igalia.com/webkit/rev/ > c7aee49ebf38fcbd12c681b28cc2e877d356a1e9/LayoutTests/TestExpectations#2671- > 2673 > ? Looks like the WPT script adds the -webkit- prefix automatically to -webkit-text-align-last in those tests, can you please remove the now unused prefixes? See: https://webkit-search.igalia.com/webkit/search?q=-webkit-text-align-last&path=LayoutTests&case=false&regexp=false

See also: https://webkit-search.igalia.com/webkit/search?q=webkitTextAlignLast&path=LayoutTests&case=false&regexp=false

Comment on attachment 446814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446814&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:768 > -#if ENABLE(CSS3_TEXT) > || first.textAlignLast != second.textAlignLast > +#if ENABLE(CSS3_TEXT) Is text-align-last no longer part of CSS3 Text?

Comment on attachment 446814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446814&action=review > Source/WebInspectorUI/UserInterface/External/CSSDocumentation/CSSDocumentation.js:2360 > + "syntax": "auto | start | end | left | right | center | justify | match-parent", Please don't modify `CSSDocumentation.js` directly. It gets automatically generated from upstream data by running the script: `Source/WebInspectorUI/Scripts/update-inspector-css-documentation` To update a field, you can add an entry to `Source/WebInspectorUI/UserInterface/External/CSSDocumentation/CSSDocumentation-overrides.json` which will **overwrite** the corresponding field from the data source: ``` "text-align-last": { "syntax": "auto | start | end | left | right | center | justify | match-parent" } ``` Then run the script `Source/WebInspectorUI/Scripts/update-inspector-css-documentation` to generate the updated `CSSDocumentation.js` file. Alternatively, you can file an issue / send a PR to the upstream data source at https://github.com/microsoft/vscode-custom-data After it gets merged, on the next manual run of the script, the changes will get picked up automatically.

You should consider adding animation support for this property in CSSPropertyAnimation. See the many uses of DiscretePropertyWrapper, this will likely be a one-liner.

Comment on attachment 446814 [details] Patch r- to remove from queue given all the current comments + EWS status.

Pull request: https://github.com/WebKit/WebKit/pull/1145

Committed r295021 (251116@main): <https://commits.webkit.org/251116@main> Reviewed commits have been landed. Closing PR #1145 and removing active labels.