Add CSS parser support for break-after, break-before and break-inside.

Add CSS parser support for break-after, break-before and break-inside.

Note that this only adds support for these properties on specified and computed
style level, and does not extend the functionality in the layout engine. In
particular, we don't support break-(after|before):(avoid|left|right) any better
than before (i.e. we just recognize the values and do nothing about them in the
engine).

The (page|-webkit-column)-break-(after|before|inside) properties are treated as
shorthands for their break-(after|before|inside) counterparts, in accordance
with the spec.

This CL intends to make as few behavioral changes on computed style level as
humanly possible, apart from actually allowing the new properties. In order to
achieve that, we go against the spec when it comes to mapping between the three
modern break-(after|before|inside) properties and the old-fashioned ones. More
specifically, we map "right" and "left" values to "always", and we even support
those values on -webkit-column-break-(after|before), which is just bogus, but
this is how it's always been. We also violate the spec when it comes to mapping
"avoid" values. While the spec says that e.g. page-break-inside:avoid should
simply map to break-inside:avoid, we map it to avoid-page, so that the computed
value of -webkit-column-break-inside isn't affected by such a declaration.

There WILL be some minor behavioral changes, no matter how hard we try, though:
Since there's now just one property for each of before, after and inside
(instead of two - one for page and one for column), declaration sequences like
"page-break-inside:avoid; -webkit-column-break-inside:auto;" will not behave
like before. This will now become "break-inside:auto" (from the
-webkit-column-break-inside declaration), effectively allowing page breaks
inside.

The new test behaves exactly as it would have without the code changes in this
CL, apart from recognizing break-after, break-before and break-inside.

BUG=223068, 492297
Committed: https://crrev.com/c78e9708e49aa13bd5bd9382983f4aa060680dfd
Cr-Commit-Position: refs/heads/master@{#376148}

[mstensho (USE GERRIT), 2016-02-09 22:19:24]

mstensho@opera.com changed reviewers:
+ leviw@chromium.org, timloh@chromium.org

[mstensho (USE GERRIT), 2016-02-09 22:19:24]

@timloh - can you take a look at the changes in core/css/ , please?

@leviw - can you take a look at the rest, please?

:)

https://codereview.chromium.org/1681273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/1681273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:1079: avoid-region
Do we want to allow the region* values in the parser, or should such
declarations just be dropped, since the layout engine isn't likely to support
CSS regions any time soon (or ever)?

[rune, 2016-02-09 23:52:17]

https://codereview.chromium.org/1681273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/1681273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:1079: avoid-region
On 2016/02/09 22:19:24, mstensho wrote:
> Do we want to allow the region* values in the parser, or should such
> declarations just be dropped, since the layout engine isn't likely to support
> CSS regions any time soon (or ever)?

Yes, should be dropped according to:
https://drafts.csswg.org/css-break-3/#conform-partial

[mstensho (USE GERRIT), 2016-02-10 06:09:03]

https://codereview.chromium.org/1681273003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/1681273003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:1079: avoid-region
On 2016/02/09 23:52:17, rune wrote:
> On 2016/02/09 22:19:24, mstensho wrote:
> > Do we want to allow the region* values in the parser, or should such
> > declarations just be dropped, since the layout engine isn't likely to
support
> > CSS regions any time soon (or ever)?
> 
> Yes, should be dropped according to:
> https://drafts.csswg.org/css-break-3/#conform-partial

Done.

[mstensho (USE GERRIT), 2016-02-17 08:01:29]

ping @leviw @timloh

This is the first patch in a series of three, which will unprefix multicol.

https://groups.google.com/a/chromium.org/d/msg/blink-dev/8P5RWP_nRTk/CSQlukIA...

[mstensho (USE GERRIT), 2016-02-17 08:16:24]

Description was changed from

==========
Add CSS parser support for break-after, break-before and break-inside.

Note that this only adds support for these properties on specified and computed
style level, and does not extend the functionality in the layout engine. In
particular, we don't support break-(after|before):(avoid|left|right) any better
than before (i.e. we just recognize the values and do nothing about them in the
engine).

