179683 - Fixed positioned elements don't print and can crash

Status: RESOLVED FIXED

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

Description

[Saulius Gurklys]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021111
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021111

The page at http://www.vtex.lt/sagu/mozilla/pp.html is displayes with any problem,
but print preview generates blank page.Also trying to print preview for the
second time browser crashes.

Reproducible: Always

Steps to Reproduce:
1.Start mozilla
2.Goto to http://www.vtex.lt/sagu/mozilla/pp.html
3.File->Print Preview (blank page)
4.Close print preview
5.File->Print Preview (browser crashes)

Comment 1

[Torben]

Confirming. Talkback ID TB13828454H

Comment 2

[Wesha]

confirmed 2002111108/NT4... GKLAYOUT.dll, as usual.

Comment 3

[rods (gone)]

over chris

Comment 4

[Torben]

Also happens on macs, TB13871256X; HW/OS->All/All

Comment 5

[karnaze (gone)]

Attachment: reduced test case without a table

Comment 6

[karnaze (gone)]

Making the summary more general.

Comment 7

[karnaze (gone)]

Attachment: simple test with 2 pages

Comment 8

[karnaze (gone)]

Attachment: patch to fix the bug

The patch adds an nsAbsoluteContainingBlock delegate to ViewportFrame
(similarly to nsBlockFrame and nsPositionedInlineFrame), makes
nsAbsoluteContainingBlock handle fixed positioning, makes nsPageContentFrame a
subclass of ViewportFrame, replicates fixed positioned frames from
nsPageContentFrame's prev-in-flow.

Comment 9

[karnaze (gone)]

Changing the component since the patch affects mostly positioning code, adding
others to CC list.

Comment 10

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

What behavior is this implementing?  My memory is that fixed positioned elements
should print on every page and should never be split.  However, you should check
the spec.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That patch seems (at quick skim) to make them show on every page.

I would like to read the patch in detail, though, because there were some
discrepancies between fixed and absolute reflow that were intentional (eg the
reflow state hacks for scrollbars); since we've rolled them both into
nsAbsoluteContainingBlock, I want to make sure that things still work right...

Comment 12

[karnaze (gone)]

Yes, they show on every page, aren't allowed to split, and the viewport
scrollbar hacks are still there.

Comment 13

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

This is excellent. It's really great to have the viewport use
nsAbsoluteContainingBlock. That simplifies things a lot.

+  nsIAtom* GetChildListName() const { return mChildListName; }
...
+  nsIAtom*    mChildListName;

Instead of using this extra field, we could make GetChildListName() virtual and
default to returning nsLayoutAtoms::absoluteList. The viewport would subclass
nsAbsoluteContainingBlock (say to "nsFixedContainingBlock") and override
GetChildListName().

