775618 - Alias page-break-* to break-*

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[fantasai]

The CSSWG aliased the name of the page-break-* properties to break-* as part of extending them to handle columns (and potentially other fragmenting things). This bug is about aliasing them. It should be sufficient to make a parse-time alias, like we do for -moz- properties, since it's unlikely anyone will be using the OM for page-breaking controls.

Comment 1

[fantasai]

http://www.w3.org/TR/css3-break/#page-break

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix

Alias CSS 2.1 page-break-* properties to CSS 3 break-* and add new break-* property values.

https://tbpl.mozilla.org/?tree=Try&rev=73f18932f3f2
(the M1 orange is unrelated)

I've also verified in a local debug build that tests under:
  layout/reftests/w3c-css/submitted/css21/pagination/
  layout/reftests/pagination/
pass with s/page-break-/break-/

Comment 3

[fantasai]

Comment on attachment 699688 [details] [diff] [review]
fix

So, dbaron should probably also look over this patch once it passes my review, since it's mostly style system things, and I won't notice if you missed something. But here's comments I have:

IsCSS21PageBreakBefore() should probably just be IsPageBreakBefore()

+#define NS_STYLE_BREAK_AVOID_PAGE              10
+#define NS_STYLE_BREAK_AVOID_COLUMN            11
+#define NS_STYLE_BREAK_AVOID_REGION            12
+CSS_KEY(avoid-column, avoid_column)
+CSS_KEY(avoid-page, avoid_page)
+CSS_KEY(avoid-region, avoid_region)
+  eCSSKeyword_column,       NS_STYLE_BREAK_COLUMN,
+  eCSSKeyword_region,       NS_STYLE_BREAK_REGION,
+  eCSSKeyword_avoid_page,   NS_STYLE_BREAK_AVOID_PAGE,
+  eCSSKeyword_avoid_column, NS_STYLE_BREAK_AVOID_COLUMN,
+  eCSSKeyword_avoid_region, NS_STYLE_BREAK_AVOID_REGION,

We don't actually implement these values, so we shouldn't parse them, either. You can put them in, but only if they're commented out. Same for 'avoid' on break-before/after.

Similarly, recto/verso should also not be parsed; alternately, they should trigger the same behavior as left/right do now, since we don't support a distinction between left/right pages.

