Bug 197656

Created attachment 369565 [details] Patch

Comment on attachment 369565 [details] Patch Attachment 369565 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12154180 New failing tests: imported/w3c/web-platform-tests/css/cssom/serialize-values.html

Created attachment 369574 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 369565 [details] Patch Attachment 369565 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12154220 New failing tests: imported/w3c/web-platform-tests/css/cssom/serialize-values.html

Created attachment 369576 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 369565 [details] Patch Attachment 369565 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12154584 New failing tests: imported/w3c/web-platform-tests/css/cssom/serialize-values.html

Created attachment 369583 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 369565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369565&action=review Patch looks like a great idea. > Source/WebCore/ChangeLog:8 > + This patch causes no behavioral change. Test failures show this is *almost* true. No significant behavioral change, but a slight change to what you see through the CSS DOM?

Comment on attachment 369565 [details] Patch Attachment 369565 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12154623 New failing tests: imported/w3c/web-platform-tests/css/cssom/serialize-values.html

Created attachment 369587 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4

Created attachment 369588 [details] Patch

Comment on attachment 369565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369565&action=review >> Source/WebCore/ChangeLog:8 >> + This patch causes no behavioral change. > > Test failures show this is *almost* true. No significant behavioral change, but a slight change to what you see through the CSS DOM? Hi, Darin:) Now page-break-* is no longer longhand, so in consumeCSSWideKeyword(propertyID, important), it is handled differently than before. I'm planning to handle it separately in https://bugs.webkit.org/show_bug.cgi?id=191803. So, for now I marked the regression for "inherit" in the test expected file.

Could you please review this new patchset?

Comment on attachment 369588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369588&action=review > LayoutTests/imported/w3c/web-platform-tests/css/cssom/serialize-values-expected.txt:579 > -PASS page-break-after: inherit > +FAIL page-break-after: inherit assert_equals: page-break-after raw inline style declaration expected "inherit" but got "" I don’t understand why it’s good to turn a pass into a fail. Should the web platform tests be updated instead?

