Closed
Bug 409084
Opened 17 years ago
Closed 16 years ago
Print Preview shows 84 pages on nytimes.com front page
Categories
(Core :: Layout: Tables, defect, P2)
Core
Layout: Tables
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.
Comment 1•17 years ago
|
||
Is this a regression?
Reporter | ||
Comment 2•17 years ago
|
||
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
Reporter | ||
Comment 3•17 years ago
|
||
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: 17 years ago
Resolution: --- → INVALID
Updated•17 years ago
|
Status: RESOLVED → VERIFIED
Comment 4•17 years ago
|
||
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.
Comment 5•17 years ago
|
||
OK, feel free to reopen it. I set it to INVALID because the original reporter claimed it was his own error.
Reporter | ||
Comment 6•17 years ago
|
||
reopening per Comment 4.
Status: VERIFIED → REOPENED
Resolution: INVALID → ---
Comment 7•17 years ago
|
||
For me this regressed between 2007-12-02-03 and 2007-12-03-03.
Comment 8•17 years ago
|
||
Check-ins in that timeframe: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-12-02+03%3A00&maxdate=2007-12-03+03%3A00&cvsroot=%2Fcvsroot
Comment 9•17 years ago
|
||
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.
Comment 10•17 years ago
|
||
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.
Comment 11•17 years ago
|
||
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 ;-).
Comment 12•17 years ago
|
||
Oh, the .css files could need some reducing as well... ;)
Comment 13•17 years ago
|
||
The "Somewhat minimal testcase" is worksforme, with current trunk build, but I can see the bug on http://nytimes.com/ .
Comment 14•17 years ago
|
||
thats why I asked you for help
Comment 15•17 years ago
|
||
This has at least removed all external references, but isn't really minimized yet.
Comment 16•17 years ago
|
||
While I tried to minimize the page, I sometimes crash on print preview, I filed bug 409480 for that.
Updated•17 years ago
|
Flags: blocking1.9?
Comment 17•17 years ago
|
||
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.
Comment 18•17 years ago
|
||
Updated•17 years ago
|
Attachment #294195 -
Attachment is obsolete: true
Comment 19•17 years ago
|
||
Comment 20•17 years ago
|
||
+'ing with P2 priority.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment 21•17 years ago
|
||
martijn, marcia - can one of you resolve the qawanted added by bernd_mozilla on 12-20?
Comment 22•17 years ago
|
||
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).
Comment 23•16 years ago
|
||
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
Comment 24•16 years ago
|
||
dholbert: can you take a look?
Assignee: bernd_mozilla → dholbert
Flags: tracking1.9+ → blocking1.9+
Assignee | ||
Comment 25•16 years ago
|
||
Assignee | ||
Comment 26•16 years ago
|
||
Assignee | ||
Comment 27•16 years ago
|
||
Assignee | ||
Comment 28•16 years ago
|
||
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
Assignee | ||
Comment 29•16 years ago
|
||
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.
Assignee | ||
Comment 30•16 years ago
|
||
(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.
Assignee | ||
Comment 31•16 years ago
|
||
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.)
Assignee | ||
Comment 32•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee: nobody → dholbert
Comment 33•16 years ago
|
||
DHolbert - what's the status on this?
Assignee | ||
Comment 34•16 years ago
|
||
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.
Assignee | ||
Comment 35•16 years ago
|
||
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.
Assignee | ||
Comment 36•16 years ago
|
||
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.
Assignee | ||
Comment 37•16 years ago
|
||
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)
Assignee | ||
Comment 38•16 years ago
|
||
(In reply to comment #37) > switching back to a -= would fix this bug. This patch does just that.
Assignee | ||
Updated•16 years ago
|
Attachment #313240 -
Attachment is obsolete: true
Assignee | ||
Comment 39•16 years ago
|
||
.. 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.
Assignee | ||
Comment 40•16 years ago
|
||
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]
Comment 41•16 years ago
|
||
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.
Comment 42•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #313477 -
Attachment is obsolete: true
Comment 43•16 years ago
|
||
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+
Assignee | ||
Comment 44•16 years ago
|
||
(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 }
Assignee | ||
Comment 45•16 years ago
|
||
(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
Assignee | ||
Comment 46•16 years ago
|
||
(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()
Assignee | ||
Comment 47•16 years ago
|
||
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+
Assignee | ||
Comment 49•16 years ago
|
||
(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.
Assignee | ||
Comment 50•16 years ago
|
||
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)
Assignee | ||
Comment 51•16 years ago
|
||
interdiff of my updates, for convenience
Assignee | ||
Comment 52•16 years ago
|
||
BTW, patch v3a passes reftests. (just verified)
Assignee | ||
Updated•16 years ago
|
Attachment #315889 -
Attachment description: interdiff between fixes v3 and v3a → interdiff between patches v3 and v3a
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs feedback dbaron/bernd] → [needs review bernd]
Attachment #315888 -
Flags: review?(bernd_mozilla) → review+
Assignee | ||
Comment 53•16 years ago
|
||
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?
Comment 54•16 years ago
|
||
> 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.
Comment 55•16 years ago
|
||
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+
Assignee | ||
Comment 56•16 years ago
|
||
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: 17 years ago → 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: wanted1.9.0.x+
You need to log in
before you can comment on or make changes to this bug.
Description
•