Serializing shorthand with "initial" values should check important flags

[Oriol Brufau]

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"

[Oriol Brufau]

Created attachment 348318 [details]
Patch

[Oriol Brufau]

Created attachment 348319 [details]
Patch

[Oriol Brufau]

WPT PR: https://github.com/web-platform-tests/wpt/pull/12729

[Oriol Brufau]

Created attachment 348343 [details]
Patch

[Darin Adler]

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]

(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 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 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]

Created attachment 348400 [details]
Patch

[Antti Koivisto]

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 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]

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]

A passing subtest is fine, not need for full test just for this.

[Darin Adler]

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]

Created attachment 348820 [details]
Patch

[Oriol Brufau]

OK, I have removed the cssText asserts from the tests with important flags.

[WebKit Commit Bot]

Comment on attachment 348820 [details]
Patch

Clearing flags on attachment: 348820

Committed r235658: <https://trac.webkit.org/changeset/235658>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.