fantasai Reporter
	Description • 12 years ago

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.

	fantasai Reporter
	Comment 1 • 12 years ago

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

	Mats Palmgren (inactive)
	Comment 2 • 12 years ago

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-/

	fantasai Reporter
	Comment 3 • 11 years ago

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?

	Florian Bender
	Comment 5 • 10 years ago

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

	Mats Palmgren (inactive)
	Comment 6 • 10 years ago

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

	Florian Bender
	Comment 7 • 10 years ago

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

	Florian Bender
	Comment 8 • 10 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 9 • 10 years ago

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.

	Florian Bender
	Comment 10 • 10 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 11 • 9 years ago

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

	fantasai Reporter
	Comment 12 • 8 years ago

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

	Jen Simmons [:jensimmons]
	Comment 13 • 8 years ago

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

	Florian Rivoal
	Comment 14 • 8 years ago

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

	Jen Simmons [:jensimmons]
	Comment 15 • 7 years ago

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

	Simon Sapin (:SimonSapin)
	Comment 16 • 7 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 17 • 7 years ago

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

	Simon Sapin (:SimonSapin)
	Comment 18 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 19 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 20 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 21 • 6 years ago

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

	fantasai Reporter
	Comment 22 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 23 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 24 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 25 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 26 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 27 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 28 • 6 years ago

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.

	Sebastian Zartner [:sebo]
	Comment 29 • 6 years ago

(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

	Mats Palmgren (inactive)
	Comment 30 • 6 years ago

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)

	fantasai Reporter
	Comment 31 • 6 years ago

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

	Pulsebot
	Comment 32 • 6 years ago

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c228095e647e
Introduce break-inside, and alias page-break-inside to it. r=heycam

	Pulsebot
	Comment 33 • 6 years ago

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f97f45821194
Introduce the concept of legacy shorthands. r=heycam

	Pulsebot
	Comment 34 • 6 years ago

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

	Noemi Erli[:noemi_erli]
	Comment 35 • 6 years ago
bugherder

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 36 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 37 • 6 years ago

Upstream PR merged

	Chris Mills [:cmills]
	Comment 38 • 5 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 39 • 5 years ago

(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

	Rachel Andrew
	Comment 40 • 5 years ago

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.

	Chris Mills [:cmills]
	Comment 41 • 5 years ago

(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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 42 • 5 years ago

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

	Chris Mills [:cmills]
	Comment 43 • 5 years ago

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

	Eric Shepherd [:sheppy]
	Comment 44 • 5 years ago

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