397428 - Multiple printing issues on google maps on Mac OSX

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P2)

Description

[Tony Chung [:tchung]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a9pre) Gecko/2007092404 Minefield/3.0a9pre
Build Identifier: 

There are numerous printing issues when searching for a To/From location on maps.google.com:

1) All text on the printed sheets are blacked out.  Unreadable. (start/end header, turn by turn directions, notes textbox)
2) The Turn by turn directions are completely missing.  Only the smaller map images are present
3) A third page is printed for no reason.  It's completely blank. (pg 3 of 3)

Note: The above is printed in Portrait setting.  Tested on Mac and Windows.

Note 2: this prints and works as expected on Firefox 2.0.0.7.   The issues listed above are not there.

Reproducible: Always

Steps to Reproduce:
1. install latest minefield trunk nightly
2. go to maps.google.com, and search for a Start/End addresses
3. Print to printer
4. Verify the issues above.  Lots of printer problems.
Actual Results:  
Printer issues on maps.google.com.  No repro on 2.0.0.7 builds.

Expected Results:  
No printer issues.

Comment 1

[Tony Chung [:tchung]]

adding blocking flag.  this is a critical User experience, as maps.google.com is a popular site.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Can you retry this with a newer nightly?

Though to separate bugs should be filed -- one for Windows and one for Mac, as the printing backends are very different.

Comment 3

[Tony Chung [:tchung]]

STR 1,2, and 3 are still repro'ing on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a9pre) Gecko/2007100804 Minefield/3.0a9pre

I'll test this on windows, and file a separate bug if still repros.

Comment 4

[Tony Chung [:tchung]]

changing this bug to mac only.  will open a separate bug on windows per comment #2

Comment 5

[Stuart Parmenter]

the missing text on page 2 is probably the same as the windows bug.  not sure about the other bits

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I tried this in my trunk build (clicking on the "Print" link to print, not a direct Cmd-P) and I get a lovely PDF:
http://people.mozilla.com/~roc/bug379428.pdf
Looks perfect. You can even select the text. Too big to attach here though.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I tried doing Cmd-P just in case and I get:
http://people.mozilla.com/~roc/bug379428-cmdP.pdf
Which is also perfect, as far as I can tell.

Comment 8

[Tony Chung [:tchung]]

Hey robert,

i noticed you dont have the Start and End image on your pdf, which exists on page 2.  Can you try enabling that option and try again?    Perhaps thats the culprit.

Comment 9

[Tony Chung [:tchung]]

For me, i see the exact same results on Mac OSX as i do for windows.  Refer to screenshots at: https://bugzilla.mozilla.org/show_bug.cgi?id=399238#c4

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Okay, I can reproduce this now. The precise steps are:
-- Get the directions
-- Click on the "Print" link
-- Enable "show original map view" in the checkbox at the top of the page (this is key)
-- Print (or print preview)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: kind-of reduced testcase

This testcase shows the bug for me. I removed all <script>s and all references to external files. It could still use considerable reduction --- currently 1318 lines...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: more reduction

Down to 1043 lines... I'll have to carry on with this later, unless someone wants to take over the fun job...

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: really reduced testcase!

This is really reduced! The print preview is three blank pages, but it should be one blank page followed by a page with "Head" on it.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes the testcase and Google Maps.

Here's what seems to be happening:

1) The page-break-after property causes creation of an nsPageBreakFrame which sets its height to the available height, eating up all the height of the page. This approach to page-break-after is entirely bogus, but oh well.

2) We carry on trying to place content at the bottom of the first page! This is the way it's supposed to work, believe it or not. The idea is that as soon as we find content that needs some height, it will be pushed to the next page.

3) In this case, we try to place the outer table, so we reflow it with zero available height. That leads us to reflow the inner table with zero available height. There isn't enough space so the inner table body calls nsTableRowGroupFrame::SplitRowGroup.

4) SplitRowGroup reflows the row again. The block frame containing the two <br>s breaks vertically after the first <br>, but the first <br> doesn't fit anyway, so it returns a reflow status of NS_FRAME_NOT_COMPLETE and NS_FRAME_TRUNCATED. This status propagates back up to SplitRowGroup. SplitRowGroup sets rowIsOnPage to PR_FALSE, to indicate that none of the row fits on this page, which is correct, and it therefore does not create a continuation for the row, since we'll try to make the row fit completely on the next page. This is also correct. But it allows the row's reflow status to return to nsTableRowGroup::Reflow and then out as the rowgroup's reflow status. This where we first go seriously wrong.

5) ... because the table, seeing that the rowgroup returns NS_FRAME_NOT_COMPLETE, creates a continuation for the rowgroup.

6) The whole outer table is eventually pushed to the second page because of course none of it fit at the bottom of the first page. We reflow the inner table again. The first row now fits, but nsTableRowGroupFrame::Reflow has this to say:
  // If we have a next-in-flow, then we're not complete
  // XXXldb This used to be done only for the incremental reflow codepath.
  if (GetNextInFlow()) {
    aStatus = NS_FRAME_NOT_COMPLETE;
  }
Indeed the rowgroup has a next-in-flow, albeit one that's empty and has no rows --- the one we created in step 5. So the rowgroup continues to return NS_FRAME_NOT_COMPLETE even though it really is complete.

7) This effectively causes another vertical break at the bottom of the inner table. I'm not sure why "Head" doesn't appear on the third page, but it's probably related to the bogosity of this whole situation and I'm not motivated to debug further given that things have already gone hopelessly wrong.

There are several things wrong here, including the nsTableRowGroupFrame::Reflow code I quoted, but the easiest fix (and I think the safest) is at step 4. If you look at nsTableRowGroupFrame::SplitRowGroup, there are other paths that handle the "we can't push children" case, and they all set aStatus to NS_FRAME_COMPLETE, which is logical because we want to push this entire rowgroup to a new page and try to reflow the row again there instead of splitting our rowgroup. (Those paths also call UndoContinuedRow to destroy a row continuation that was already created; we don't need to do that in my patch because when rowIsOnPage is false, we didn't create a row continuation. The assertion checks that.)

The big picture is that table splitting is poorly/incompletely implemented, even for just the non-incremental/non-dynamic case, even though what we have is already horrendously complex. The continuation model itself is a flawed design.

Not sure who should review this. Let's try Bernd and Boris.

Comment 15

[Bernd]

Comment on attachment 287081 [details] [diff] [review]
fix

I think this is correct, I first saw the patch and thought thats it, and only then the lengthy comment, it looks to me like a clear violation of the contract.

>The continuation model itself is a flawed design.

Robert you are old enough to know the sad history (bug 165772) how this has happened.

There are more bad things here (bug 373400) which slips due to my inability to devote more time to mozilla.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks Bernd!

This bad decision was made in 1998 or before, really...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 287081 [details] [diff] [review]
fix

sr=bzbarky

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: reftests

This can land with the patch

Comment 19

[Mike Schroepfer]

Comment on attachment 287081 [details] [diff] [review]
fix

a+ - note that blocking bugs do not need patch approvals...

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in

Comment 21

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007111404 Minefield/3.0b2pre.  

relnote for Beta 1