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

ASSIGNED
Assigned to

Status

()

Core
Layout
ASSIGNED
3 years ago
a month ago

People

(Reporter: Brandon Frohs, Assigned: fantasai)

Tracking

(Blocks: 1 bug, {dev-doc-needed, testcase})

Trunk
dev-doc-needed, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8533919 [details]
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

3 years ago
bugday-20141216 : Working as expected.

Updated

3 years ago
Component: Untriaged → Layout
Product: Firefox → Core

Comment 2

2 years ago
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
(Assignee)

Comment 3

2 years ago
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

2 years ago
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
Blocks: 132035
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Linux → All
Hardware: x86_64 → All
Version: 34 Branch → Trunk
(Assignee)

Updated

2 years ago
Assignee: nobody → fantasai.bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
Created attachment 8739582 [details] [diff] [review]
3-line patch + 2 testcases

(Haven't run it through try yet; will do once I've got access again.)
Attachment #8739582 - Flags: review?(mats)
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.
Attachment #8739582 - Flags: review?(mats) → review-
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.
Keywords: dev-doc-needed
(Assignee)

Comment 8

2 months ago
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
(Assignee)

Comment 9

a month ago
Created attachment 8925304 [details] [diff] [review]
patch

I think this addresses the review comments.
Attachment #8925304 - Flags: review?(mats)

Updated

a month ago
Attachment #8925304 - Attachment is patch: true
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.
Attachment #8925304 - Flags: review?(mats) → review-
Created attachment 8926460 [details] [diff] [review]
updated patch with corrections (but two of the reftests fails)
Attachment #8739582 - Attachment is obsolete: true
Attachment #8925304 - Attachment is obsolete: true
Attachment #8926460 - Flags: review-
You need to log in before you can comment on or make changes to this bug.