371449 - restrict 'page-break-before' and 'page-break-after' to block-level elements

Status: NEW

(Core :: Layout: Block and Inline, defect)

Description

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

In bug 9458 comment 87, bzbarsky wrote:
> Do we need to add NS_STYLE_DISPLAY_INLINE_TABLE in the condition in
> PageBreakBefore()?  Then again, inline-table is not a block-level element, but
> I don't see where we restrict page-break to those...

In bug 9458 comment 88, dbaron wrote:
> We really should restrict it to block-level elements; if we don't, that's a
> bug.

I'm not sure what the status is here, but I'm filing the bug so I can close bug 9458.

Comment 1

[Andrew Hagen]

All/all. Blocking bug 132035.

Comment 3

[Thomas Wisniewski]

Attachment: 371449-do_not_honor_page-break-before_or_after_for_inline_elements.diff

Is this simple patch anywhere close to correct?

Try seems fine with the change, at least (the intermittents seem unrelated): https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d9e1189fa9

Comment 4

[Thomas Wisniewski]

Actually the try-run I meant to link was this one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf44efe12c68  (the other was for an earlier version of the patch).

Comment 5

[Gérard Talbot]

Actual tests
------------

"The 'page-break-after' property must be applied to block-level elements and may be applied to other elements."
http://test.csswg.org/suites/css2.1/latest/html4/page-break-after-002.htm


"The 'page-break-before' property applies to block-level elements only."
http://test.csswg.org/suites/css2.1/latest/html4/page-break-before-002.htm

These 2 tests somewhat, somehow contradicts themselves, most likely because of leniency ("may also apply these properties to other elements") of CSS2.1 ...

- - - - - - -

David, 

I think CSS2.2 should specifically states page-break-[ after | before ] must *not* apply to inline-level elements. But that is not my decision.

- - - - - - -

2 tests are missing in the test suite in

http://test.csswg.org/suites/css2.1/20110323/html4/chapter-13.html#s13.3.1

One which would specifically test an inline-block and one which would specifically test an inline-table.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 8793620 [details] [diff] [review]
371449-do_not_honor_page-break-before_or_after_for_inline_elements.diff

Sorry, I'm not sufficiently up-to-date with mozilla code to be a good reviewer. I'll file a bug to get me removed from the list of layout peers.

Comment 7

[Simon Montagu :smontagu]

... except that that seems to have happened 4 months ago already without asking my opinion on the subject.

Thomas, where did you get my name from as potential reviewer?

Comment 8

[Thomas Wisniewski]

Odds are that I simply got your name as a drop-down suggestion when I submitted the patch to Bugzilla (back when it used to suggest reviewers).

Based on what I'm seeing now, I wouldn't have chosen you as the reviewer, so it's safe to say that whatever it was that mislead me is gone now.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8793620 [details] [diff] [review]
371449-do_not_honor_page-break-before_or_after_for_inline_elements.diff

Review of attachment 8793620 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

> Is this simple patch anywhere close to correct?

It's probably close! (see nits below)  Have you tested locally see if it changes our behavior on inline-level stuff with "page-break-before|after"?

Before this lands, it should include some reftests that fail without the fix & pass with the fix, to demonstrate that the change is actually making a difference.

Particularly:
 - reftest with <span> (to test non-replaced inlines)
 - reftest with <img> or <canvas> (to test replaced inlines)
 - Each of those ^^ reftests should exercise both "page-break-before" and "page-break-after" (or you could test those each in their own test, and have twice as many tests, if you like).

You'll want to use "reftest-print" in these reftests, too, which is how you can end up exercising the printing codepath. (That makes the reftest render to tiny "printed pages", around 3 inches tall or thereabouts). See https://dxr.mozilla.org/mozilla-central/source/layout/reftests/printing/966419-2.html (or https://dxr.mozilla.org/mozilla-central/search?q=reftest-print ) for example usage.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +5837,5 @@
>      aState.mPresContext->IsPaginated() &&
>      !display->IsAbsolutelyPositionedStyle() &&
>      !(aParentFrame &&
>        aParentFrame->GetType() == nsGkAtoms::gridContainerFrame) &&
> +    !(bits & FCDATA_IS_INLINE) &&

I'm not sure this is the correct flag to check.

The documentation for this bit says:
 /* If FCDATA_IS_INLINE is set, then the frame is a non-replaced CSS
    inline box. */
https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/layout/base/nsCSSFrameConstructor.h#660-662

Note the "non-replaced" there. The spec doesn't seem to restrict this special-case to *non-replaced* inlines, so our code shouldn't either.

I suspect you really want to be checking FCDATA_IS_LINE_PARTICIPANT here.

@@ -5841,3 @@
>      !(bits & FCDATA_IS_TABLE_PART) &&
>      !(bits & FCDATA_IS_SVG_TEXT);
> -

Please don't remove this blank line. It's helpful to visually separate the giant multi-line assignment from the logic that follows it.

Comment 10

[Thomas Wisniewski]

Thanks for the feedback! I'll try to get this one across the finish line as soon as I can.

>Have you tested locally see if it changes our behavior on inline-level stuff with "page-break-before|after"?

