Bug 188984

Summary: Serializing shorthand with "initial" values should check important flags
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, koivisto, rego, simon.fraser, webkit-bug-importer
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Oriol Brufau
Reported 2018-08-27 09:11:45 PDT
Run this code: element.style.setProperty("margin-top", "initial"); element.style.setProperty("margin-right", "initial"); element.style.setProperty("margin-bottom", "initial"); element.style.setProperty("margin-left", "initial", "important"); element.style.margin; Expected: "", like Firefox and Blink Result: "initial"
Attachments
Patch (5.70 KB, patch)
2018-08-28 12:03 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (5.70 KB, patch)
2018-08-28 12:05 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (5.70 KB, patch)
2018-08-28 14:19 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (6.62 KB, patch)
2018-08-29 06:35 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (6.11 KB, patch)
2018-09-04 09:48 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Oriol Brufau
Comment 1 2018-08-28 12:03:25 PDT
Created attachment 348318 [details] Patch
Oriol Brufau
Comment 2 2018-08-28 12:05:29 PDT
Created attachment 348319 [details] Patch
Oriol Brufau
Comment 3 2018-08-28 12:43:27 PDT
WPT PR: https://github.com/web-platform-tests/wpt/pull/12729
Oriol Brufau
Comment 4 2018-08-28 14:19:03 PDT
Created attachment 348343 [details] Patch
Darin Adler
Comment 5 2018-08-28 20:51:33 PDT
Comment on attachment 348343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348343&action=review > LayoutTests/imported/w3c/ChangeLog:9 > + The test still has some failures due to https://bugs.webkit.org/show_bug.cgi?id=188984 > + but without this patch it would fail earlier. What does "fail earlier" mean in practice? How would the test output be different?
Darin Adler
Comment 6 2018-08-28 20:52:00 PDT
(In reply to Oriol Brufau from comment #0) > Run this code: > > element.style.setProperty("margin-top", "initial"); > element.style.setProperty("margin-right", "initial"); > element.style.setProperty("margin-bottom", "initial"); > element.style.setProperty("margin-left", "initial", "important"); > element.style.margin; > > Expected: "", like Firefox and Blink > Result: "initial" Can we also add a test that does exactly this?
Antti Koivisto
Comment 7 2018-08-29 04:09:16 PDT
Comment on attachment 348343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348343&action=review > Source/WebCore/ChangeLog:10 > + The test still has some failures due to https://bugs.webkit.org/show_bug.cgi?id=188984 This is the bug 188984. Did you mean something else?
Oriol Brufau
Comment 8 2018-08-29 06:22:12 PDT
Comment on attachment 348343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348343&action=review >> Source/WebCore/ChangeLog:10 >> + The test still has some failures due to https://bugs.webkit.org/show_bug.cgi?id=188984 > > This is the bug 188984. Did you mean something else? Oh I copied the wrong one, I meant https://bugs.webkit.org/show_bug.cgi?id=185953 >> LayoutTests/imported/w3c/ChangeLog:9 >> + but without this patch it would fail earlier. > > What does "fail earlier" mean in practice? How would the test output be different? The failure would be here: > assert_equals(testElem.style.margin, ""); So it would produce this output > FAIL Shorthand serialization with 'initial' value, one longhand with important flag. assert_equals: expected "" but got "initial" instead of > FAIL Shorthand serialization with 'initial' value, one longhand with important flag. assert_equals: expected "margin-top: initial !important; margin-right: initial; margin-bottom: initial; margin-left: initial;" but got "" > > element.style.setProperty("margin-top", "initial"); > > element.style.setProperty("margin-right", "initial"); > > element.style.setProperty("margin-bottom", "initial"); > > element.style.setProperty("margin-left", "initial", "important"); > > element.style.margin; > > > > Expected: "", like Firefox and Blink > > Result: "initial" > > Can we also add a test that does exactly this? OK, but I don't really see the difference.
Oriol Brufau
Comment 9 2018-08-29 06:35:21 PDT
Created attachment 348400 [details] Patch
Antti Koivisto
Comment 10 2018-08-29 08:02:45 PDT
Comment on attachment 348400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348400&action=review > Source/WebCore/ChangeLog:11 > + Check important flags when serializing shorthand with "initial" values > + https://bugs.webkit.org/show_bug.cgi?id=188984 > + > + Reviewed by NOBODY (OOPS!). > + > + Test: imported/w3c/web-platform-tests/css/cssom/shorthand-serialization.html > + > + The test still has some failures due to https://bugs.webkit.org/show_bug.cgi?id=185953 > + but without this patch it would fail earlier. It is not clear how this patch improves things. It contains no new passing tests and the description doesn't say why this is more correct. Does this code change make sense without 185953?
Oriol Brufau
Comment 11 2018-08-29 09:26:35 PDT
Comment on attachment 348400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348400&action=review >> Source/WebCore/ChangeLog:11 >> + but without this patch it would fail earlier. > > It is not clear how this patch improves things. It contains no new passing tests and the description doesn't say why this is more correct. > > Does this code change make sense without 185953? Yes, without 185953 the new tests would pass. They still fail after this patch but only when checking testElem.style.cssText instead of the earlier testElem.style.margin Do you prefer removing the cssText check for now?
Antti Koivisto
Comment 12 2018-08-30 13:04:50 PDT
Yes, it would be good to have some sort of test that fails before the code change and passes after. It is fine to include additional still-failing tests. You should add some motivation to the ChangeLog since it is not obvious.
Antti Koivisto
Comment 13 2018-08-30 13:05:51 PDT
A passing subtest is fine, not need for full test just for this.
Darin Adler
Comment 14 2018-09-01 22:05:53 PDT
Comment on attachment 348400 [details] Patch Please make the changes to the tests that Antti suggested. review- to the version without any newly passing subtests.
Oriol Brufau
Comment 15 2018-09-04 09:48:11 PDT
Created attachment 348820 [details] Patch
Oriol Brufau
Comment 16 2018-09-04 09:49:27 PDT
OK, I have removed the cssText asserts from the tests with important flags.
WebKit Commit Bot
Comment 17 2018-09-04 22:03:00 PDT
Comment on attachment 348820 [details] Patch Clearing flags on attachment: 348820 Committed r235658: <https://trac.webkit.org/changeset/235658>
WebKit Commit Bot
Comment 18 2018-09-04 22:03:01 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.