609227 - Cannot print / print-preview messages from a google webmail account

Status: VERIFIED FIXED

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

Description

[Maniac Vlad Florin (:vladmaniac)]

Build ID:

Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101102 Firefox/4.0b8pre

=========================================================================

Prerequisites: 
1. Clean profile
2. No extensions 
3. No custom themes/personas
4. gmail account

==========================================================================
Steps:
1. Login to gmail
2. Open some mail from inbox 
3. Go to File->Print from Firefox Menu

==========================================================================
Expected: 
Mail should print 

==========================================================================
Actual: 
You will get a blank page
 
==========================================================================
Note: 
Printing works if done via gmail menu. 
Also, printing is OK with Safari. 
Could this be some iframes or JS issues?

Comment 1

[juan becerra [:juanb]]

I'm nominating this because it goes against the expectation that File->Print should print an email document, which can be done if you use Safari.

Next steps would be to find if faking the user agent could perhaps fix this, and it not we should find a regression range.

Comment 2

[Daniel Holbert [:dholbert]]

Confirmed on Linux -- print-preview is entirely blank (aside from the print header/footer -- page title / page number / date / etc) when I've got a gmail message open.

Comment 3

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

I'll let this block, but if it turns out to be hard we should unblock.

Comment 4

[Mats Palmgren (inactive)]

Regression from bug 129941 confirmed by local backout.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So this is _partially_ a web page bug.  The page has this style:

  html, body { width: 100%; height: 100%; overflow: hidden }

applying.  That means that only the first page of content should be shown (since the body has hidden overflow and will be exactly one page in height).

The problem on our end seems to be that we end up pushing the iframe that has the message text to the second page for some reason....

Comment 6

[Boris Zbarsky [:bzbarsky]]

And in particular, our page height is 52960 units, the iframe in question is 52960 units tall, but the line it's on ends up being 53216 units high and at y=0.

The only things on that line other than the iframe are some placeholders (which end up at y=52960 height=0, and some Text frames containing only "\n", which end up at y=52080 and height == 1120.  The deal here is, presumably, the fact that the iframe sits on the baseline, so the actual line its on ends up taller than the iframe due to descenders.  This looks like correct behavior to me, in standards mode.

The source of the remaining 16 units is that mMinLineHeight in nsLineLayout::VerticalAlignFrames seems to be 1151, and yTop ends up -895 while yBottom ends up 256.  256 + 52960 == 53216, qed.

If I set the iframe to vertical-align: text-bottom to work around the baseline issue, then when we're doing the whole min line height thing I have:

$51 = -52720
(gdb) p maxY
$52 = 240
(gdb) p yTop
$53 = -895
(gdb) p yBottom
$54 = 256

so we still end up with a line that's 16 units taller than the iframe.  I have no idea why yBottom ends up 256 while the text-bottom alignment puts the bottom at 240...  Almost feels like a rounding error in the font code.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, and this used to not be a problem because the body is overflow:hidden, so we created a scrollframe for it, and then had the too-tall line inside the scrollframe, but that was OK, because the scrollframe didn't try to split.  But we do want the block to split here; that's what bug 129941 is all about.

Oh, and the "extra" 16 units are from the fact that the line-height is _bigger_ than the font-size.  Note that 1151 > 1120, where the latter is the font size.  16 is about half the difference.  I'm sure this ends up making sense if we go through the line layout gunk involved... or something.  I'm a little surprised the difference is only 0.5px, by the way.

In any case, the only ways I see of fixing this bug are:

1)  Back out bug 129941.
2)  Do something to make the content paint in spite of the splitting and
    without making other content, which really shouldn't paint, paint (but the
    printout will still be 2 pages, note).
3)  Do something to make us return true from PlaceLine() in this case.  Note
    that the line misses being placed by about 4px (the descender height); the
    line-height bit adds another quarter-px or so.
4)  Get Google to set line-height:0 in their print stylesheet (which they
    already have; it totally reformats the page) for the <body>, which solves
    all these problems... except for the one where only the first page of the
    conversation will print.
5)  Get Google to set the <iframe> to display:block when printing.

Google can't remove the height:100% on the <body> and <iframe>, because then the iframe would end up 150px tall or some such, of course...  Ideally they could get the iframe to size based on its contents, but there's no way to do that right now.  So no matter what you will only be able to print the first page of your email, unless you print the frame with the mail in it directly.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, and one more thing.  The reason PlaceLine returns false is that this is not the block's first line.  It's the _third_ line.  The first line is a 0-height line with some text and placeholders on it.  The second line is a 0-height line with a block on it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490819 [details] [diff] [review]
Proposed fix

I wasn't sure whether to test for "> 0" or "!= 0", and I wasn't sure whether to test aState.mY or aLine->mBounds.y.  But I suspect that these last two are always equal, and pushing things that are starting at y < 0 seems silly to me.  They won't fit any better in the next continuation...

This all assumes the original "always place our first line" logic makes sense to start with, which assumes that either continuations are equal height or we push things as needed so that we don't end up with an overflowing first-continuation while the next-continuation is taller, I guess.  I have no idea whether that assumption is true.  roc, do you?

I did check the provenance of that "first line" check and the comment I'm changing, and it's:

  3.155 <[e-mail]> 1998-12-05 08:02
  Major spankage; refactored code; common base class for block/inline frames

