Closed Bug 78070 Opened 20 years ago Closed 12 years ago
scrollbar code should be able to handle document root element being replaced
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
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).
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
Target Milestone: mozilla1.0.1 → ---
Assignee: john → nobody
Status: ASSIGNED → NEW
QA Contact: chrispetersen → layout
Needed for HTML5-compliant document.open().
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.
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: 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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.