Closed Bug 409084 Opened 12 years ago Closed 12 years ago

Print Preview shows 84 pages on nytimes.com front page

Categories

(Core :: Layout: Tables, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: marcia, Assigned: dholbert)

References

()

Details

(Keywords: regression, testcase)

Attachments

(11 files, 4 obsolete files)

28.05 KB, text/html
Details
23.07 KB, text/html
Details
20.85 KB, text/html
Details
444 bytes, text/html
Details
660 bytes, text/html
Details
563 bytes, text/html
Details
13.51 KB, text/plain
Details
1.13 KB, patch
Details | Diff | Splinter Review
415 bytes, text/html
Details
12.31 KB, patch
bernd_mozilla
: review+
dholbert
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
3.28 KB, patch
Details | Diff | Splinter Review
Seen using  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007121905 Minefield/3.0b3pre.

STR:

1. Go to site listed in URL field
2. File | Print Preview

Expected: I would see approximately 5 pages
Actual: I see 84 pages listed. ack.
Is this a regression?
Using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2, it shows 4 pages, although the pages don't show correctly, which is a different issue. adding the regression keyword.
Keywords: regression
user error - I realized after the fact the Vista machine I was using had the scale set to 100%. If it is set to shrink to fit it reduces the page number significantly.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Status: RESOLVED → VERIFIED
Um, I do not think that this bug is invalid. With 125% I also get only ~5 pages. But with 80% I get 76 pages. This looks wrong to me.
OK, feel free to reopen it.  I set it to INVALID because the original reporter claimed it was his own error.
reopening per Comment 4.
Status: VERIFIED → REOPENED
Resolution: INVALID → ---
For me this regressed between 2007-12-02-03 and 2007-12-03-03.
Bernd: Looks like your patch from Bug 373400 caused this bug here. Tested by local backout with the help of FrenchFrgg for finding that check-in. Sorry that there is no minimal testcase for this site yet.
Frank, so this is your subtle way of making Christmas presents to me?? ;-).

A reproducible stripped test case would be nice and would help me a lot. As this is a news page the content may change frequently.
Assignee: nobody → bernd_mozilla
Status: REOPENED → NEW
Keywords: qawanted
Attached file Somewhat minimal testcase (obsolete) —
Thanks to Jesse's Lithium tool reducing went somewhat fast, but: I'm not sure if this is really the correct testcase to demonstrate the bug :|. At the end it more or less went down to the assertion "###!!! ASSERTION: data loss - incomplete row needed more height than available, on top of page: 'rowMetrics.height <= rowReflowState.availableHeight', file f:/m
ozilla/tree-cvsmo/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1093" firing or not, because the page count went down quite fast when minimizing the testcase and one line difference sometimes made print preview display 3 or 4 pages (and fired one assertion). I tested with 125% zoom factor, with higher values it may assert as well.
And next time I'll deliver the present on the 24th ;-).
Oh, the .css files could need some reducing as well... ;)
The "Somewhat minimal testcase" is worksforme, with current trunk build, but I can see the bug on http://nytimes.com/ .
thats why I asked you for help 
Attached file unminimized testcase
This has at least removed all external references, but isn't really minimized yet.
While I tried to minimize the page, I sometimes crash on print preview, I filed bug 409480 for that.
Flags: blocking1.9?
Attached file slightly less
There is only one table left,  is  down to

<table>
 <tbody>
  <tr>
   <td rowspan="2"/><td/>
  </tr>
  <tr><td/></tr>
 </tbody>
</table>

The reminder is fill material. This is probably good enough for debugging. Thanks Martijn.
Attached file more removed
Attachment #294195 - Attachment is obsolete: true
Attached file reduced testcase
+'ing with P2 priority.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
martijn, marcia - can one of you resolve the qawanted added by bernd_mozilla on 12-20?
Sorry, the qawanted keyword is not needed anymore. There is a minimal testcase now, which Bernd made (I would have done it, if I had the time at that moment).
Keywords: qawantedtestcase
I cannot succeed to reproduce on Linux after the landing of the gtk print dialog, but it may be only because print preview doesn't seem to be affected as before by some of the print settings
dholbert: can you take a look?
Assignee: bernd_mozilla → dholbert
Flags: tracking1.9+ → blocking1.9+
When I print-preview the reduced testcases, I get this warning:

  WARNING: data loss - complete row needed more height than available, on top of page: file mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1168

Here's a link to that warning in MXR:
http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.cpp#1169
Rendering of reduced testcases changed between these builds:

2007120204
2007120304

Bonsai link: http://tinyurl.com/26lz2u