so I think it's safe to assume it's there in the form it's in just because, not for any particular reason....  ;)

Comment 11

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

(In reply to comment #10)
> This all assumes the original "always place our first line" logic makes sense
> to start with, which assumes that either continuations are equal height or we
> push things as needed so that we don't end up with an overflowing
> first-continuation while the next-continuation is taller, I guess.  I have no
> idea whether that assumption is true.  roc, do you?

I think the assumption is that the next continuation will always have at least as much available height as this continuation. I think that assumption is true for now. It wouldn't be true, for example, if we could flow text into columns with different fixed heights. But we can't.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you double-check (and maybe even add a reftest-print reftest for) that in the case where we have:

<div> (zero height)
  <large float>
</div>
<div>
  <inline-block that doesn't fit next to float>
</div>

we push the inline-block to the next page?  (In the case where the placeholder is on the same line, I think we'd split the line, but I'm not sure we'd split the line if the inline-block is the first thing on it.  It might even be worth checking both with and without whitespace prior to the inline-block.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Could you double-check (and maybe even add a reftest-print reftest for)

Did both.  Basic test structure I used:

    <div>
      <div style="float: left; width: 3.5in; height: 1.5in;"></div>
    </div>
    <div>
      <div style="display: inline-block; width: 1in; height: 1in">Should be on second page</div>
    </div>

(with and without the leading space).  Let me know if that's now what you meant?

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/2719d97b6766

Comment 15

[Boris Zbarsky [:bzbarsky]]

Backed out on suspicion of causing orange.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Specifically, a crash on layout/reftests/bugs/421710-1.html

Comment 17

[Boris Zbarsky [:bzbarsky]]

What happens in that testcase is that mLines.front() == aLine but aState.mY == 600.

When this happens, mContent is the <div id="spBreadCrumbLine"> in the testcase.  And this has 10px top padding.

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK, so the thing is that if mY > 0 then that's presumably due to top padding, but a continuation will have 0 top padding.  So pushing can make sense there.  I think we should probably not push the first line no matter what, though.

That said, when splitting across _pages_, our continuation could in fact end up taller than us even if there's no padding anything going on: we could be starting halfway down the page.  roc, is there a sane way to detect that situation?

Comment 19

[Boris Zbarsky [:bzbarsky]]

That could happen with columns too, actually.  Just start partway down a column.

I suppose I could check that the offsets of all the frames between |this| and whatever the "root" of the splitting (pagecontent's child canvas in the printing case; the block inside the columnset in the columns case) is are 0.  But that would involve having a good way to tell when to stop.  :(

Comment 20

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

(In reply to comment #18)
> OK, so the thing is that if mY > 0 then that's presumably due to top padding,
> but a continuation will have 0 top padding.  So pushing can make sense there. 
> I think we should probably not push the first line no matter what, though.
> 
> That said, when splitting across _pages_, our continuation could in fact end up
> taller than us even if there's no padding anything going on: we could be
> starting halfway down the page.  roc, is there a sane way to detect that
> situation?

What exactly are you worried about happening in that situation?

Can you just check mY > the block's used top padding?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> What exactly are you worried about happening in that situation?

In that situation, even if we're at mY <= block's top padding and taller than the block, it might make sense to push, since the next continuation is taller than this block and we might fit in it.

Or in other words, the mY check is not really valid for the first continuation unless its top padding is 0 _and_ it's at the very beginning of its page or column.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm going to get in touch with the Google folks and see about options 4 and 5 from comment 7.

Comment 23

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

(In reply to comment #21)
> In that situation, even if we're at mY <= block's top padding and taller than
> the block, it might make sense to push, since the next continuation is taller
> than this block and we might fit in it.
> 
> Or in other words, the mY check is not really valid for the first continuation
> unless its top padding is 0 _and_ it's at the very beginning of its page or
> column.

Ah, right. Well, we should just check nsHTMLReflowState::mIsAtTopOfPage then.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Hrm.  That seems to be true even if our parent has top padding (in which case our first continuation is shorter than following continuations would be)....

Comment 25

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

Then we should clear it when we construct a reflow state for frames that have top border/padding.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I could do that, but right now we actually carefully preserve it in those situations.... and I'm not sure what else depends on this flag.  How safe is clearing it?  Should I just introduce another flag that's just like that one, but cleared on top margin/border/padding?

Comment 27

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

It's used in the same way everywhere AFAIK: to force content to be placed even where it doesn't fit, instead of pushed.

My main concern is that I think up till now we've had a deliberate policy of placing at least one child in every block continuation. Changing that is probably a good idea, for blocks that have top border/padding on the first continuation only, but it might possibly break stuff. Or not.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Safe enough for 2.0?

Comment 29

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

I don't know.

Comment 30

[Boris Zbarsky [:bzbarsky]]

The changes in bug 626395 fixed this without the need for blockframe reflow surgery.  Happy days!

I've landed the tests as http://hg.mozilla.org/mozilla-central/rev/2221636653f7

Comment 31

[Aaron Train [:aaronmt]]

Verified fixed testing on 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110203
Firefox/4.0b12pre

20110204030345
http://hg.mozilla.org/mozilla-central/rev/847a825087f2

Comment 32

[[:Aleksej]]

WFM with 4.0b11b3 Mozilla/5.0 (X11; Linux i686; rv:2.0b11) Gecko/20100101 Firefox/4.0b11