(Our implementation of forced page breaks is also broken; it doesn't propagate through a multi-col element. But probably better not to actually turn that off atm.)

http://dev.w3.org/csswg/css3-break/#page-break-properties
The table here maps "page-break-before: always" to "break-before: page".
I'm not seeing where that's handled in your code, since you're just listing the properties as aliases, right?

Comment 5

[Florian Bender]

Mats, do you have time to work on this for Train 30?

Comment 6

[Mats Palmgren (inactive)]

(In reply to Florian Bender from comment #5)
> Mats, do you have time to work on this for Train 30?

No, I don't think I'll have time to work on this in the near future. Sorry.

Comment 7

[Florian Bender]

OK, I reset the assignee so someone else can look at this. If you find time for this bug, please retake.

Comment 8

[Florian Bender]

Comment on attachment 699688 [details] [diff] [review]
fix

(In reply to fantasai from comment #3)
> So, dbaron should probably also look over this patch once it passes my
> review, since it's mostly style system things, and I won't notice if you
> missed something. 

Though this did not pass review yet, David may have some input before someone else has a stab on it.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 699688 [details] [diff] [review]
fix

Not really, other than that it would have been nice to split into multiple patches for the different parts of the patch (e.g., aliasing properties, changing values, layout changes, etc.).  (It has the appropriate property_database.js changes (although I didn't check for the new/changed values), so if the mochitests pass, it's probably mostly on the right track style-system-wise.)

Or was there something in particular you wanted me to look at; it's a pretty large patch.

Comment 10

[Florian Bender]

Thanks for the fast response! Fantasai may have more specific questions but I guess this is enough for now. :)

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

FWIW, I'm not convinced this is something we currently want to do; I'm really not convince that extending page-break-* to break-* is a sensible design (as we just discussed at the CSS WG meeting).

Comment 12

[fantasai]

Fwiw, the spec is now in CR. https://www.w3.org/TR/css-break-3/

Comment 13

[Jen Simmons [:jensimmons]]

The lack of `break-inside` is the cause of many terrible multicolumn layout bugs, and is a big reason folks don't use multi-column. 

Here's an example: http://labs.jensimmons.com/examples/multicolumn-3-bug-demo.html 

Is this just a matter of aliasing working functionality to new names? Then let's do it!! There are a lot of usecases for `break-*` properties that go far beyond printing on paper. 

(And my apologies if this is the wrong bugzilla ticket. I'm guessing this is it, but could be wrong.)

Comment 14

[Florian Rivoal]

Supporting `break-inside`, `break-before` and `break-after` for the values already supported by `page-break-inside`, `page-break-before` and `page-break-after` should indeed just be a trivial matter of aliasing to the later, and the spec is nice enough to define how the aliasing should work[1] (tl;dr: make `page-break-*` a shorthand of `break-*`).


Supporting the `avoid` value on `(page-)break-before` and `(page-)break-after` would be very useful as well (and as Jen says, not just on paged media), but not as trivial as merely aliasing. This is already tracked as https://bugzilla.mozilla.org/show_bug.cgi?id=775617


Supporting the other values of break-* would be nice as well, but I think it would be less urgent, and there's already a bunch of bugs to track that. The tracker bug for such things is https://bugzilla.mozilla.org/show_bug.cgi?id=775628

[1] https://drafts.csswg.org/css-break/#page-break-properties

Comment 15

[Jen Simmons [:jensimmons]]

`page-break-inside` has been implemented. 
`break-inside` has not. 

Fixing this aliasing bug would gain a tremendous amount of goodwill from developers who are using multicolumn, even if we just start with `break-inside`. It's been a source of a lot of frustration to not have `break-inside` in Firefox. Using Autoprefixer solves this for some developers, but it would be far better for this to just work in the browser.

Is this an afternoon's work for someone familiar with the style system? It'd be a great quick win. It might not be easy to alias all the `break-*` properties, but this one alone is worth shipping. It will ease the biggest point of pain for Authors.

Comment 16

[Simon Sapin (:SimonSapin)]

It looks like the page-break-inside property does affect multicol already (based on editing http://labs.jensimmons.com/2016/examples/multicolumn-3-bug-demo.html with devtools), so making an alias in the style system(s) without changing any layout code should be sufficient for this use case.

https://drafts.csswg.org/css-break/#page-break-properties says page-break-* should become shorthands to new break-* longhand properties. We could do that and only support the subset of break-* values that have a mapping from page-break-*. Namely, these would *not* be supported: avoid-page | recto | verso | avoid-column | column | avoid-region | region.

We’ll likely need to do all this twice, in both Stylo and the previous style system.

David, what do you think of this plan? You seemed opposed in comment 11 to the larger patch adding all values.

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Given that it's a CR now, I'm OK with it, as long as we only add values that we actually support.

(The spec might then require that we remove some features too, like the influence of page-break-* on column breaking -- though we should probably only do that if it would make us *more* compatible with other browsers.)

Comment 18

[Simon Sapin (:SimonSapin)]

> The spec might then require that we remove some features too, like the influence of page-break-* on column breaking

I don’t think that’s the case. https://drafts.csswg.org/css-break/#page-break-properties defines 'avoid' in page-break-* as mapping to 'avoid' in break-*, not 'avoid-page'.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

So I thought a bit about this.

Given we only support page-break-{before,after}: {always, left, right, avoid, auto}, I don't think there's any point in doing anything more complicated than a simple alias. We respect page-break-before: always for non-paginated contexts anyway, so I think just aliasing them is the right thing to do to preserve existing behavior and not break stuff.

I don't think we should care about whether to parse future values of the page-break-* properties in the break-* properties, at least for now, and I don't think it really matters given they're defined to be aliases in the spec anyway, (with the page / always gotcha, but I think we should push to remove that since as I said we respect page-break-* on non-paginated contexts).

I actually had an implementation of the more complicated thing, until I realized that I couldn't implement the more complicated thing since we don't restrict breaking to pages anyway.

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 775618 - Introduce break-{before,after,inside}, and alias page-break-{before,after,inside} to them.

Given we only support page-break-{before,after}: {always, left, right, avoid,
auto}, I don't think there's any point in doing anything more complicated than
a simple alias.

We respect page-break-before: always for non-paginated contexts anyway, so I
think just aliasing them is the right thing to do to preserve existing behavior
and not break stuff.

I don't think we should care about whether to parse future values of the
page-break-* properties in the break-* properties, given they're deprecated,
and don't think it really matters given they're defined to be aliases in the
spec anyway, (with the page <-> always gotcha, but I think we should remove
that since as I said we respect page-break-* on non-paginated contexts anyway,
so page-break-before: always is effectively the same as break-before: always).

I actually had an implementation of the more complicated thing, until I
realized that I couldn't implement the more complicated thing since we don't
restrict breaking to pages anyway.

If this sounds reasonable I'll send an intent email to dev-platform@, and file
the relevant spec issues.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Err, actually it's a bit more complicated, since I forgot break-{before,after} don't have the "always" value. So this requires layout work...

Comment 22

[fantasai]

Emilio,
Let's do what we can without layout work, since the majority of the usage doesn't involve forced breaks:
https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/?q=break
https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/css/break-after/
https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/css/break-before/

That would mean:
* aliasing 'break-inside' to 'page-break-inside', because that corresponds to what we actually implement atm.
* shorthanding 'page-break-before/after: auto | avoid' to 'break-before/after: auto | avoid' for similar reasons

This means that 'page-break-before/after: always' won't have a corresponding value in 'break-before/after', but that's something the shorthanding mechanism should be able to handle. Also, since it looks like IE supports an 'always' value (which it seems to be parsing in on 0.67% of pages?), CSSWG should look into adding that value once we have a clear idea of what it is. (There was an 'always' value in the past, but it became unclear whether it should cause a break only within the nearest fragmentation context, or all the way to the top-level fragmentation context, so we dropped it from L3.) If the 'break-before/after: always' that IE implements matches our 'page-break-before/after: always' implementation, then we can map that over, too. Filed https://github.com/w3c/csswg-drafts/issues/3318 on this.

Comment 23

[Emilio Cobos Álvarez [:emilio]]

(In reply to fantasai from comment #22)
FWIW I'm actually working into get something landable here...

Just doing some preliminary cleanup and refactoring before that :-)

Comment 24

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 775618 - Implement page-break-{before,after} as legacy shorthands for {before,after}.

This is all the style-system work needed for this.

This implements the concept of legacy shorthands, teaches tests to understand
it, and adds a few more tests for these properties in particular.

The WPT even caught a few WebKit / Blink bugs:

  https://bugs.chromium.org/p/chromium/issues/detail?id=906332
  https://bugs.webkit.org/show_bug.cgi?id=191803

This doesn't change the layout behavior for page-break-before: always, since
it'd stop breaking in multicol and such. Similarly, break-before / break-after:
column and page still behave the same, I'll file followups for those given
comment 22.

Comment 25

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 775618 - Introduce break-inside, and alias page-break-inside to it.

This property has no weird value mapping, so we can just do this.

Comment 26

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 775618 - Introduce the concept of legacy shorthands.

We need this because there's a weird mapping between these properties' values
('always' maps to 'page').

See https://drafts.csswg.org/css-cascade-4/#legacy-shorthand.

Comment 27

[Emilio Cobos Álvarez [:emilio]]

Fantasai, do you know why page-break-*: avoid maps to break-*: avoid instead of avoid-page?

I've implemented that, but looks a bit fishy... It's what other browsers do though so shrug.

Comment 28

[Emilio Cobos Álvarez [:emilio]]

So, I got this patch into a landable state. We decided not to expose break-{before,after}: column: Is that acceptable jen?

I'm not sure how this is used in the wild.

Comment 29

[Sebastian Zartner [:sebo]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #28)
> We decided not to expose break-{before,after}: column

That's covered by bug 549114, anyway.

Sebastian

Comment 30

[Mats Palmgren (inactive)]

FYI: https://github.com/w3c/csswg-drafts/issues/3318

"The CSS Working Group just discussed "Should 'break-before' / 'break-after'
have an 'always' value?", and agreed to the following:

    RESOLVED: add 'always' and 'all' to break-before/after where always 
is on the nearest fragmentationer and all breaks across all fragmentaners

(sic)

Comment 31

[fantasai]

> Fantasai, do you know why page-break-*: avoid maps to break-*: avoid instead of avoid-page?

If you want to avoid a break in pagination, you usually want to also avoid a break for column breaking. (Typical cases include inside or right after a heading, between an image and its caption, etc.)

> So, I got this patch into a landable state. We decided not to expose break-{before,after}: column: Is that acceptable jen?

See https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/?q=break-
I think it's fine. I suspect this isn't the type of feature where people are keying support of column off of other values in @supports and then drastically changing their layouts if it's unsupported or anything like that...

Also, as Mats noted, we did just resolve on adding 'always' to mean, afaict, what Gecko does if we map it over to the existing layout infrastructure for forced breaks, so mapping that over would provide an equivalent to 'column' (since it's just like 'column' as long as you're in a multicol).

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c228095e647e
Introduce break-inside, and alias page-break-inside to it. r=heycam

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f97f45821194
Introduce the concept of legacy shorthands. r=heycam

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4c0c0fecdba
Implement page-break-{before,after} as legacy shorthands for {before,after}. r=heycam

Comment 35

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c228095e647e
https://hg.mozilla.org/mozilla-central/rev/f97f45821194
https://hg.mozilla.org/mozilla-central/rev/b4c0c0fecdba

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14312 for changes under testing/web-platform/tests

Comment 37

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 38

[Chris Mills [:cmills]]

Note to MDN writers:

This is a little bit confusing, but I *think* I get it. I've add a note to the Fx 65 rel notes that I think describes what happened here:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#CSS

In terms of the docs work, There is a bit to do here. We already have separate pages for

page-break-before
page-break-after
page-break-inside
break-before
break-after
break-inside

I think we need to delete the page-break-* pages since they are the obsolete ones, and redirect them to their break-* coun terparts.

On the break-* pages, we need to make sure they satisfy the current situation, so:

* Update BCD to say what the current state is. use alternative_name to describe the old property names.
* Update the titles to make it clear that the old names existed, e.g. "break-before (page-break-before)"
* Make sure a cross-browser example is shown that uses page-break-* properties where necessary.

I think what happened here is just straight aliasing, so I don't think any new property values need documenting.

Comment 39

[Emilio Cobos Álvarez [:emilio]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> I think what happened here is just straight aliasing, so I don't think any
> new property values need documenting.

This is not true, these properties are quite special, see:

 * https://drafts.csswg.org/css-cascade-4/#legacy-shorthand
 * https://drafts.csswg.org/css-break/#page-break-properties

Comment 40

[Rachel Andrew]

I think we probably need to keep the page-break-* pages at least for the time being. They have existed for a long time, plus only take the subset of values so I think it probably worth keeping pages and referring them to each other to explain the situation clearly.

Anyway, I can take this one to document if you like.

Comment 41

[Chris Mills [:cmills]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #39)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> > I think what happened here is just straight aliasing, so I don't think any
> > new property values need documenting.
> 
> This is not true, these properties are quite special, see:
> 
>  * https://drafts.csswg.org/css-cascade-4/#legacy-shorthand
>  * https://drafts.csswg.org/css-break/#page-break-properties

Oh, OK ;-)

So does my release note still work?

(In reply to Rachel Andrew from comment #40)
> I think we probably need to keep the page-break-* pages at least for the
> time being. They have existed for a long time, plus only take the subset of
> values so I think it probably worth keeping pages and referring them to each
> other to explain the situation clearly.
> 
> Anyway, I can take this one to document if you like.

WFM Rachel, thanks!

I've added it to the multi-col card:

https://trello.com/c/X0vhUJhk/2-css-multi-col-fx-65

Comment 42

[Emilio Cobos Álvarez [:emilio]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #41)
> So does my release note still work?

Maybe just "The break-* properties have been implemented, and the legacy page-break-* have been aliased to them" or something of that sort?

Comment 43

[Chris Mills [:cmills]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #42)

> Maybe just "The break-* properties have been implemented, and the legacy
> page-break-* have been aliased to them" or something of that sort?

OK, updated, thanks!

Comment 44

[Eric Shepherd [:sheppy]]

Squee! I'm so happy we have this now!

Comment 45

[Rachel Andrew]

I've been doing the docs and data update for this, including splitting up the compat data for multicol and paged media.

• https://developer.mozilla.org/en-US/docs/Web/CSS/break-inside

• https://developer.mozilla.org/en-US/docs/Web/CSS/break-before

• https://developer.mozilla.org/en-US/docs/Web/CSS/break-after

• https://developer.mozilla.org/en-US/docs/Web/CSS/page-break-inside

• https://developer.mozilla.org/en-US/docs/Web/CSS/page-break-before

• https://developer.mozilla.org/en-US/docs/Web/CSS/page-break-after

I have an open PR for some tweaks to the compat data https://github.com/mdn/browser-compat-data/pull/3374 if anyone sees anything wrong, or has better info than me for support let me know.

Comment 46

[Emilio Cobos Álvarez [:emilio]]

Looks like blink supports avoid-column, but other than that looks good, thanks!

Comment 47

[Chris Mills [:cmills]]

All done, so looks like we can DDC this now.