Bug 223598 - Use Element for checking Settings in CSSComputedStyleDeclaration
Summary: Use Element for checking Settings in CSSComputedStyleDeclaration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-22 13:28 PDT by Rob Buis
Modified: 2021-04-01 03:36 PDT (History)
8 users (show)

See Also:

Attachments
Patch (9.53 KB, patch)
2021-03-26 02:44 PDT, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2021-03-26 06:00 PDT, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (17.95 KB, patch)
2021-03-31 11:53 PDT, Rob Buis 		no flags Details | Formatted Diff | Diff
Patch (18.61 KB, patch)
2021-04-01 00:16 PDT, Rob Buis 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2021-03-22 13:28:05 PDT 
Use Element for checking Settings in CSSComputedStyleDeclaration. Right now renderer is used but this will not work for non-rendered elements.
Comment 1 Rob Buis 2021-03-26 02:44:49 PDT 
Created attachment 424338 [details]
Patch
Comment 2 Rob Buis 2021-03-26 06:00:34 PDT 
Created attachment 424343 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-03-29 13:29:11 PDT 
<rdar://problem/75970166>
Comment 4 Darin Adler 2021-03-31 09:24:07 PDT 
Comment on attachment 424343 [details]
Patch

8 changes, but only 2 tests; I’d expect we’d need 8 different tests to cover them all. Also, did these tests fail before making the change?
Comment 5 Rob Buis 2021-03-31 09:43:26 PDT 
(In reply to Darin Adler from comment #4)
> Comment on attachment 424343 [details]
> Patch
> 
> 8 changes, but only 2 tests; I’d expect we’d need 8 different tests to cover
> them all. Also, did these tests fail before making the change?

Yes, I verified for aspect-ratio and contain. I missed that scroll-behaviour has a similar test. Should I adapt that one and add a similar if-disabled test for cssIndividualTransformProperties before landing?
Comment 6 Darin Adler 2021-03-31 10:23:31 PDT 
Yes, I suggest we make sure we have a test for every change. I don’t feel strongly about how to structure those tests; one test, many, use existing tests that happen to have the correct set-up.
Comment 7 Rob Buis 2021-03-31 11:53:56 PDT 
Created attachment 424795 [details]
Patch
Comment 8 Darin Adler 2021-03-31 18:18:24 PDT 
Looks like some of the new tests are failing on Windows. Maybe DumpRenderTree doesn’t have the necessary support for the CSSIndividualTransformPropertiesEnabled=false on Windows?
Comment 9 Rob Buis 2021-04-01 00:16:55 PDT 
Created attachment 424876 [details]
Patch
Comment 10 EWS 2021-04-01 03:26:42 PDT 
Committed r275351: <https://commits.webkit.org/r275351>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424876 [details].
Comment 11 Rob Buis 2021-04-01 03:36:30 PDT 
(In reply to Darin Adler from comment #8)
> Looks like some of the new tests are failing on Windows. Maybe
> DumpRenderTree doesn’t have the necessary support for the
> CSSIndividualTransformPropertiesEnabled=false on Windows?

Yes, this happened before with the aspect-ratio test, it was marked as failure. Looks like the bug on Windows is that experimental features that are enabled by default can't be disabled for testing. I will leave this to the windows experts.