Bug 197656 – Implement page-break-* and -webkit-column-break-* as legacy-shorthands.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Joonghun Park]

According to https://drafts.csswg.org/css-cascade-4/#legacy-shorthand,
implement page-break-* and -webkit-column-break-* as legacy-shorthands for break-*.

[Joonghun Park]

Created attachment 369565 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Joonghun Park]

Created attachment 369588 [details]
Patch

[Joonghun Park]

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.

[Joonghun Park]

Could you please review this new patchset?

[Darin Adler]

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?

[Joonghun Park]

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

[Antti Koivisto]

Comment on attachment 369588 [details]
Patch

You should come up with a set of changes that produces progress in tests, not just regressions.

[Joonghun Park]

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:)

[Joonghun Park]

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

[Joonghun Park]

Created attachment 369675 [details]
Revise commit message

[Joonghun Park]

Created attachment 369677 [details]
tidy up redundant codes

[Joonghun Park]

Could you please review this change?

[Joonghun Park]

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

[Joonghun Park]

Created attachment 369685 [details]
Handle CSS wide keyword correctly

[Joonghun Park]

Created attachment 369686 [details]
Handle CSS generic keyword correctly

[Joonghun Park]

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

[Joonghun Park]

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

[Darin Adler]

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.

[Joonghun Park]

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:)

[Joonghun Park]

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

[Joonghun Park]

Created attachment 369830 [details]
Patch

[Joonghun Park]

Created attachment 369831 [details]
Patch for landing

[Joonghun Park]

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

[WebKit Commit Bot]

Comment on attachment 369831 [details]
Patch for landing

Clearing flags on attachment: 369831

Committed r245276: <https://trac.webkit.org/changeset/245276>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50765181>

[Joonghun Park]

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