The (page|-webkit-column)-break-(after|before|inside) properties are treated as
shorthands for their break-(after|before|inside) counterparts, in accordance
with the spec.

This CL intends to make as few behavioral changes on computed style level as
humanly possible, apart from actually allowing the new properties. In order to
achieve that, we go against the spec when it comes to mapping between the three
modern break-(after|before|inside) properties and the old-fashioned ones. More
specifically, we map "right" and "left" values to "always", and we even support
those values on -webkit-column-break-(after|before), which is just bogus, but
this is how it's always been. We also violate the spec when it comes to mapping
"avoid" values. While the spec says that e.g. page-break-inside:avoid should
simply map to break-inside:avoid, we map it to avoid-page, so that the computed
value of -webkit-column-break-inside isn't affected by such a declaration.

There WILL be some minor behavioral changes, no matter how hard we try, though:
Since there's now just one property for each of before, after and inside
(instead of two - one for page and one for column), declaration sequences like
"page-break-inside:avoid; -webkit-column-break-inside:auto;" will not behave
like before. This will now become "break-inside:auto" (from the
-webkit-column-break-inside declaration), effectively allowing page breaks
inside.

The new test behaves exactly as it would have without the code changes in this
CL, apart from recognizing break-after, break-before and break-inside.

BUG=223068
==========

to

==========
Add CSS parser support for break-after, break-before and break-inside.

Note that this only adds support for these properties on specified and computed
style level, and does not extend the functionality in the layout engine. In
particular, we don't support break-(after|before):(avoid|left|right) any better
than before (i.e. we just recognize the values and do nothing about them in the
engine).

The (page|-webkit-column)-break-(after|before|inside) properties are treated as
shorthands for their break-(after|before|inside) counterparts, in accordance
with the spec.

This CL intends to make as few behavioral changes on computed style level as
humanly possible, apart from actually allowing the new properties. In order to
achieve that, we go against the spec when it comes to mapping between the three
modern break-(after|before|inside) properties and the old-fashioned ones. More
specifically, we map "right" and "left" values to "always", and we even support
those values on -webkit-column-break-(after|before), which is just bogus, but
this is how it's always been. We also violate the spec when it comes to mapping
"avoid" values. While the spec says that e.g. page-break-inside:avoid should
simply map to break-inside:avoid, we map it to avoid-page, so that the computed
value of -webkit-column-break-inside isn't affected by such a declaration.

There WILL be some minor behavioral changes, no matter how hard we try, though:
Since there's now just one property for each of before, after and inside
(instead of two - one for page and one for column), declaration sequences like
"page-break-inside:avoid; -webkit-column-break-inside:auto;" will not behave
like before. This will now become "break-inside:auto" (from the
-webkit-column-break-inside declaration), effectively allowing page breaks
inside.

The new test behaves exactly as it would have without the code changes in this
CL, apart from recognizing break-after, break-before and break-inside.

BUG=223068,492297
==========

[leviw_travelin_and_unemployed, 2016-02-18 02:37:13]

Tim still needs to sign-off on css/, but the rest LGMT.

[leviw_travelin_and_unemployed, 2016-02-18 02:37:34]

whoops.... lgtm

[Timothy Loh, 2016-02-18 02:40:00]

On 2016/02/18 02:37:13, leviw wrote:
> Tim still needs to sign-off on css/, but the rest LGMT.

yeah haven't looked at this yet, will get to it shortly. sorry for the delay!

[Timothy Loh, 2016-02-18 06:25:45]

lgtm

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.in:156: break-after
type_name=EBreak, initial=initialBreakAfter
doesn't it guess exactly these initial functions?

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:561: case
CSSPropertyBreakAfter: // auto | avoid | always | any | avoid-page | page | left
| right | recto | verso | avoid-column | column
I wouldn't add the comments.. we should probably remove all the ones which don't
add any value at some point :\

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4278: if
(property == CSSPropertyPageBreakInside || property ==
CSSPropertyWebkitColumnBreakInside)
I prefer the style of having the last if just be an ASSERT. But I don't feel
strongly about it, so do what you want.

[mstensho (USE GERRIT), 2016-02-18 09:05:25]

