Bug 197656 - Implement page-break-* and -webkit-column-break-* as legacy-shorthands.
Summary: Implement page-break-* and -webkit-column-break-* as legacy-shorthands.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: InRadar
: 191803 (view as bug list)
Depends on:
Blocks: 191803
  Show dependency treegraph
 
Reported: 2019-05-07 04:32 PDT by Joonghun Park
Modified: 2019-05-14 07:03 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.46 KB, patch)
2019-05-10 10:43 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.21 MB, application/zip)
2019-05-10 11:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.99 MB, application/zip)
2019-05-10 12:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (2.88 MB, application/zip)
2019-05-10 12:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (7.85 MB, application/zip)
2019-05-10 13:11 PDT, Build Bot
no flags Details
Patch (11.65 KB, patch)
2019-05-10 13:11 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Fix web platform test result's previous regression (16.37 KB, patch)
2019-05-12 09:37 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Revise commit message (16.37 KB, patch)
2019-05-12 09:43 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
tidy up redundant codes (16.34 KB, patch)
2019-05-12 09:54 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Handle C css wide keyword correctly (16.69 KB, patch)
2019-05-12 18:25 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Handle CSS wide keyword correctly (16.69 KB, patch)
2019-05-12 18:26 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Handle CSS generic keyword correctly (16.69 KB, patch)
2019-05-12 18:35 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (17.08 KB, patch)
2019-05-14 03:01 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch for landing (17.20 KB, patch)
2019-05-14 03:10 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joonghun Park 2019-05-07 04:32:15 PDT
According to https://drafts.csswg.org/css-cascade-4/#legacy-shorthand,
implement page-break-* and -webkit-column-break-* as legacy-shorthands for break-*.
Comment 1 Joonghun Park 2019-05-10 10:43:43 PDT
Created attachment 369565 [details]
Patch
Comment 2 Build Bot 2019-05-10 11:47:19 PDT
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
Comment 3 Build Bot 2019-05-10 11:47:21 PDT
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 4 Build Bot 2019-05-10 12:01:58 PDT
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
Comment 5 Build Bot 2019-05-10 12:02:00 PDT
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 6 Build Bot 2019-05-10 12:58:36 PDT
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
Comment 7 Build Bot 2019-05-10 12:58:38 PDT
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 8 Darin Adler 2019-05-10 13:09:27 PDT
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 9 Build Bot 2019-05-10 13:11:11 PDT
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
Comment 10 Build Bot 2019-05-10 13:11:13 PDT
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
Comment 11 Joonghun Park 2019-05-10 13:11:26 PDT
Created attachment 369588 [details]
Patch
Comment 12 Joonghun Park 2019-05-10 13:18:54 PDT
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.
Comment 13 Joonghun Park 2019-05-10 18:33:59 PDT
Could you please review this new patchset?
Comment 14 Darin Adler 2019-05-11 13:00:32 PDT
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?
Comment 15 Joonghun Park 2019-05-11 18:43:16 PDT
(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 16 Antti Koivisto 2019-05-12 03:08:41 PDT
Comment on attachment 369588 [details]
Patch

You should come up with a set of changes that produces progress in tests, not just regressions.
Comment 17 Joonghun Park 2019-05-12 03:17:36 PDT
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:)
Comment 18 Joonghun Park 2019-05-12 09:37:51 PDT
Created attachment 369674 [details]
Fix web platform test result's previous regression
Comment 19 Joonghun Park 2019-05-12 09:43:30 PDT
Created attachment 369675 [details]
Revise commit message
Comment 20 Joonghun Park 2019-05-12 09:54:39 PDT
Created attachment 369677 [details]
tidy up redundant codes
Comment 21 Joonghun Park 2019-05-12 15:18:58 PDT
Could you please review this change?
Comment 22 Joonghun Park 2019-05-12 18:25:04 PDT
Created attachment 369684 [details]
Handle C css wide keyword correctly
Comment 23 Joonghun Park 2019-05-12 18:26:31 PDT
Created attachment 369685 [details]
Handle CSS wide keyword correctly
Comment 24 Joonghun Park 2019-05-12 18:35:09 PDT
Created attachment 369686 [details]
Handle CSS generic keyword correctly
Comment 25 Joonghun Park 2019-05-12 18:37:20 PDT
I addressed css wide keyword serialization in the latest patchset.
Could you please review this change?
Comment 26 Joonghun Park 2019-05-13 00:46:51 PDT
It seems that win-ews's warning could be ignored because it shows flacky regardless of this change.
Comment 27 Darin Adler 2019-05-13 07:50:52 PDT
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 28 Joonghun Park 2019-05-13 09:15:24 PDT
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 29 Joonghun Park 2019-05-13 09:19:17 PDT
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
Comment 30 Joonghun Park 2019-05-14 03:01:50 PDT
Created attachment 369830 [details]
Patch
Comment 31 Joonghun Park 2019-05-14 03:10:57 PDT
Created attachment 369831 [details]
Patch for landing
Comment 32 Joonghun Park 2019-05-14 04:26:45 PDT
*** Bug 191803 has been marked as a duplicate of this bug. ***
Comment 33 WebKit Commit Bot 2019-05-14 07:02:56 PDT
Comment on attachment 369831 [details]
Patch for landing

Clearing flags on attachment: 369831

Committed r245276: <https://trac.webkit.org/changeset/245276>
Comment 34 WebKit Commit Bot 2019-05-14 07:02:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2019-05-14 07:03:23 PDT
<rdar://problem/50765181>