78070 - scrollbar code should be able to handle document root element being replaced

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

From bug 55334, "No scrollbars for javascript-generated content".  jst is 
currently working around a scrollbar problem for the case of javascript-
generated content.  [e-mail] has a stack trace (2000-10-13 13:26 
comment in bug 55334) that shows the problem in the scrollbar code.

jst said:

The scrollbar code needs to be able to cope with the root element changing in a 
document, and currently that involves setting the document in the old root to 
null. 

jst also said:

------- Additional Comments From Johnny Stenback 2001-04-12 03:03 -------

Ok, workaround checked in, reassigning to evaughan in hope of getting a real fix
for this problem in the scrollbar code.

Eric, the scrollbar code must be able to function properly even if the root
element in a document is removed and re-inserted, or simply just replaced. This
can happen in the case where a script writer does document.open()... (see
attached testcase) or if someone replaces or removes the root element in a
document from script, both cases are valid cases and the scrollbar code simply
needs to cope with this. To test this either back out my workaround and run the
attached testcase or shout and I'll create a testcase that replaces the root in
a document.

Comment 1

[Peter Trudelle]

->moz1.0

Comment 2

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 3

[Jacob Kjome]

Added the attachment from bug 140060 to the URL field.  It is an ideal testcase 
for this bug.

Jake

Comment 4

[John Keiser (jkeiser)]

Taking, since Eric is not here anymore.

This problem (and its attendant workarounds) really yuckifies our code.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So there are several things going on here, depending on what one does with the document element.

If one changes the document element, an obvious problem is that the root scrollframe has the root content node as its mContent and we do NOT reconstruct it when we move around the root content node.  So its mContent has nothing to do with the document.  I can get scrollbars at least on the testcases from bug 205725 to work by unbinding and rebinding the scrollbar mContents to the new root, but they display wrong....

The testcases in bug 205725 use the same document element; not sure why they still break.

But really, given that the scrollframe points to the document element, I think we should blow away said scrollframe when the document element changes (and change nsViewportFrame to handle dynamic changes to its principal child list).

Comment 6

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

I agree.

Comment 7

[Gábor Stefanik]

Confirmed to exist on Mac. Mozilla 1.0.1 is gone long ago, removing. Nominating to block Mozilla 2 (we really need a blocking2.0 flag...)

Comment 8

[Henri Sivonen (:hsivonen)]

Needed for HTML5-compliant document.open().

Comment 9

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

This shouldn't be too hard to fix, but we need to make sure we handle switching between XUL, non-XUL, and framesets. It might be easiest to get rid of ReconstructDocElementHierarchyInternal and just completely destroy the frame tree and then rebuild it by calling ConstructRootFrame again.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w (easier to review the presshell changes here, maybe)

Comment 12

[David Baron :dbaron:]

Could you describe the approach you took to fixing this?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Sure.  The basic idea is that the only frame we keep around no matter what is the viewport.  The rest of the frames around the root (root scrollframe, root element box frame, root block, root svg, root table, canvas, page sequence, etc) we remove and recreate as needed when we get ContentRemoved()/ContentInserted() calls on the root element.

The viewport frame continues to have a null mContent (and we might just need to keep that).

In particular, this means that removing the root element removes all frames in the frame tree except the viewport.  If a new root, or the same one, is then inserted, we'll create new scrollbars hooked up to the right content node, as needed.  That's what fixes this bug.

So the patch basically has three parts:

1)  Move construction of everything but the viewport from ConstructRootFrame to
    ConstructDocElementFrame.
2)  Make ContentRemoved on the root remove all the frames under the viewport,
    and clean up ContentInserted a bit.
3)  Remove ReinsertContent and our existing
    ReconstructDocElementHierarchyInternal code; make all frame reconstruction
    go through RecreateFramesForContent.

Comment 14

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

Yippee!

Comment 15

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

+  nsresult ConstructRootFrame(nsIFrame**      aNewFrame);
+nsCSSFrameConstructor::ConstructRootFrame(nsIFrame**      aNewFrame)

Fix param indent

+      // Make sure to clean up the work that SetUpDocElementContainingBlock() might have done.
+      nsFrameManager* frameManager = mPresShell->FrameManager();
+      nsIFrame* viewport = frameManager->GetRootFrame();
+      nsIFrame* viewportChild = viewport->GetFirstChild(nsnull);

Do we really need to do all this here? Could we just arrange for ContentRemoved to be a bit better behaved if it runs when we have no document element frame but the canvas and scrollbars were set up?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing with the comments addressed

> Could we just arrange for ContentRemoved to be a bit better behaved

Yes, but it's a bit tricky; see the nice long comment in ContentRemoved.  Good thing we have reftests and crashtests (esp. the latter) that trigger this code in all sorts of ... interesting ... ways.

Comment 17

[David Baron :dbaron:]

Comment on attachment 371372 [details] [diff] [review]
Same thing with the comments addressed

r=dbaron.  Sorry for the long delay on this.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b43f2b9e0c32