I do recall testing that it seemed to do the trick in print-preview, but clearly it must have slipped my mind to try testing replaced inlines as well. I'll definitely ensure that the test-cases cover them as well (I appreciate the pointers on how to write reftests for this, btw).

Comment 11

[Daniel Holbert [:dholbert]]

Thanks! Since you've used Try before, I'm assuming you also know how to run reftests locally (basially ./mach reftest path/to/whatever/reftest.list) but let me know if you need help with that, too.

(You'll definitely want to run the tests locally to be sure they fail without the patch & pass with it -- and the reftest harness is the only way to be sure of that, since that's the only place "reftest-print" will be respected.)

Comment 12

[Thomas Wisniewski]

Attachment: 371449-do_not_honor_page-break-before_or_after_for_inline_elements.diff

It seems that FCDATA_IS_LINE_PARTICIPANT is not set for img/canvas and such (it's false for them). As such, I've opted to pull the isInline logic up, and used that as the basis of my decision instead. Hopefully that's a reasonable enough approach?

I also added a couple of reftests as suggested; one testing that inline/replaced elements don't allow breaks, and one testing that block elements do allow breaks. Try seems alright with this version, at least (just unrelated-seeming issues, and I can't retrigger those jobs for some reason right now): https://treeherder.mozilla.org/#/jobs?repo=try&revision=56a57ba8d5ba9013228caeb11f8a64cc569f86f9

Comment 13

[Boris Zbarsky [:bzbarsky]]

The "but may also be false for some inlines" bit makes it likely that this boolean doesn't quite do what you want in some circumstances, right?

Comment 14

[Thomas Wisniewski]

Yes, I fear so, but I'm not familiar enough with the code to know what cases it might be missing (or whether we'd even know that information at this point in the code).

Comment 15

[Boris Zbarsky [:bzbarsky]]

I think the main (only?) thing it's missing is that isInline will be false for things that are position:absolute/fixed and float left:left/right, even though those are typically (but not always!) "inline" in the sense this code currently cares about.

What behavior should page-break-* have for those?

What behavior should it have for table parts, both in an inline-table and in a table?

Comment 16

[Boris Zbarsky [:bzbarsky]]

And should the behavior for a table part in an inline-table depend on whether the inline-table is implicit or explicit?  I think with your patch it does.

Comment 17

[Thomas Wisniewski]

If we're going by CSS 2.1, then the spec says this about break before/after [1]:

>Applies to:  	block-level elements (but see text) 
> 
>User Agents must apply these properties to block-level elements in the
>normal flow of the root element. User agents may also apply these
>properties to other elements, e.g., 'table-row' elements.


If we're going by the renamed "break-before/after" properties in the CSS draft spec for Breaks [2], then it says:

>Applies to:    block-level elements, table row groups, table rows (but see prose)
> 
>User Agents must apply these properties to boxes in the normal flow
>of the fragmentation root. User agents should also apply these
>properties to floated boxes whose containing block is in the normal
>flow of the root fragmented element. User agents may also apply
>these properties to other boxes. 


So I'm guessing absolute/fixed positioning should disallow breaks, as you say. But floating may require some thought if we want to support the draft spec's definition. I did see this code in the file:

>    item->mIsBlock = !isInline &&
>                     !display->IsAbsolutelyPositionedStyle() &&
>                     !display->IsFloatingStyle() &&
>                     !(bits & FCDATA_IS_SVG_TEXT);

Yet when I try to use that as the basis for the decision, other reftests related to floats fail (moz-css21-float-page-break-inside-avoid-4.html to -8.html and bugs/585598-2.xhtml), so I'm guessing I'll need to figure out if "the containing block is in the normal flow of the root fragment element" as well.

Also, it seems up to us whether we handle table parts, but since the draft spec mentions table-rows and table-row-groups, we might as well handle those.

I'm not seeing any mention of explicit vs implicit inline-table considerations, so I'm guessing that does need to be accounted-for... do you have any hints on what I should be doing for that instead?

Comment 18

[Boris Zbarsky [:bzbarsky]]

For the table bits, you should just treat table parts consistently regardless of whether their parent is an inline frame.  I can see an argument for allowing pagination after table rows/rowgroups of a table but not inline-table, maybe.  It's not a very strong argument (e.g. a table can be inside an inline-block), and in any case that's all a spec issue...  The only part I can say for sure is that the "table part whose parent is an inline" case should not behave differently from any other table part inside an inline table.  In this case, allowing page-break-before on both is likely the smallest change from our current behavior.

For the rest, the basic problem you're having is that the code you're changing is early enough in the frame construction process that we don't know things like whether floats will actually float (e.g. because there are no actual block ancestors at all here). At least that used to be the case; I'm not sure whether we can still end up there.  So another option is to insert the break-before/after frame construction items here unconditionally, but then when it comes time to construct frames from them (such that we know things like what our ancestor chain looks like) just skip some of them.  That would make things like checking the spec's requirements about being in-flow in the fragmentation root much simpler.

Comment 19

[Thomas Wisniewski]

Comment on attachment 8812956 [details] [diff] [review]
371449-do_not_honor_page-break-before_or_after_for_inline_elements.diff

Ah, I see... thanks for the explanation. I'll try to find the time to figure that out as soon as I can. (Canceling review for now).