Closed Bug 368079 Opened 13 years ago Closed 12 years ago

we leave room for bottom padding/border at a column/page break

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: fantasai.bugs)

References

Details

Attachments

(10 files, 2 obsolete files)

At page breaks, we leave extra space for bottom padding/border at column/page breaks, but we shouldn't be leaving this space.

Steps to reproduce:
 * load attached testcase
 * look at bottom edge of image on left

Expected results:  no yellow underneath left image

Actual results: 5px of yellow underneath left image
This is pretty hard to fix. We need to stop subtracting bottom-padding and bottom-margins from the available height, but then we also need to make it so when we discover that bottom-padding or bottom-margin makes us exceed the available height, back up and put in a page/column break at the last available position (much like we are now doing with inlines). Whenever we get around to doing this, we should also implement the page-break-*/column-break-* properties at the same time.
This was cut/pasted from the source window in Firefox 2.01.
This is the .PDF file generated when "printed" to PDF from firefox.
I also have an equivalent .PDF which renders correctly from IE, however the resultant IE PDF file is 12 times as large as the Firefox version and is too big to attach, even when zipped.  If you desire to see it. Send me an e-mail request and I will send it back.
Dave H.
Attachment #255755 - Attachment description: PDF output from attached source page → PDF output from attached source page. Note missing entry at each page break
We know exactly what this bug is, so more examples are not helpful.  Either they're not this bug, in which case they belong elsewhere, or they duplicate what's already here.  Either way, it's just clutter that makes this bug harder to fix.
Attached file testcase 2
A more complicated testcase.
Attached patch patch (obsolete) — Splinter Review
This fixes the testcases and doesn't seem to regress any reftests. I'm not sure what case you're concerned about, roc. I would expect the code in nsBlockFrame::ComputeFinalSize to make sure there's room for the bottom border+padding.
Assignee: nobody → fantasai.bugs
Status: NEW → ASSIGNED
Attachment #296454 - Flags: superreview?(roc)
Attachment #296454 - Flags: review?(roc)
Hm, sorry, it fixes one testcase but not the other... so I guess there's two problems here.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #296454 - Attachment is obsolete: true
Attachment #296459 - Flags: superreview?(roc)
Attachment #296459 - Flags: review?(roc)
Attachment #296454 - Flags: superreview?(roc)
Attachment #296454 - Flags: review?(roc)
So what happens when bottom border or padding falls on the page boundary? I think with this patch, it remains on the first page and is partially or completely chopped off?
No, the code in ComputeFinalSize pushes the border out to the second page. The testcase I'm attaching now passes with this patch.
Another test that doesn't involve overflow containers.
> No, the code in ComputeFinalSize pushes the border out to the second page. The
> testcase I'm attaching now passes with this patch.

Can you explain how that happens? It must be this code, presumably:

    if (NS_FRAME_IS_COMPLETE(aState.mReflowStatus)) {
      if (computedHeightLeftOver > 0 &&
          NS_UNCONSTRAINEDSIZE != aReflowState.availableHeight &&
          aMetrics.height > aReflowState.availableHeight) {
        // We don't fit and we consumed some of the computed height,
        // so we should consume all the available height and then
        // break.  If our bottom border/padding straddles the break
        // point, then this will increase our height and push the
        // border/padding to the next page/column.
        aMetrics.height = PR_MAX(aReflowState.availableHeight,
                                 aState.mY + nonCarriedOutVerticalMargin);
        NS_FRAME_SET_INCOMPLETE(aState.mReflowStatus);

but here computedHeightLeftOver should be 0.
computedHeightLeftOver is not zero on the first-in-flow because there are no prev-in-flows. It's zero on the next-in-flow/last-in-flow because we used up all the computed height already, but in that case we're complete so we go to the else clause.
Ok, that didn't make any sense. Change "but in that case..." to 
in that case we keep only
  aMetrics.height = borderPadding.top + computedHeightLeftOver +
                    borderPadding.bottom
which since borderPadding.top =- computedHeightLeftOver == 0 is equivalent to
  aMetrics.height = borderPadding.bottom
Comment on attachment 296459 [details] [diff] [review]
patch v2

okay, thanks for all that. If you could make those into reftests or something ... :-)

I'm not sure about this for 1.9 though.
Attachment #296459 - Flags: superreview?(roc)
Attachment #296459 - Flags: superreview+
Attachment #296459 - Flags: review?(roc)
Attachment #296459 - Flags: review+
Attached file new testcase
Ok, wrote reftests. I'm replacing dbaron's original testcase with this.
Attached file reftests.zip
Wrt 1.9, this isn't a very important fix, but it's also pretty low-risk; it's mainly an arithmetic change. It makes certain kinds of complex pagination reftests easier, which is mainly why I wanted to fix it. :) Your call.
Comment on attachment 296459 [details] [diff] [review]
patch v2

Making other testing easier is actually quite a good reason. The patch is quite low risk IMHO, let's take it.
Attachment #296459 - Flags: approval1.9+
Fix checked in, nsBlockFrame.cpp v3.918 nsBlockReflowState.cpp v3.536
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Looks like the patch was backed out.

As I'm sure you know, you need to adjust the reftest.list to mark 368020-1.html as passing.  It even has a comment pointing to this bug; I think writing that test was what made me file this bug.

Not sure why 397428-1.html was failing, though.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yeah, I'm still trying to figure that one out. The top line of the table just disappears, even if it has content or is 40pt high. Adding a 1px top margin to the table fixes it. The problem doesn't seem to exist at all in multicolumn pagination.
Attached patch patch v3Splinter Review
This seems to fix the problem.
Attachment #296459 - Attachment is obsolete: true
Attachment #297501 - Flags: superreview?(roc)
Attachment #297501 - Flags: review?(roc)
Attachment #297501 - Flags: approval1.9?
Attachment #297501 - Flags: approval1.9?
Comment on attachment 297501 [details] [diff] [review]
patch v3

+    aMetrics.height = (aState.mY > aReflowState.availableHeight) ?
+                      aState.mY : aReflowState.availableHeight;

PR_MAX
Attachment #297501 - Flags: superreview?(roc)
Attachment #297501 - Flags: superreview+
Attachment #297501 - Flags: review?(roc)
Attachment #297501 - Flags: review+
Checked in nsBlockReflowState.cpp v3.538 nsBlockFrame.cpp v3.920
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 414061
Depends on: 414255
Depends on: 427017
You need to log in before you can comment on or make changes to this bug.