Bug 201098

Summary: Unprefix -webkit-print-color-adjust CSS property
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: CSSAssignee: Tim Nguyen (:ntim) <ntim>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, joepeck, macpherson, menard, ntim, pangle, philip, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=228738
https://bugzilla.mozilla.org/show_bug.cgi?id=1747595
Bug Depends on:    
Bug Blocks: 217522    
Attachments:
Description Flags
WIP
none
Patch
darin: review+, ews-feeder: commit-queue-
Patch none

Description Myles C. Maxfield 2019-08-23 15:59:55 PDT
https://www.w3.org/TR/css-color-adjust-1/#perf it's now named "color-adjust".
Comment 1 Myles C. Maxfield 2019-08-24 22:01:21 PDT
Created attachment 377220 [details]
WIP
Comment 2 Philip Jägenstedt 2020-11-26 04:08:54 PST
Are there any existing tests for this? I had a look in https://wpt.fyi/results/css/css-color-adjust?label=master&label=experimental&aligned but can't find any, those tests are for https://drafts.csswg.org/css-color-adjust-1/#preferred
Comment 5 Tim Nguyen (:ntim) 2021-12-24 18:33:26 PST
Filed a bug on the Gecko side to rename color-adjust to print-color-adjust (so color-adjust can eventually be removed from the spec).
Comment 6 Tim Nguyen (:ntim) 2021-12-24 18:33:39 PST
(In reply to Tim Nguyen (:ntim) from comment #5)
> Filed a bug on the Gecko side to rename color-adjust to print-color-adjust
> (so color-adjust can eventually be removed from the spec).

https://bugzilla.mozilla.org/show_bug.cgi?id=1747595
Comment 7 Tim Nguyen (:ntim) 2022-01-06 06:30:58 PST
Created attachment 448495 [details]
Patch
Comment 9 Darin Adler 2022-01-06 10:16:05 PST
Comment on attachment 448495 [details]
Patch

Looks like we missed at least 6 files:

platform/ios-wk2/fast/css/getComputedStyle/computed-style-expected.txt
platform/ios-wk2/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt
platform/ios-wk2/svg/css/getComputedStyle-basic-expected.txt
platform/mac/fast/css/getComputedStyle/computed-style-expected.txt
platform/mac/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt
platform/mac/svg/css/getComputedStyle-basic-expected.txt

Assuming we update those too, r=me

Also a little disappointing there are no progressions in WPT from this. Does this mean WPT doesn’t have any tests of this property?
Comment 10 Darin Adler 2022-01-06 10:16:41 PST
Oh, I see your "WPT is" comment lists a test, but maybe one we haven’t imported yet?
Comment 11 Tim Nguyen (:ntim) 2022-01-06 10:48:54 PST
Created attachment 448509 [details]
Patch
Comment 12 Tim Nguyen (:ntim) 2022-01-06 10:53:53 PST
(In reply to Darin Adler from comment #10)
> Oh, I see your "WPT is" comment lists a test, but maybe one we haven’t
> imported yet?

Yeah, the 2 relevant tests (inheritance and parsing) need to be imported, I filed bug 234927 for that.
Comment 13 Darin Adler 2022-01-06 11:04:09 PST
(In reply to Tim Nguyen (:ntim) from comment #12)
> Yeah, the 2 relevant tests (inheritance and parsing) need to be imported, I
> filed bug 234927 for that.

As a general rule, in cases like this, I would suggest that we first import a test, add it to WebKit along with the expected failure, then fix the bug or add the feature and land the patch that includes a progression on that test.

Not insisting on that in this case, but I think it’s the best practice.
Comment 14 Tim Nguyen (:ntim) 2022-01-06 11:13:49 PST
(In reply to Darin Adler from comment #9)
> Comment on attachment 448495 [details]
> Patch
> 
> Looks like we missed at least 6 files:
> 
> platform/ios-wk2/fast/css/getComputedStyle/computed-style-expected.txt
> platform/ios-wk2/fast/css/getComputedStyle/computed-style-without-renderer-
> expected.txt
> platform/ios-wk2/svg/css/getComputedStyle-basic-expected.txt
> platform/mac/fast/css/getComputedStyle/computed-style-expected.txt
> platform/mac/fast/css/getComputedStyle/computed-style-without-renderer-
> expected.txt
> platform/mac/svg/css/getComputedStyle-basic-expected.txt

r287460 removed the mac files and r287403 renamed the ios-wk2 ones to ios. The remaining ones should already be handled in the patch.

(In reply to Darin Adler from comment #13)
> (In reply to Tim Nguyen (:ntim) from comment #12)
> > Yeah, the 2 relevant tests (inheritance and parsing) need to be imported, I
> > filed bug 234927 for that.
> 
> As a general rule, in cases like this, I would suggest that we first import
> a test, add it to WebKit along with the expected failure, then fix the bug
> or add the feature and land the patch that includes a progression on that
> test.
> 
> Not insisting on that in this case, but I think it’s the best practice.

I agree, I didn't do it in this case since I was planning to update my build first to tip of webkit (which requires a new XCode SDK) before running the tests.

The WPT don't add much over the current webkit LayoutTest, aside from inheritance testing, so I thought that we could land this first. I'll get to bug 234927 ASAP.
Comment 15 Darin Adler 2022-01-06 11:20:58 PST
(In reply to Tim Nguyen (:ntim) from comment #14)
> r287460 removed the mac files and r287403 renamed the ios-wk2 ones to ios.
> The remaining ones should already be handled in the patch.

I goy that impression because of the mac-wk2 failure on EWS; I guess you dealt with that and I just didn’t know the details.
Comment 16 Radar WebKit Bug Importer 2022-01-06 11:28:09 PST
<rdar://problem/87210305>
Comment 17 EWS 2022-01-06 12:22:15 PST
Committed r287712 (245797@main): <https://commits.webkit.org/245797@main>

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