Summary: | Unprefix -webkit-print-color-adjust CSS property | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
Component: | CSS | Assignee: | 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
Myles C. Maxfield
2019-08-23 15:59:55 PDT
Created attachment 377220 [details]
WIP
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 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). (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 Created attachment 448495 [details]
Patch
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?
Oh, I see your "WPT is" comment lists a test, but maybe one we haven’t imported yet? Created attachment 448509 [details]
Patch
(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. (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. (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. (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. 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]. |