mstensho@opera.com changed reviewers:
+ philipj@opera.com

[mstensho (USE GERRIT), 2016-02-18 09:05:26]

Looks like I need an approval from an API owner as well, since I'm modifying
third_party/WebKit/LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt

@philipj, care to take a look?

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSProperties.in:156: break-after
type_name=EBreak, initial=initialBreakAfter
On 2016/02/18 06:25:44, Timothy Loh wrote:
> doesn't it guess exactly these initial functions?

Indeed.

Done.

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:561: case
CSSPropertyBreakAfter: // auto | avoid | always | any | avoid-page | page | left
| right | recto | verso | avoid-column | column
On 2016/02/18 06:25:45, Timothy Loh wrote:
> I wouldn't add the comments.. we should probably remove all the ones which
don't
> add any value at some point :\

Yeah, I agree that they are pretty silly and useless.

Removed.

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1681273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4278: if
(property == CSSPropertyPageBreakInside || property ==
CSSPropertyWebkitColumnBreakInside)
On 2016/02/18 06:25:45, Timothy Loh wrote:
> I prefer the style of having the last if just be an ASSERT. But I don't feel
> strongly about it, so do what you want.

I like it!

Done.

[mstensho (USE GERRIT), 2016-02-18 10:21:26]

mstensho@opera.com changed reviewers:
+ jochen@chromium.org

[mstensho (USE GERRIT), 2016-02-18 10:21:27]

Trying an API owner in Europe. :)

@jochen - can you take a look, please?

It's about the changes in LayoutTests/webexposed/

[philipj_slow, 2016-02-18 12:08:22]

On 2016/02/18 10:21:27, mstensho wrote:
> Trying an API owner in Europe. :)
> 
> @jochen - can you take a look, please?
> 
> It's about the changes in LayoutTests/webexposed/

It's still not that late in Hanoi :)

third_party/WebKit/LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt
LGTM assuming that the rest of the support will follow before the next branch
point. If not, exposing these new things without the layout engine support is
not ideal, people might use it for feature detection?

[mstensho (USE GERRIT), 2016-02-18 12:18:41]

On 2016/02/18 12:08:22, philipj_UTC7 wrote:
> On 2016/02/18 10:21:27, mstensho wrote:
> > Trying an API owner in Europe. :)
> > 
> > @jochen - can you take a look, please?
> > 
> > It's about the changes in LayoutTests/webexposed/
> 
> It's still not that late in Hanoi :)
> 
>
third_party/WebKit/LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt
> LGTM assuming that the rest of the support will follow before the next branch
> point. If not, exposing these new things without the layout engine support is
> not ideal, people might use it for feature detection?

Yeah, that's next on my list. Note, though, that declarations like
"page-break-before:avoid" has been allowed in Blink since day one, although the
layout engine has never supported it. So, not supporting "break-before:avoid" in
the layout engine isn't going to make the situation much different or worse.

[mstensho (USE GERRIT), 2016-02-18 12:18:49]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-02-18 12:18:50]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org,
timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1681273003/#ps80001
(title: "rebase master")