bug 373400 seems like the most likely candidate, or possibly bug 404666.
(In reply to comment #29)
> Rendering of reduced testcases changed between these builds:
> 
> 2007120204
> 2007120304

The old (pre-regression-window) rendering of reduced testcase 2 (attachment 310862 [details]) was:
  - Two pages of output
  - iframe is on second page (extending off the bottom of it)
  - All text inside table is *completely missing*

The new rendering is (as described at the top of the testcase):
  - Ten pages of output
  - iframe is on last page (extending off the bottom of it)
  - Text inside the table is split across the first 9 pages, with one line per page

The new rendering is better in that it shows all of the text inside the table, but it's worse in that it takes 10 pages to do this.
Switching OS to All/All, as the reduced testcase behavior is present on Linux as well.  (I think the only platform-specific thing about this bug is how tall we need to make the iframe in order for it to extend off the page and trigger the buggy behavior.)
Blocks: 373400
OS: Windows Vista → All
Hardware: PC → All
Switching component to tables, as this is bug is due to our table-splitting code.
Assignee: dholbert → nobody
Component: Print Preview → Layout: Tables
QA Contact: printing → layout.tables
Assignee: nobody → dholbert
DHolbert - what's the status on this?
No updates on this currently -- I've been working on my other blockers, as table splitting / row-height-distribution seems scary.

I'm looking over this now, though, and I'll post again when I've got more info.
So, the main problem here is with situations like this:

   ---------------
   |       |  B  |
   |       |_____|
   |  A.1  |     |
   |  A.2  |  C  |
   |  A.3  |  .  |
   |  A.4  |  .  |
   |  A.5  |  .  |
   |       |  .  |
   |       |  C  |
   ---------------

 where..
   - Section A is multiple lines of text (5 lines here)
   - Section B's contents don't matter -- it can even be empty
   - Section C is an iframe (or maybe something more general) that is taller than one page

Conceptually, Trunk ends up going through this process:
  1. We notice that the row-group ABC is too tall for the page.
  2. We split up cell "A", placing just the top line in its own row on the first page, and we push the rest of the table to the next page.
  3. Repeat steps 1-2 for every line in A (generating a new row & page for each line)
  4. When we've split off all of A's lines, we hit a fall-back condition where we realize that cell C just isn't going to fit on the page, even though it's by itself, so we print out the data loss warning (see comment 28) and call it quits.


I think the desired behavior would be as follows:
    - Keep cells A and B on the first page, and let C be pushed (by itself) to second page. (It needs its own page so we can show as much of C's height as we possibly can.)

... and here's one optional incremental improvent
   - If cell C is aligned at the very top of the page (i.e. if B is completely empty and there's nothing else above the table), then we'd like to just leave C on the first page, because in this case we don't get any benefit from pushing it to its own page.
Attached patch patch v1 (hack-ish) (obsolete) — Splinter Review
This patch isn't entirely correct, but it demonstrates what's going wrong & roughly what the fix should be, I think.

Basically, on this bug's testcases, we're inadvertantly setting spanningRowBottom to 0 in SplitRowGroup [1], because prevRowFrame->GetRect().YMost() is still 0 in this case. (I think it's 0 here because the previous row just contains the rowspanning cell, and the rowspanning cell's height hasn't propagated up to its row yet.)

And if we have spanningRowBottom == 0, that's bad because this gets passed to SplitSpanningCells, and it's used there to set cellAvailHeight = 0 for a reflow of the spanning cell. [2]  This explains why we just shave off one line at a time in current trunk -- it's because we think we have 0 available space, so we split that cell to be as small as it possibly can, leaving only one line per page.

This patch just acknowledges that prevRowFrame->GetRect().YMost() isn't set yet in this case, and so we'll no longer use it as the value for spanningRowBottom.  Instead, we leave spanningRowBottom at its default value, which is the remaining available height on the page (I think).

Here's how many pages of print-preview output I get pre-patch and post-patch for the reduced testcases:
                      Pre-patch   Post-patch
  reduced testcase:       5            3
  reduced testcase 2:    10            2
  reduced testcase 3:     4            2

(Note that all the unminimized testcases and http://nytimes.com itself all look fine to me, both before and after the patch -- the unminimized testcases are 2pgs and nytimes.com is 3pgs)

[1] http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.cpp#1224
[2] http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.cpp#985.
Bernd: this bit from attachment 290038 [details] [diff] [review] (the patch for bug 373400) looks like a typo...

+      nscoord spanningRowBottom = availHeight;
       if (!rowIsOnPage) {
         if (prevRowFrame) {
-          availHeight -= prevRowFrame->GetRect().YMost();
+          spanningRowBottom = prevRowFrame->GetRect().YMost();


Specifically, those last two lines -- I'm guessing/hoping you intended to keep the '-=', instead of replacing it with '='...?

I'm asking because switching back to a -= would fix this bug.  (I haven't looked at that patch in too much detail, though, so maybe I'm misunderstanding it)
Attached patch patch v2 (Restore original -=) (obsolete) — Splinter Review
(In reply to comment #37)
> switching back to a -= would fix this bug.

This patch does just that.
Attachment #313240 - Attachment is obsolete: true
.. although, I guess that switch was part of the "backing out stuff broken by bug 231823" aspect of your patch, because bug 231823 switched this line from "=" to "-=".  So maybe it wasn't a typo.
bernd / dbaron / Eli:  You guys were involved with writing / reviewing the bug 373400 patch -- any ideas on what's the right thing to to do in this case?

For background, see comment 36 to comment 39 on this bug, noting this in particular:
> Basically, on this bug's testcases, we're inadvertantly setting
> spanningRowBottom to 0 in SplitRowGroup [1], because
> prevRowFrame->GetRect().YMost() is still 0 in this case.
Whiteboard: [needs feedback dbaron/bernd]
I hoped that roc fix for the style height would fix this too but it does not.

>Specifically, those last two lines -- I'm guessing/hoping you intended to keep
>the '-=', instead of replacing it with '='...?
No this was very intentional. Previously the name availHeight was used, this led to the fix in bug 231823, which basically took the variable name literally and did what happens at every other place in the tree when you see a availHeight. Here however it meant the coordinate of the last rows ymost. Davids fix did the correct thing but it did not adapt the splitting cells routines to this new meaning. When the split spanning cells and splitrowgroups disagree we crash. That is bug 373400. Basically I reversed the patch, Eli did the renaming. However I needed to fix bug 231823, I did this by a innocent looking change (see https://bugzilla.mozilla.org/show_bug.cgi?id=422678#c7 for the details) which led to not honored style heights. Robert did cover this by making the row reflow following the layout rules.  But I learned from bug 422678 that split row groups matches the behavior of the row reflow.



Attached patch my hack Splinter Review
this is a hack, it changes the is on top of the page flag inconsistently at one place.

We make two pass reflow where the first unocnstrained height reflow yields a 0 height for the first row and say then that the second row is not on top of the page.
Attachment #313477 - Attachment is obsolete: true
Per discussion with dbaron and roc, wouldn't hold the release for this if it were the last bug on the list (we're at that point now).  Moving to wanted1.9.0.x+.

Will take a patch if reviews are passed.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
(In reply to comment #42)
> this is a hack, it changes the is on top of the page flag inconsistently at one
> place.

Talked to bernd a bit in IRC -- he clarified this as follows:
  <Bernd> there are several places that set this flag [isTopOfPage]
  <Bernd> and I believe that they should be changed consistently

Bernd's patch fixes a place we'd been clearing isTopOfPage due to a false assumption: "// after the 1st row, we can't be on top of the page any more."  

Here are two other places we change isTopOfPage based on similar assumptions:

http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableFrame.cpp#2977
2972   // If this isn't the first row group, then we can't be at the top of the page
2976   if (childX > (thead ? 1 : 0)) {
2977     kidReflowState.mFlags.mIsTopOfPage = PR_FALSE;
2978   }


http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.cpp#446
444   // If this isn't the first row, then we can't be at the top of the page
445   if (kidFrame != GetFirstFrame()) {
446     kidReflowState.mFlags.mIsTopOfPage = PR_FALSE;
447   }
(In reply to comment #44)
> http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableFrame.cpp#2977
> 2972   // If this isn't the first row group, then we can't be at the top of the
> page
> 2976   if (childX > (thead ? 1 : 0)) {
> 2977     kidReflowState.mFlags.mIsTopOfPage = PR_FALSE;
> 2978   }

This testcase is like reduced testcase 3, except it uses rowgroups (tfoot in particular) to exploit the false assumption in the above-quoted code.

* Expected behavior: output 1 page
* Actual behavior (after applying Bernd's hack): output 2 pages
(In reply to comment #44)
> http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.cpp#446
> 444   // If this isn't the first row, then we can't be at the top of the page
> 445   if (kidFrame != GetFirstFrame()) {
> 446     kidReflowState.mFlags.mIsTopOfPage = PR_FALSE;
> 447   }

I've been trying to construct a testcase that fails because of this spot's invalid assumption, but I can't, and I'm not sure it's possible.

It looks like the "mIsTopOfPage" flag here only affects the reflow of the row-frame and its cell children. and the reflow code for rows & cells doesn't actually care about the value of mIsTopOfPage.  So this mIsTopOfPage assignment doesn't actually affect anything, AFAIK.

e.g. this MXR search shows that the only places we actually *react* to the value of mIsTopOfPage are:
 - in nsTableRowGroupFrame.cpp (regarding its own aReflowState)
 - in nsTableFrame.cpp (regarding its direct children, which are row groups)
http://mxr.mozilla.org/seamonkey/search?string=mistopofpage
 - in nsHTMLReflowState::SetTruncated()
Attached patch patch v3 (obsolete) — Splinter Review
This patch contains:
 - Bernd's fix (attachment 314019 [details] [diff] [review])
 - Similar logic in the other two spots where we need it (see comment 44.)
 - two reftests based on reduced testcases 3 and 4.  (I'd like to be able to include one more reftest that tests the third chunk of code, but I don't think that code has any reftestable effects right now -- see comment 46)
Attachment #315696 - Flags: superreview?(dbaron)
Attachment #315696 - Flags: review?(bernd_mozilla)
Comment on attachment 315696 [details] [diff] [review]
patch v3

>       // If this isn't the first row, then we can't be at the top of the page
>       if (kidFrame != GetFirstFrame()) {
>-        kidReflowState.mFlags.mIsTopOfPage = PR_FALSE;
>+        NS_ASSERTION(prevKidFrame, 
>+                     "If we're not on the first frame, we should have a "
>+                     "previous sibling...");
>+        if (prevKidFrame && prevKidFrame->GetRect().YMost() > 0) {
>+          kidReflowState.mFlags.mIsTopOfPage = PR_FALSE;
>+        }
>       }

Looks like you don't need the kidFrame != GetFirstFrame() check anymore.

Also maybe the prevKidFrame=kidFrame assignment should be part of the for loop's increment-part, in case somebody puts a continue statement in the loop.


sr=dbaron, but I'm counting on Bernd to look as well since I don't know this code all that well.
Attachment #315696 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #48)

> Looks like you don't need the kidFrame != GetFirstFrame() check anymore.

Ah, good point -- because the "if (prevKidFrame" will already only pass if we're not the first frame.

Though after I remove the "if (kidFrame != GetFirstFrame())" check, I'll also need to also tweak the assertion that I added, to be something like:
+        NS_ASSERTION(kidFrame == GetFirstFrame()  || prevKidFrame, 
+                     "If we're not on the first frame, we should have a "
+                     "previous sibling...");

> Also maybe the prevKidFrame=kidFrame assignment should be part of the for
> loop's increment-part, in case somebody puts a continue statement in the loop.

Ok, that sounds good.
This updated patch addresses dbaron's superreview comments.  It also updates the comments in the code to match what we're doing now.
(e.g. "If this isn't the first row, then we can't be at the top of the page" becomes "If prev row has nonzero YMost, then we can't be at the top of the page")
Attachment #315696 - Attachment is obsolete: true
Attachment #315888 - Flags: superreview+
Attachment #315888 - Flags: review?(bernd_mozilla)
Attachment #315696 - Flags: review?(bernd_mozilla)
interdiff of my updates, for convenience
BTW, patch v3a passes reftests. (just verified)
Attachment #315889 - Attachment description: interdiff between fixes v3 and v3a → interdiff between patches v3 and v3a
Status: NEW → ASSIGNED
Whiteboard: [needs feedback dbaron/bernd] → [needs review bernd]
Attachment #315888 - Flags: review?(bernd_mozilla) → review+
Comment on attachment 315888 [details] [diff] [review]
patch v3a (addresses comment 48 and fixes code-comments)

Requesting approval, per comment #43:

> ...wouldn't hold the release for this...
> Will take a patch if reviews are passed.

Low-risk, fixes annoying printing issue on popular site, and includes tests.
Attachment #315888 - Flags: approval1.9?
> Low-risk

Table printing is not low risk due to its nearly missing test coverage. However I think that the benefit is worth the risk.
Whiteboard: [needs review bernd]
Comment on attachment 315888 [details] [diff] [review]
patch v3a (addresses comment 48 and fixes code-comments)

a1.9=beltzner
Attachment #315888 - Flags: approval1.9? → approval1.9+
patch v3a checked in.

RCS file: /cvsroot/mozilla/layout/reftests/bugs/409084-1-ref.html,v
done
Checking in reftests/bugs/409084-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/409084-1-ref.html,v  <--  409084-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/409084-1a.html,v
done
Checking in reftests/bugs/409084-1a.html;
/cvsroot/mozilla/layout/reftests/bugs/409084-1a.html,v  <--  409084-1a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/409084-1b.html,v
done
Checking in reftests/bugs/409084-1b.html;
/cvsroot/mozilla/layout/reftests/bugs/409084-1b.html,v  <--  409084-1b.html
initial revision: 1.1
done
Checking in reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.445; previous revision: 1.444
done
Checking in tables/nsTableFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableFrame.cpp,v  <--  nsTableFrame.cpp
new revision: 3.723; previous revision: 3.722
done
Checking in tables/nsTableRowGroupFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableRowGroupFrame.cpp,v  <--  nsTableRowGroupFrame.cpp
new revision: 3.409; previous revision: 3.408
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.