-  if (aListName == nsLayoutAtoms::absoluteList) {
+  if (mAbsoluteContainer.GetChildListName() == aListName) {

Why are you doing this here and below? This can only be
nsLayoutAtoms::absoluteList. I don't think this abstraction buys us anything
significant. If you feel like leaving it in, though, that's OK; it doesn't hurt
either.

+#include "nsAbsoluteContainingBlock.h"

You don't need this because nsViewportFrame.h includes nsAbsoluteContainingBlock.h.

+  if (mFixedContainer.GetChildListName() == aListName) {
Like in nsBlockFrame and nsInlineFrame, I don't think this abstraction is needed
and we could just use nsLayoutAtoms::fixedList without any problems now or later.

+  PRBool isHandled = PR_FALSE;
It looks like this local variable is never used. You can just get rid of it.

+    // XXX Find a cleaner way to do this...
I think the way you're doing this is fine.

+    PRBool wasHandled; // XXX can this ever be false?
Sure. It will be set if and only if the incremental reflow was completely
handled by incrementally reflowing the fixed frames. If the incremental reflow
targets anything under the normal child of the viewport, then this will be
false. You could use this to do a little optimization --- if it returns PR_TRUE,
you can skip some of the stuff below --- but it's not really worth it here.

+  if (NS_FAILED(rv))
+    return NS_ERROR_NULL_POINTER;
Why not just return rv?

+    if (firstFixed) {
You might as well move this up above the creation of "nsFrameConstructorState
state".

Is it possible for script in a document to make DOM calls to add new fixed
elements after frame construction for the first page but before reflow for
printing? I think so, because inline script could call functions (such as
getComputedStyle()) that force flushing of reflows before the whole document is
loaded. Or does this not happen in printing for some reason?

The refactoring of page construction warms my heart :-).

Comment 14

[karnaze (gone)]

Attachment: revised patch with roc's comments

I made all of the changes except those below.

>>-  if (aListName == nsLayoutAtoms::absoluteList) {
>>+  if (mAbsoluteContainer.GetChildListName() == aListName) {

>>Why are you doing this here and below? This can only be
>>nsLayoutAtoms::absoluteList. I don't think this abstraction buys us anything
>>significant. If you feel like leaving it in, though, that's OK; it doesn't
hurt
>>either.

I would prefer to leave this abstration in. There are places where it is needed
and places where it isn't, but it is just simpler to always use it.

>>+    if (firstFixed) {
>>You might as well move this up above the creation of "nsFrameConstructorState

>>state".

It's on the line before it is first used.

>>Is it possible for script in a document to make DOM calls to add new fixed
>>elements after frame construction for the first page but before reflow for
>>printing? I think so, because inline script could call functions (such as
>>getComputedStyle()) that force flushing of reflows before the whole document
is
>>loaded. Or does this not happen in printing for some reason?

There are no incremental reflows in printing. The whole docuement must be
loaded.

Comment 15

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

Comment on attachment 107082 [details] [diff] [review]
revised patch with roc's comments

nice. sr=roc+moz

Comment 16

[José Jeria]

This will fix bug 135653, right? Same bug.

Comment 17

[karnaze (gone)]

*** Bug 135653 has been marked as a duplicate of this bug. ***

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 107082 [details] [diff] [review]
revised patch with roc's comments

In nsViewportFrame.cpp:

> +    NS_ASSERTION(PR_FALSE, "unexpected child list");

Use NS_ERROR. This happens in a few places in that file.

> +    mFixedContainer.SetInitialChildList(this, aPresContext, aListName, aChildList);

Any good reason not to do rv = .... here?  Or even "return ....;"?

> -  if (aIndex < 0) {
> -    return NS_ERROR_INVALID_ARG;

Maybe NS_ASSERTION(aIndex >= 0, "illegal index"); here?

nsViewportFrame.h:

> +  * ViewportFrame is the parent of the doc element's frame which is 
> +  * either ...

I find this comment confusing... maybe replace "which is either" by "this
latter is either" (it's not clear what the antecedent of "this" is).

> +  * ViewportFrame is also the base class for nsPageContentFrame.

Does that comment really need to live in the superclass?  That seems a little
odd (superclasses should not really know about classes derived from them....)

nsCSSFrameConstructor.cpp:

> +    // XXX should this set for every new page (in ConstructPageFrame)?

"should this be set"

> -        rv = state.mFixedItems.containingBlock->AppendFrames(aPresContext, *shell,
> +        state.mFixedItems.containingBlock->AppendFrames(aPresContext, *shell,

You have this change in two places. Why do we not care about the return value? 
Worth commenting perhaps?

Would it make sense to pass the style context to ConstructPageFrame?  That way
the CreateContinuingFrame code won't have to do the extra style context
resolution it's ending up with in the current code (just move the style context
resolution for the pageframe over into ConstructRootFrame).

This looks really good, Chris!

Don't worry about the wasHandled optimization roc mentioned; it can't actually
be made because we need to reflow the primary child before the fixed frames (I
have a bug on that).

Comment 19

[Carine Held]

Reproduced on 1/08/03 Trunk, Win XP. Testcase keyword added.

Comment 20

[karnaze (gone)]

Attachment: revised patch with Boris' comments

>> -	    rv = state.mFixedItems.containingBlock->AppendFrames(aPresContext,
*shell,
>> +	    state.mFixedItems.containingBlock->AppendFrames(aPresContext,
*shell,

>You have this change in two places. Why do we not care about the return value?

>Worth commenting perhaps?

I changed all occurences of this except one (where rv wasn't declared and no
other callers of AppendFrames used a return value).

>Would it make sense to pass the style context to ConstructPageFrame?  That way

>the CreateContinuingFrame code won't have to do the extra style context
>resolution it's ending up with in the current code (just move the style
context
>resolution for the pageframe over into ConstructRootFrame).

I don't think this is much of an optimization, considering the large overhead
of creating a page.

Comment 21

[karnaze (gone)]

Comment on attachment 112010 [details] [diff] [review]
revised patch with Boris' comments

carrying over roc's sr.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> +    NS_ERROR(PR_FALSE, "unexpected child list");

NS_ERROR("unexpected child list");

NS_ERROR is not conditional; that's why I said to use it instead of
NS_ASSERTION(PR_FALSE, whatever);  I'm surprised that this built...

> +  return rv; // XXX account for NS_FAILED(rv) above

Why is this worthy of an "XXX" comment?  What's wrong with that code exactly? 
("XXX" denotes things that should really be fixed, no?)

My comment about the style context resolution was not so much about the perf
aspect as about the readability/codesize aspect... but I'm ok with leaving it
as-is if you would prefer.

Comment 23

[karnaze (gone)]

Attachment: revised patch

>Why is this worthy of an "XXX" comment?  What's wrong with that code exactly? 

>("XXX" denotes things that should really be fixed, no?)

Just an innocent observation that the code above it (which the patch doesn't
change) never checks for a failed rv, and if it finds one, how much subsequent
code should be executed.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 112037 [details] [diff] [review]
revised patch

looks good.  ;)

Comment 25

[karnaze (gone)]

Attachment: revised patch (pick up missing part of original patch)

I managed to lose the following piece of the original patch. This patch puts it
back in. 


   } else if (nsLayoutAtoms::pageFrame == frameType) {
-    rv = NS_NewPageFrame(aPresShell, &newFrame);
-    if (NS_SUCCEEDED(rv)) {
-      newFrame->Init(aPresContext, content, aParentFrame, styleContext,
aFrame);
-      nsHTMLContainerFrame::CreateViewForFrame(aPresContext, newFrame,
-						styleContext, nsnull, PR_TRUE);

-      nsIFrame* pageContentFrame = nsnull;
-      NS_NewPageContentFrame(aPresShell, &pageContentFrame);
-
-      nsCOMPtr<nsIStyleContext> pageContentPseudoStyle;
-      aPresContext->ResolvePseudoStyleContextFor(nsnull,
nsLayoutAtoms::pageContentPseudo,
-						  styleContext,
-						 
getter_AddRefs(pageContentPseudoStyle));
-
-      pageContentFrame->Init(aPresContext, nsnull, newFrame,
pageContentPseudoStyle, nsnull);
-      nsHTMLContainerFrame::CreateViewForFrame(aPresContext, pageContentFrame,

-						pageContentPseudoStyle, nsnull,
PR_TRUE);
-
-     // Set the initial child lists
-      newFrame->SetInitialChildList(aPresContext, nsnull, pageContentFrame);
-
-    }
-
+    nsIFrame* pageContentFrame;
+    rv = ConstructPageFrame(aPresShell, aPresContext, aParentFrame, aFrame, 
+			     newFrame, pageContentFrame);

Comment 26

[karnaze (gone)]

fixed on trunk.