129034 - IFrames display on the wrong page in PP

Status: VERIFIED FIXED

(Core :: Print Preview, defect, P1)

Description

[rods (gone)]

Comment 1

[rods (gone)]

If you have enough lines in a document to push the IFrame on to the second page
(for PP) it will be shown on the first page of the PP. This is the same for
object tags.

Comment 2

[Kevin McCluskey (gone)]

Bulk moving all nsbeta1 nominations to future-P1. If they are approved
(nsbeta1+) they will be moved to mozilla1.0

Comment 3

[Kevin McCluskey (gone)]

nsbeta1-

Comment 4

[rods (gone)]

Fails on www.news.com

Comment 5

[Kevin McCluskey (gone)]

nsbeta1+

Comment 6

[rods (gone)]

Note that the testcase is part of the build, located at:
file:///S:/mozilla/layout/html/tests/printer/iframe_2nd_page/index.html

Comment 7

[rods (gone)]

One interesting thing I noticed by setting some breakpoints in the
CSSFrameConstructor is that the 1st PageFrame gets created and then the IFrame
frame get created as part of the block reflow and then the 2nd PageFrame frame
is created. 

Then the IFrame gets moved to the second page and it's parenting is messed up.

Comment 8

[karnaze (gone)]

Attachment: 2nd IFRAME for test case

Comment 9

[karnaze (gone)]

Attachment: 1st IFRAME for test case

Comment 10

[karnaze (gone)]

Attachment: test case using attachment #2 (which uses attachment #1)

Comment 11

[karnaze (gone)]

Attachment: kmcclusk's wip, win32 specific, proof of concept patch

Comment 12

[karnaze (gone)]

->kmcclusk

Comment 13

[Kevin McCluskey (gone)]

Attachment: Fix which includes nsIWidget::SetParent implementation for WIN32

Fix which reparents the widgets owned by the view or its descendants when the
view is inserted into its parents list of children. Solves the problem on
WIN32. Mac and Linux need to have nsIWidget::SetParent(nsIWidget *aNewParent)
implemented.

Comment 14

[Kevin McCluskey (gone)]

Attachment: Same as last patch minus the small part of an unrelated patch.

Comment 15

[Kevin McCluskey (gone)]

Note: As expected, browsing webpages and running jrgm's pageloader tests it
*never* triggers the logic which calls GetWidgetForView or ReparentChildWidgets.
 It only gets executed when print previewing pages so there will not be any page
load performance impact from this fix.

SetParent needs to be implemented on Mac and Linux. I'll file separate bugs for
this work. If the current patch is checked in SetParent will be a no-op on those
platforms so this bug will still occur until we implement nsIWidget::SetParent

Comment 16

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

Shouldn't SetParent be setting the mParent field of the widget?

Comment 17

[Kevin McCluskey (gone)]

Some platforms use the basewidget's mParent member and others don't. WIN32
doesn't use it at all. I didn't set the mParent field because I didn't want to
regress any behavior on the non-WIN32 platforms which may rely on mParent. I was
assuming that when the nsIWidget::SetParent implementation is done for a
platform that actually uses mParent it will become clear what the proper
implementation for the basewidget's ::SetParent should be. For now, I think the
safest behavior is to report that it's not implemented.

I would like to check in this patch which includes the xp-code and WIN32
specific nsIWidget::SetParent code, and have platform experts implement the
nsIWidget::SetParent logic for Mac, Linux and other platforms.  

Comment 18

[kinmoz]

Comment on attachment 98215 [details] [diff] [review]
Same as last patch minus the small part of an unrelated patch.

r/sr=[e-mail]

Just add a comment to nsWindow::SetParent() that explains why you aren't
setting mParent for win32.

Comment 19

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

Comment on attachment 98215 [details] [diff] [review]
Same as last patch minus the small part of an unrelated patch.

r=roc+moz

This is rather horrid, really, but I can't think of a better way to do solve
the problem.

Comment 20

[Kevin McCluskey (gone)]

Fix checked in to the trunk

Comment 21

[Kevin McCluskey (gone)]

bug 168116 covers the GTK nsIWidget::SetParent implementation
bug 168117 covers the Mac nsIWidget::SetParent implementation

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 98215 [details] [diff] [review]
Same as last patch minus the small part of an unrelated patch.

a=[e-mail] for 1.0 branch.  Please mark on the Linux and Mac sub-bugs
that we'll want those patches for 1.0 when they're done.

WHen checked in, change mozilla1.0.2+ to fixed1.0.2

Comment 23

[Jaime Rodriguez, Jr.]

This is effecting scrolling performance on a top site.

Changing edt1.0.2 nomination to adt1.0.2, as the ADT is monitoring the 1.0.2
checkins. 

Comment 24

[Jaime Rodriguez, Jr.]

Pls excuse my last comment, "This is effecting scrolling performance on a top
site." It was a copy and paste error brought on b y big thumbs on a sunday
afternoon. ;-)

sujay: can you pls verify this as fixed on the trunk? thanks!

Comment 25

[Daniel Veditz [:dveditz]]

adt-, not verified on the trunk. Please get this verified before you renominate
and then make the case why this fits the Blackbird goals.

Comment 26

[sujay]

verified in 9/26 trunk build

news.com seems to render properly in PP mode. I compared actual
print output with PP.

Comment 27

[Jaime Rodriguez, Jr.]

Removing the minus to renominate this one, as it effects a top100 site.

Comment 28

[Michael Buckland]

please check in to branch and add keyword fixed1.0.2
if new driver approval is needed please make sure to get it.

Comment 29

[rods (gone)]

Attachment: patch for 1.0.2 branch

ported over....

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 104897 [details] [diff] [review]
patch for 1.0.2 branch

sr=dveditz based on it being the same as the reviewed trunk patch

Comment 31

[Michael Buckland]

rod:  please get r= and drivers approval today as we are driving toward a
release candidate

Comment 32

[kinmoz]

Comment on attachment 104897 [details] [diff] [review]
patch for 1.0.2 branch

r=[e-mail]

Comment 33

[chris hofmann]

Comment on attachment 104897 [details] [diff] [review]
patch for 1.0.2 branch

a=chofmann for 1.0.2

Comment 34

[rods (gone)]

fixed on branch

Comment 35

[Michael Buckland]

Just talked to Rod.  This has landed on the branch.  Linking  to our meta.

Comment 36

[sujay]

verified in 11/6 branch build.