1109275 - page-break-inside:avoid ignored for img with display:block

Status: NEW

(Core :: Layout, defect)

Description

[Brandon Frohs]

Attachment: firefox-page-break-inside-img.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141127111021

Steps to reproduce:

1. Open firefox-page-break-inside-img.html
2. View with Print Preview


Actual results:

Second image split between first and second page.


Expected results:

Second image should appear (completely) on second page.

Comment 1

[pratyasmitamishra]

bugday-20141216 : Working as expected.

Comment 2

[MS]

I can confirm the issue with the example provided.
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0

Comment 3

[fantasai]

I think the fix for this is in this bit of nsImageFrame.cpp
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#996
989   if (aPresContext->IsPaginated() &&
990       ((loadStatus & imgIRequest::STATUS_SIZE_AVAILABLE) || (mState & IMAGE_SIZECONSTRAINED)) &&
991       NS_UNCONSTRAINEDSIZE != aReflowState.AvailableHeight() && 
992       aMetrics.Height() > aReflowState.AvailableHeight()) { 
993     // our desired height was greater than 0, so to avoid infinite
994     // splitting, use 1 pixel as the min
995     aMetrics.Height() = std::max(nsPresContext::CSSPixelsToAppUnits(1), aReflowState.AvailableHeight());
996     aStatus = NS_FRAME_NOT_COMPLETE;
997   }

Replace aStatus = NS_FRAME_NOT_COMPLETE with
aStatus = (ShouldAvoidBreakInside(aReflowState)) ? NS_INLINE_BREAK_LINE_BREAK_BEFORE() : NS_FRAME_NOT_COMPLETE;

(... while we're on the topic, why is block fragmentation controlled by macros named NS_INLINE...LINE_BREAK? If they're generic to breaking, shouldn't they have more generic names? Or am I missing something here?)

Comment 4

[Gérard Talbot]

Another test

http://www.gtalbot.org/BugzillaSection/Bug1109275-page-break-inside-avoid-img-display-block-002.html

Chrome 48.0.2564.116 passes this test.

Firefox 44.0.2 and Firefox 47.0a1 buildID=20160224155540 fail this test.

(Anyone can write in here if Edge14.14271 passes this test? ... )

In print preview, you must customize page size (127mm for page width; 76,2mm for page height) and page margins (12,7mm). Chrome's Print Preview can set Page margins manually with the UI.

I can upload a screenshot of actual result and  a screenshot of expected result for that test.

Marking as NEW

I think Component should be "Printing: Output"...

Adding testcase keyword

Comment 5

[fantasai]

Attachment: 3-line patch + 2 testcases

(Haven't run it through try yet; will do once I've got access again.)

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8739582 [details] [diff] [review]
3-line patch + 2 testcases

Don't we need to check that the image really is a block?
aReflowState.mStyleDisplay->IsBlockOutside(this) say?

Please add tests for inline images with/without page-break-inside:avoid.

Both tests fail when I run them locally because class="reftest-print"
is missing in the reference files (they pass when I add that).

FYI, all other page-break-inside:avoid tests are located under
layout/reftests/w3c-css/submitted/css21/pagination/
Since page-break-inside is technically a CSS 2 property and we already
have existing tests there, I'd prefer if you add your tests there too.

FYI, the patch lacks a commit message.

Comment 7

[Mats Palmgren (inactive)]

Fwiw, css-break-3 says for 'break-inside':
"Applies to: 	elements in the normal flow that establish formatting contexts, or that are block containers, table row groups, or table rows"
https://drafts.csswg.org/css-break/#propdef-break-inside

So according to that, 'break-inside' doesn't apply to images with display:block
since they are not "containers" IMO.

CSS 2 is clearer that it does:
"Applies to:  	block-level elements (but see text)"
https://www.w3.org/TR/2011/REC-CSS2-20110607/page.html#page-break-props

BTW, does css-break-3 define the break behavior for atomic inlines anywhere?
I can't find it.

Comment 8

[fantasai]

Filed https://github.com/w3c/csswg-drafts/issues/1904 on the `break-inside` applying to block-level replaced elements bit, since that's just an error in the L3 spec. (It's more correct on some fronts, but forgot to define its behavior for replaced elements.)

Atomic inlines are inside a line box, which is monolithic. They don't get fragmented, they just get sliced if necessary.
See https://www.w3.org/TR/css-break-3/#monolithic and last paragraph of https://www.w3.org/TR/css-break-3/#unforced-breaks

Comment 9

[fantasai]

Attachment: patch

I think this addresses the review comments.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8925304 [details] [diff] [review]
patch

The code change looks fine, but the reftests are using "reftest-print"
which is obsolete -- it should now be "reftest-paged".  Also, the actual
file names don't match the ones in the manifest.  After fixing those
issues locally, when I run the reftests two of them fails:
moz-css21-img-page-break-inside-inline-auto-001.html
moz-css21-img-page-break-inside-inline-avoid-001.html
The test rendering looks OK (two pages), but the reference rendering
is many more pages.

r- because some of the new reftests fails.

Comment 11

[Mats Palmgren (inactive)]

Attachment: updated patch with corrections (but two of the reftests fails)

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.