Alias page-break-* to break-*

RESOLVED FIXED in Firefox 65

Status

()

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: fantasai.bugs, Assigned: emilio, NeedInfo)

Tracking

(Blocks 4 bugs, {dev-doc-complete, DevAdvocacy})

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [DevRel:P1][wptsync upstream], )

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

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

Updated

7 years ago
Blocks: css-break-3

Comment 2

7 years ago
Posted patch fixSplinter Review
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-/
Assignee: nobody → matspal
Attachment #699688 - Flags: review?(fantasai.bugs)
Reporter

Comment 3

7 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?
Attachment #699688 - Flags: review?(fantasai.bugs) → review-

Updated

7 years ago
Duplicate of this bug: 832626

Updated

6 years ago
Blocks: css3test

Comment 5

5 years ago
Mats, do you have time to work on this for Train 30?
Flags: needinfo?(matspal)

Updated

5 years ago
Blocks: 549114

Comment 6

5 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.
Flags: needinfo?(matspal)

Comment 7

5 years ago
OK, I reset the assignee so someone else can look at this. If you find time for this bug, please retake.
Assignee: matspal → nobody
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk

Comment 8

5 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.
Attachment #699688 - Flags: feedback?(dbaron)
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.
Attachment #699688 - Flags: feedback?(dbaron) → feedback+

Comment 10

5 years ago
Thanks for the fast response! Fantasai may have more specific questions but I guess this is enough for now. :)
See Also: → 132035
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).
Blocks: 775622
Reporter

Comment 12

3 years ago
Fwiw, the spec is now in CR. https://www.w3.org/TR/css-break-3/
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
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

3 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
`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.
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.
Flags: needinfo?(dbaron)
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.)
Flags: needinfo?(dbaron)
> 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'.
Reporter

Updated

2 years ago
Component: Layout → CSS Parsing and Computation
Flags: needinfo?(emilio)
Assignee

Updated

7 months ago
Depends on: 1507127
Assignee

Comment 19

7 months 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.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Assignee

Comment 20

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

Comment 21

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

Comment 22

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

Comment 23

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

Updated

7 months ago
Depends on: 1507305
Assignee

Comment 24

7 months 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.
Attachment #9025030 - Attachment is obsolete: true
Assignee

Comment 25

7 months ago
This property has no weird value mapping, so we can just do this.
Assignee

Comment 26

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

Comment 27

7 months 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.
Flags: needinfo?(fantasai.bugs)
Assignee

Comment 28

7 months 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.
Flags: needinfo?(jensimmons)
(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

7 months 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)
Reporter

Comment 31

7 months 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).
Flags: needinfo?(fantasai.bugs)

Comment 32

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

Comment 33

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

Comment 34

7 months 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14312 for changes under testing/web-platform/tests
Whiteboard: [DevRel:P1] → [DevRel:P1][wptsync upstream]
Upstream PR merged
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.
Assignee

Comment 39

6 months 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

Comment 40

6 months 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.
(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
Assignee

Comment 42

6 months 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?
(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!
Squee! I'm so happy we have this now!

Comment 45

4 months ago
Flags: needinfo?(emilio)
Assignee

Comment 46

4 months ago

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

Flags: needinfo?(emilio)

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

You need to log in before you can comment on or make changes to this bug.