(In reply to Darin Adler from comment #14) > Comment on attachment 369588 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369588&action=review > > > LayoutTests/imported/w3c/web-platform-tests/css/cssom/serialize-values-expected.txt:579 > > -PASS page-break-after: inherit > > +FAIL page-break-after: inherit assert_equals: page-break-after raw inline style declaration expected "inherit" but got "" > > I don’t understand why it’s good to turn a pass into a fail. Should the web > platform tests be updated instead? I think the web platform test is per spec, so the failures I added in expected file are just temporary, only until https://bugs.webkit.org/show_bug.cgi?id=191803 is resolved. Currently, if I run the below test in FF and Safari, it shows the result as below. ////////////////////////////////////////////////////// Test: <!doctype html> <div style="page-break-after: inherit"></div> <pre><script> let div = document.querySelector("div"); document.writeln(`Specified: ${div.style.breakAfter}, ${div.style.pageBreakAfter}`); document.writeln(`Computed: ${getComputedStyle(div).breakAfter}, ${getComputedStyle(div).pageBreakAfter}`); </script></pre> Result 1) Safari : Specified: , inherit Computed: auto, auto 2) Specified: inherit, inherit Computed: auto, auto ////////////////////////////////////////////////////// The Safari's result is because |consumeCSSWideKeyword| has been parsing page-break-after as longhand as below. ////////////////////////////////////////////////////// bool CSSPropertyParser::parseValueStart(CSSPropertyID propertyID, bool important) { if (consumeCSSWideKeyword(propertyID, important)) return true; ////////////////////////////////////////////////////// After this patch is applied, the test above shows the result below in Safari. Specified: inherit, Computed: auto, auto But actually this should be as FF which's behavior is per spec. Specified: inherit, inherit Computed: auto, auto I'm planning to add some changes in |StyleProperties::getPropertyValue| to support to serialize these legacy *-break-* properties for CSSStyleDeclaration in https://bugs.webkit.org/show_bug.cgi?id=191803, as a separate bug.

Comment on attachment 369588 [details] Patch You should come up with a set of changes that produces progress in tests, not just regressions.

Ok, then I will include the change for https://bugs.webkit.org/show_bug.cgi?id=191803 in here instead of address these bugs in separate patches. Thank you for your review Koivisto:)

Created attachment 369674 [details] Fix web platform test result's previous regression

Created attachment 369675 [details] Revise commit message

Created attachment 369677 [details] tidy up redundant codes

Could you please review this change?

Created attachment 369684 [details] Handle C css wide keyword correctly

Created attachment 369685 [details] Handle CSS wide keyword correctly

Created attachment 369686 [details] Handle CSS generic keyword correctly

I addressed css wide keyword serialization in the latest patchset. Could you please review this change?

It seems that win-ews's warning could be ignored because it shows flacky regardless of this change.

Comment on attachment 369686 [details] Handle CSS generic keyword correctly View in context: https://bugs.webkit.org/attachment.cgi?id=369686&action=review > Source/WebCore/css/StyleProperties.cpp:728 > + // FIXME: If all longhands are the same css-generic keyword(e.g. initial or inherit), > + // then the shorthand should be serialized to that keyword. > + // It seems to be needed to handle this in a single function commonly for all the shorthands, > + // not in each of the shorthand serialization function. I think this is a good comment, but doesn’t belong here inside one of the specific property value functions. The right place to put it is in StyleProperties::getPropertyValue, I think. If we had a FIXME here it would say: // FIXME: Remove this isGlobalKeyword check after we do this consistently for all shorthands in getPropertyValue. In fact, I think we could probably fix this in the same place we currently have the isPendingSubstitutionValue check. If the web platform tests already have sufficient coverage then I think we should come back and deal with this as soon as possible. But I think we need to test this for every shorthand, because while it seems logical that it’s correct for all of them, I am not certain that it is, and a test case would be the best way to prove it to ourselves. > Source/WebCore/css/StyleProperties.cpp:733 > + return "always"; This could be "always"_s for slightly better performance. It’s kind of inelegant that this is the only hard-coded string in this entire source file, by the way. > Source/WebCore/css/StyleProperties.cpp:737 > + if (valueId == CSSValueAuto || valueId == CSSValueAvoid || valueId == CSSValueLeft > + || valueId == CSSValueRight) > + return value->cssText(); > + return String(); Why do we need to check these? There’s no reason we have to support invalid values. I think we can just return value->cssText() for any case other than CSSValuePage without checking all these specific cases. If we do want to check them then I suggest a switch rather than an if statement.

Comment on attachment 369686 [details] Handle CSS generic keyword correctly View in context: https://bugs.webkit.org/attachment.cgi?id=369686&action=review >> Source/WebCore/css/StyleProperties.cpp:728 >> + // not in each of the shorthand serialization function. > > I think this is a good comment, but doesn’t belong here inside one of the specific property value functions. The right place to put it is in StyleProperties::getPropertyValue, I think. > > If we had a FIXME here it would say: > > // FIXME: Remove this isGlobalKeyword check after we do this consistently for all shorthands in getPropertyValue. > > In fact, I think we could probably fix this in the same place we currently have the isPendingSubstitutionValue check. If the web platform tests already have sufficient coverage then I think we should come back and deal with this as soon as possible. But I think we need to test this for every shorthand, because while it seems logical that it’s correct for all of them, I am not certain that it is, and a test case would be the best way to prove it to ourselves. Ok, then I will place these comments into where proper ones as you commented. And About handling css wide keyword for shorthand in common function, let me make a bug if there isn't yet, and try to make a patch for it. >> Source/WebCore/css/StyleProperties.cpp:733 >> + return "always"; > > This could be "always"_s for slightly better performance. It’s kind of inelegant that this is the only hard-coded string in this entire source file, by the way. Ok, then I will use "always"_s here:) >> Source/WebCore/css/StyleProperties.cpp:737 >> + return String(); > > Why do we need to check these? There’s no reason we have to support invalid values. I think we can just return value->cssText() for any case other than CSSValuePage without checking all these specific cases. If we do want to check them then I suggest a switch rather than an if statement. This value is shorthand.properties()[0], which is page-break-*. Its possible value list is "auto | left | right | avoid | always", while e.g. page-break-after can have possible values as below. auto | avoid | avoid-page | page | left | right | recto | verso | avoid-column | column | avoid-region | region So, if we don't have this checks, when we run the test below, ///////////////////////////////////////////// <!doctype html> <div style="break-after: verso;"></div> <pre><script> let div = document.querySelector("div"); document.writeln(`Specified: ${div.style.breakAfter}, ${div.style.pageBreakAfter}`); document.writeln(`Computed: ${getComputedStyle(div).breakAfter}, ${getComputedStyle(div).pageBreakAfter}`); </script></pre> ///////////////////////////////////////////// 1) originally we expect this result, Specified: verso, Computed: verso, auto 2) but we are going to meet this result. Specified: verso, verso Computed: verso, auto So I think this check is needed one, and I will change this as switch statements as you commented:)

Comment on attachment 369686 [details] Handle CSS generic keyword correctly View in context: https://bugs.webkit.org/attachment.cgi?id=369686&action=review >>> Source/WebCore/css/StyleProperties.cpp:737 >>> + return String(); >> >> Why do we need to check these? There’s no reason we have to support invalid values. I think we can just return value->cssText() for any case other than CSSValuePage without checking all these specific cases. If we do want to check them then I suggest a switch rather than an if statement. > > This value is shorthand.properties()[0], which is page-break-*. Its possible value list is "auto | left | right | avoid | always", while e.g. page-break-after can have possible values as below. > > auto | avoid | avoid-page | page | left | right | recto | verso | avoid-column | column | avoid-region | region > > So, if we don't have this checks, when we run the test below, > ///////////////////////////////////////////// > <!doctype html> > <div style="break-after: verso;"></div> > <pre><script> > let div = document.querySelector("div"); > document.writeln(`Specified: ${div.style.breakAfter}, ${div.style.pageBreakAfter}`); > document.writeln(`Computed: ${getComputedStyle(div).breakAfter}, ${getComputedStyle(div).pageBreakAfter}`); > </script></pre> > ///////////////////////////////////////////// > > 1) originally we expect this result, > > Specified: verso, > Computed: verso, auto > > 2) but we are going to meet this result. > Specified: verso, verso > Computed: verso, auto > > So I think this check is needed one, and I will change this as switch statements as you commented:) I'd like to revise typo as below. This value is shorthand.properties()[0], which is page-break-*. Its possible value list is "auto | left | right | avoid | always", while e.g. break-after can have possible values as below. auto | avoid | avoid-page | page | left | right | recto | verso | avoid-column | column | avoid-region | region

Created attachment 369830 [details] Patch

Created attachment 369831 [details] Patch for landing

*** Bug 191803 has been marked as a duplicate of this bug. ***

Comment on attachment 369831 [details] Patch for landing Clearing flags on attachment: 369831 Committed r245276: <https://trac.webkit.org/changeset/245276>

All reviewed patches have been landed. Closing bug.

<rdar://problem/50765181>

*** Bug 172112 has been marked as a duplicate of this bug. ***