Closed Bug 78070 Opened 20 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [blocking2.0?])

Attachments

(1 file, 2 obsolete files)

From bug 55334, "No scrollbars for javascript-generated content".  jst is 
currently working around a scrollbar problem for the case of javascript-
generated content.  evaughan@netscape.com 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.
Target Milestone: --- → mozilla0.9.2
->moz1.0
Target Milestone: mozilla0.9.2 → mozilla1.0
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)
Target Milestone: mozilla1.0 → mozilla1.0.1
Added the attachment from bug 140060 to the URL field.  It is an ideal testcase 
for this bug.

Jake
Taking, since Eric is not here anymore.

This problem (and its attendant workarounds) really yuckifies our code.
Assignee: eric → jkeiser
Status: NEW → ASSIGNED
Blocks: 225868
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).
Blocks: 369950
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...)
OS: Windows 98 → All
Hardware: PC → All
Whiteboard: [blocking2.0?]
Target Milestone: mozilla1.0.1 → ---
Assignee: john → nobody
Status: ASSIGNED → NEW
QA Contact: chrispetersen → layout
Blocks: 325352
No longer blocks: 325352
Needed for HTML5-compliant document.open().
Blocks: 325352
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.
Blocks: 467935
Blocks: 481806
Attached patch Proposed fix (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #370927 - Flags: superreview?(roc)
Attachment #370927 - Flags: review?(roc)
Attachment #370927 - Flags: review?(dbaron)
Blocks: 486741
Could you describe the approach you took to fixing this?
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.
+  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?
> 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.
Attachment #370927 - Attachment is obsolete: true
Attachment #370928 - Attachment is obsolete: true
Attachment #370927 - Flags: superreview?(roc)
Attachment #370927 - Flags: review?(roc)
Attachment #370927 - Flags: review?(dbaron)
Attachment #371372 - Flags: superreview?(roc)
Attachment #371372 - Flags: review?(roc)
Attachment #371372 - Flags: review?(dbaron)
Attachment #371372 - Flags: superreview?(roc)
Attachment #371372 - Flags: superreview+
Attachment #371372 - Flags: review?(roc)
Attachment #371372 - Flags: review+
Attachment #371372 - Flags: review?(dbaron) → review+
Comment on attachment 371372 [details] [diff] [review]
Same thing with the comments addressed

r=dbaron.  Sorry for the long delay on this.
Pushed http://hg.mozilla.org/mozilla-central/rev/b43f2b9e0c32
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 502447
Blocks: 378665
Depends on: 614909
Blocks: 300540
Blocks: 370812
You need to log in before you can comment on or make changes to this bug.