376662 - Convert nsIFrame::GetOffsetTo to not explicitly use views

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 365294 comment 12.

Note that this bug depends on bug 366689 getting fixed first.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to do it when it can be done

Comment 2

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

Attachment: fix

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 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

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

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

Comment 5

[Keith Schwarz [:kschwarz]]

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.

Comment 6

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

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.

Comment 7

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

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.

Comment 8

[Keith Schwarz [:kschwarz]]

Attachment: Testcase for SVG

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.

Comment 9

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

Attachment: fix v2

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.

Comment 10

[Martijn Wargers (dead)]

Perhaps that patch would fix bug 444988 too?

Comment 11

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

I doubt it.

Comment 12

[Keith Schwarz [:kschwarz]]

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?

Comment 13

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

Attachment: fix v3

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

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

Attachment: fix v4

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

Comment 16

[Ian Neal]

Looks like this has caused bustage on SeaMonkey :(

Comment 17

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

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

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

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, ick.  OK.

Comment 21

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

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.

Comment 22

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

Backed out to fix regression bug 453661.

Comment 23

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

Attachment: fix v5

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.

Comment 24

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

I'll reland this at some point.

Comment 25

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

Pushed f7414f6cb95b.