See bug 365294 comment 12. Note that this bug depends on bug 366689 getting fixed first.

This updates Boris's patch, fixes a bug with scrolling where we need to update the frame position as soon as the view position is changed (and *before* we repaint anything due to the scroll), and fixes the popup frame positioning. It also removes the evil GetOriginToViewOffset which we don't need anymore because we've removed the only caller (and because it no longer makes sense).

Comment on attachment 334982 [details] [diff] [review] fix >+++ b/layout/generic/nsFrame.cpp >+ aOther = aOther->GetParent(); That should be GetCrossDocParentFrame. >-NS_IMETHODIMP nsFrame::GetOriginToViewOffset(nsPoint& Sweet! So nice to see this die! >+++ b/view/src/nsScrollPortView.cpp >+ if (NS_SUCCEEDED(mListeners->QueryElementAt(i, kScrollPositionListenerIID, (void**)&listener))) { Ugh. If we weren't planning to remove this code, I'd ask that we switch to using an nsCOMPtr and do_QueryElementAt, or better yet nsCOMArray. But we plan to remove the code, right? r+sr=bzbarsky with the GetParent fix.

Yes, nsScrollPortView should die ASAP. moving scrolling to layout is probably the first step of compositor.

With the patch applied on my system (and with Boris's change to use GetCrossDocParentFrame), IFRAMEs with scrollbars do not display properly. The inner content draws on top of the frame border. You can best see this by making an IFRAME with a very large (20px) border.

This would be because there's an offset from the nsSubdocumentFrame and the nsVewportFrame it contains, but nsViewportFrame always has position 0,0. I'll have to think about the best way to fix that.

The best thing might be to have the nsSubdocumentFrame contain an anonymous frame, which is the actual parent of the viewport frame, like it has an anonymous inner view right now.

It seems like click detection inside SVG foreignObjects is a bit off with the patch applied. The testcase attached has an SVG foreignObject with scrolling. The links up at the top seem to be working, but scrolling the object downward causes the graphics to respond to mouseOver events, but not click events.

Updated. This fixes the subdocument positioning issue. With this patch, I don't see any problems in your foreignObject testcase. It's using an iframe so maybe that was related? I dunno. Let me know if you still see a problem with foreignObject witht his patch.

Perhaps that patch would fix bug 444988 too?

I doubt it.

The foreignObject test case works wonderfully using patch v2. However, on my Linux system, patch v2 is failing reftest "/tests/dom/tests/mochitest/general/test_offsets.xul". The errors all pertain to positions being slightly mismatched after vertical and horizontal scrolls. I don't know much about XUL, but perhaps something in the XUL directory (maybe nsScrollFrame) needs to implement ViewPositionDidChange?

Okay ... the reason that test failed is that in the initial reflow of the XUL scrollbox, the scrolled XUL frame was positioned at (0,0) even though the XUL scrollbox has a 4px border --- the scrolled frame should then be positioned at (4,4) pixels. And the reason for that is that although nsXULScrollFrame::LayoutScrollArea computed the correct childOffset, the call to SetBounds declined to set the frame position because it saw flags & NS_FRAME_NO_MOVE_FRAME was nonzero. And the *that* is because NS_FRAME_NO_MOVE_FRAME is actually 0x0002 | NS_FRAME_NO_MOVE_VIEW, and we've set NS_FRAME_NO_MOVE_VIEW in LayoutScrollArea! So I've changed nsBox::SetBounds to check (flags & NS_FRAME_NO_MOVE_FRAME) == NS_FRAME_NO_MOVE_FRAME, which I think is what was intended. I think there's a similar problem here: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp#767 So I've changed that to match. That change is a little scary perhaps, but as far as I can tell the only place we pass NS_FRAME_NO_MOVE_VIEW (which is the only way to change behaviour here) into ReflowChild is nsComboboxControlFrame::ReflowDropdown, which happens to be passing in the frame's existing coordinates in aX/aY anyway, so I just added NS_FRAME_NO_MOVE_FRAME to the flags there for sanity. It seems all the other uses of NS_FRAME_NO_MOVE_VIEW are to pass into nsContainerFrame::SyncFrameView*, so they won't be affected. I also fixed an assertion caused by nsGenericElement::GetOffsetRect, which shouldn't have been passing in a null aRelativeTo for GetOffsetTo. There are no other changes from the previous iteration.

roc, that diff seems to be missing the nsContainerFrame and nsComboboxControlFrame chunks. Did you mean to include them? It's kinda weird that NO_MOVE_FRAME is actually two bits, but let's not worry about that for now, I guess.

OOps. I'm not sure how those changes got lost.

Looks like this has caused bustage on SeaMonkey :(

Checked in 1bdd5da49865. I also checked in a bustage fix for Seamonkey.

roc, I'm not seeing a SeaMonkey checkin... I just wanted to take a look at the bustage fix that was needed if you happen to have a link to a diff.

I had to check in to CVS because that's where extensions/typeaheadfind currently lives :-(

Oh, ick. OK.

We should back this out because of the regression. Unfortunately I can't do it myself today. We should only need to back out the trunk hg change --- the typeahead find fix that landed in CVS should be harmless to leave there.

Backed out to fix regression bug 453661.

Sigh. This change in nsContainerFrame::ReflowChild inverted the sense of the test. Before it was testing "if allowed to move the frame", after it's testing "if NOT allowed to move the frame". I'm amazed anything worked at all. - if (0 == (aFlags & NS_FRAME_NO_MOVE_FRAME)) { + if (NS_FRAME_NO_MOVE_FRAME == (aFlags & NS_FRAME_NO_MOVE_FRAME)) { Oops. This patch fixes it. Well, the reason stuff worked is that nsHTMLScrollFrame sets NS_FRAME_NO_MOVE_FRAME in ReflowScrolledArea, which was setting the scrolled frame to (0,0), but then after reflowing the scrolled area we'd get around to placing it and that would set its position to the right thing. So all that was affected is invalidates issued during the reflow of the scrolled area. Also, in general setting the wrong coordinates in ReflowChild would have been corrected by settting the right coordinates (again) in FinishReflowhild.

I'll reland this at some point.

Pushed f7414f6cb95b.