[commit-bot: I haz the power, 2016-02-18 12:20:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681273003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681273003/80001

[commit-bot: I haz the power, 2016-02-18 12:28:14]

Description was changed from

==========
Add CSS parser support for break-after, break-before and break-inside.

Note that this only adds support for these properties on specified and computed
style level, and does not extend the functionality in the layout engine. In
particular, we don't support break-(after|before):(avoid|left|right) any better
than before (i.e. we just recognize the values and do nothing about them in the
engine).

The (page|-webkit-column)-break-(after|before|inside) properties are treated as
shorthands for their break-(after|before|inside) counterparts, in accordance
with the spec.

This CL intends to make as few behavioral changes on computed style level as
humanly possible, apart from actually allowing the new properties. In order to
achieve that, we go against the spec when it comes to mapping between the three
modern break-(after|before|inside) properties and the old-fashioned ones. More
specifically, we map "right" and "left" values to "always", and we even support
those values on -webkit-column-break-(after|before), which is just bogus, but
this is how it's always been. We also violate the spec when it comes to mapping
"avoid" values. While the spec says that e.g. page-break-inside:avoid should
simply map to break-inside:avoid, we map it to avoid-page, so that the computed
value of -webkit-column-break-inside isn't affected by such a declaration.

There WILL be some minor behavioral changes, no matter how hard we try, though:
Since there's now just one property for each of before, after and inside
(instead of two - one for page and one for column), declaration sequences like
"page-break-inside:avoid; -webkit-column-break-inside:auto;" will not behave
like before. This will now become "break-inside:auto" (from the
-webkit-column-break-inside declaration), effectively allowing page breaks
inside.

The new test behaves exactly as it would have without the code changes in this
CL, apart from recognizing break-after, break-before and break-inside.

BUG=223068,492297
==========

to

==========
Add CSS parser support for break-after, break-before and break-inside.

Note that this only adds support for these properties on specified and computed
style level, and does not extend the functionality in the layout engine. In
particular, we don't support break-(after|before):(avoid|left|right) any better
than before (i.e. we just recognize the values and do nothing about them in the
engine).

The (page|-webkit-column)-break-(after|before|inside) properties are treated as
shorthands for their break-(after|before|inside) counterparts, in accordance
with the spec.

This CL intends to make as few behavioral changes on computed style level as
humanly possible, apart from actually allowing the new properties. In order to
achieve that, we go against the spec when it comes to mapping between the three
modern break-(after|before|inside) properties and the old-fashioned ones. More
specifically, we map "right" and "left" values to "always", and we even support
those values on -webkit-column-break-(after|before), which is just bogus, but
this is how it's always been. We also violate the spec when it comes to mapping
"avoid" values. While the spec says that e.g. page-break-inside:avoid should
simply map to break-inside:avoid, we map it to avoid-page, so that the computed
value of -webkit-column-break-inside isn't affected by such a declaration.

There WILL be some minor behavioral changes, no matter how hard we try, though:
Since there's now just one property for each of before, after and inside
(instead of two - one for page and one for column), declaration sequences like
"page-break-inside:avoid; -webkit-column-break-inside:auto;" will not behave
like before. This will now become "break-inside:auto" (from the
-webkit-column-break-inside declaration), effectively allowing page breaks
inside.

The new test behaves exactly as it would have without the code changes in this
CL, apart from recognizing break-after, break-before and break-inside.

BUG=223068,492297
==========

[commit-bot: I haz the power, 2016-02-18 12:28:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-18 12:29:33]

Description was changed from

==========
Add CSS parser support for break-after, break-before and break-inside.

Note that this only adds support for these properties on specified and computed
style level, and does not extend the functionality in the layout engine. In
particular, we don't support break-(after|before):(avoid|left|right) any better
than before (i.e. we just recognize the values and do nothing about them in the
engine).

The (page|-webkit-column)-break-(after|before|inside) properties are treated as
shorthands for their break-(after|before|inside) counterparts, in accordance
with the spec.

This CL intends to make as few behavioral changes on computed style level as
humanly possible, apart from actually allowing the new properties. In order to
achieve that, we go against the spec when it comes to mapping between the three
modern break-(after|before|inside) properties and the old-fashioned ones. More
specifically, we map "right" and "left" values to "always", and we even support
those values on -webkit-column-break-(after|before), which is just bogus, but
this is how it's always been. We also violate the spec when it comes to mapping
"avoid" values. While the spec says that e.g. page-break-inside:avoid should
simply map to break-inside:avoid, we map it to avoid-page, so that the computed
value of -webkit-column-break-inside isn't affected by such a declaration.

There WILL be some minor behavioral changes, no matter how hard we try, though:
Since there's now just one property for each of before, after and inside
(instead of two - one for page and one for column), declaration sequences like
"page-break-inside:avoid; -webkit-column-break-inside:auto;" will not behave
like before. This will now become "break-inside:auto" (from the
-webkit-column-break-inside declaration), effectively allowing page breaks
inside.

The new test behaves exactly as it would have without the code changes in this
CL, apart from recognizing break-after, break-before and break-inside.

BUG=223068,492297
==========

to

==========
Add CSS parser support for break-after, break-before and break-inside.

Note that this only adds support for these properties on specified and computed
style level, and does not extend the functionality in the layout engine. In
particular, we don't support break-(after|before):(avoid|left|right) any better
than before (i.e. we just recognize the values and do nothing about them in the
engine).

The (page|-webkit-column)-break-(after|before|inside) properties are treated as
shorthands for their break-(after|before|inside) counterparts, in accordance
with the spec.

This CL intends to make as few behavioral changes on computed style level as
humanly possible, apart from actually allowing the new properties. In order to
achieve that, we go against the spec when it comes to mapping between the three
modern break-(after|before|inside) properties and the old-fashioned ones. More
specifically, we map "right" and "left" values to "always", and we even support
those values on -webkit-column-break-(after|before), which is just bogus, but
this is how it's always been. We also violate the spec when it comes to mapping
"avoid" values. While the spec says that e.g. page-break-inside:avoid should
simply map to break-inside:avoid, we map it to avoid-page, so that the computed
value of -webkit-column-break-inside isn't affected by such a declaration.

There WILL be some minor behavioral changes, no matter how hard we try, though:
Since there's now just one property for each of before, after and inside
(instead of two - one for page and one for column), declaration sequences like
"page-break-inside:avoid; -webkit-column-break-inside:auto;" will not behave
like before. This will now become "break-inside:auto" (from the
-webkit-column-break-inside declaration), effectively allowing page breaks
inside.

The new test behaves exactly as it would have without the code changes in this
CL, apart from recognizing break-after, break-before and break-inside.

BUG=223068,492297

Committed: https://crrev.com/c78e9708e49aa13bd5bd9382983f4aa060680dfd
Cr-Commit-Position: refs/heads/master@{#376148}
==========

[commit-bot: I haz the power, 2016-02-18 12:29:34]

Patchset 5 (id:??) landed as
https://crrev.com/c78e9708e49aa13bd5bd9382983f4aa060680dfd
Cr-Commit-Position: refs/heads/master@{#376148}

[PhistucK, 2016-02-18 12:53:13]

This looks like a breaking change
(getComputedStyle(element).webkitColumnBreakAfter will return undefined from now
on, right?)...
Does this have a use counter?

Since the Chrome 50 branch point is near, perhaps revert this instead of having
a partially implemented feature (getComputedStyle(element).pageBreak !==
undefined but not really does anything, right?) that can break feature
detection?

[PhistucK, 2016-02-18 12:53:20]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[PhistucK, 2016-02-18 12:55:27]

On 2016/02/18 at 12:53:13, PhistucK wrote:
> This looks like a breaking change
(getComputedStyle(element).webkitColumnBreakAfter will return undefined from now
on, right?)...
> Does this have a use counter?
> 
> Since the Chrome 50 branch point is near, perhaps revert this instead of
having a partially implemented feature (getComputedStyle(element).pageBreak !==
undefined but not really does anything, right?) that can break feature
detection?

Oh, it does do something, just not all of the values are supported, like you
agreed in the thread. You can ignore this bullet.

[drott, 2016-02-22 06:45:21]

Similar to https://codereview.chromium.org/1710003002/, could it be that this is
missing the tools/metrics/histograms/histograms.xml update?

[PhistucK, 2016-02-22 06:50:41]

On 2016/02/18 at 12:55:27, PhistucK wrote:
> On 2016/02/18 at 12:53:13, PhistucK wrote:
> > This looks like a breaking change
(getComputedStyle(element).webkitColumnBreakAfter will return undefined from now
on, right?)...
> > Does this have a use counter?

This bullet still stands, though.

[mstensho (USE GERRIT), 2016-02-22 10:40:52]

On 2016/02/22 06:45:21, drott wrote:
> Similar to https://codereview.chromium.org/1710003002/, could it be that this
is
> missing the tools/metrics/histograms/histograms.xml update?

See https://codereview.chromium.org/1720823002/

[drott, 2016-02-25 07:46:46]

On 2016/02/22 at 10:40:52, mstensho wrote:

> See https://codereview.chromium.org/1720823002/

Thanks!