Bug 229083 - Unprefix -webkit-text-align-last
Summary: Unprefix -webkit-text-align-last
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on:
Blocks: 217522 146772
  Show dependency treegraph
 
Reported: 2021-08-13 10:53 PDT by Sam Sneddon [:gsnedders]
Modified: 2022-05-30 01:29 PDT (History)
19 users (show)

See Also:


Attachments
Patch (29.43 KB, patch)
2021-12-10 14:25 PST, Brent Fulgham
ntim: review-
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Sneddon [:gsnedders] 2021-08-13 10:53:51 PDT
I don't think there's been much in the way of changes here, so this should be a simple change.
Comment 1 Radar WebKit Bug Importer 2021-08-20 10:54:22 PDT
<rdar://problem/82176885>
Comment 2 Brent Fulgham 2021-12-09 16:58:20 PST
It looks like it's hidden behind ENABLE(CSS3_TEXT), so not sure it's even available for most people?
Comment 3 Brent Fulgham 2021-12-10 14:25:45 PST
Created attachment 446814 [details]
Patch
Comment 4 Sam Sneddon [:gsnedders] 2021-12-10 14:57:26 PST
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 5 Brent Fulgham 2021-12-10 15:06:28 PST
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 6 Tim Nguyen (:ntim) 2021-12-10 16:33:11 PST
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 9 Tim Nguyen (:ntim) 2021-12-10 16:41:12 PST
(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
Comment 11 Dean Jackson 2022-01-13 09:35:55 PST
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 12 Razvan Caliman 2022-01-13 11:13:29 PST
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.
Comment 13 Antoine Quint 2022-03-08 08:42:48 PST
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 14 Tim Nguyen (:ntim) 2022-03-25 12:22:49 PDT
Comment on attachment 446814 [details]
Patch

r- to remove from queue given all the current comments + EWS status.
Comment 15 Tim Nguyen (:ntim) 2022-05-29 02:55:19 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1145
Comment 16 EWS 2022-05-30 01:29:50 PDT
Committed r295021 (251116@main): <https://commits.webkit.org/251116@main>

Reviewed commits have been landed. Closing PR #1145 and removing active labels.