Closed Bug 376662 Opened 13 years ago Closed 12 years ago

Convert nsIFrame::GetOffsetTo to not explicitly use views

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: roc)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

See bug 365294 comment 12.

Note that this bug depends on bug 366689 getting fixed first.
Attached patch fix (obsolete) — Splinter Review
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).
Assignee: nobody → roc
Attachment #260777 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #334982 - Flags: superreview?(bzbarsky)
Attachment #334982 - Flags: review?(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.
Attachment #334982 - Flags: superreview?(bzbarsky)
Attachment #334982 - Flags: superreview+
Attachment #334982 - Flags: review?(bzbarsky)
Attachment #334982 - Flags: review+
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.
Attached image 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.
Attached patch fix v2 (obsolete) — Splinter Review
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.
Attachment #334982 - Attachment is obsolete: true
Attachment #335453 - Flags: superreview?(bzbarsky)
Attachment #335453 - Flags: review?(bzbarsky)
Attachment #335453 - Attachment is patch: true
Attachment #335453 - Attachment mime type: application/text → text/plain
Perhaps that patch would fix bug 444988 too?
Attachment #335453 - Flags: superreview?(bzbarsky)
Attachment #335453 - Flags: superreview+
Attachment #335453 - Flags: review?(bzbarsky)
Attachment #335453 - Flags: review+
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?
Attached patch fix v3 (obsolete) — Splinter Review
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.
Attachment #335453 - Attachment is obsolete: true
Attachment #335840 - Flags: superreview?(bzbarsky)
Attachment #335840 - Flags: review?(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.
Attached patch fix v4 (obsolete) — Splinter Review
OOps. I'm not sure how those changes got lost.
Attachment #335840 - Attachment is obsolete: true
Attachment #335971 - Flags: superreview?(bzbarsky)
Attachment #335971 - Flags: review?(bzbarsky)
Attachment #335840 - Flags: superreview?(bzbarsky)
Attachment #335840 - Flags: review?(bzbarsky)
Attachment #335971 - Flags: superreview?(bzbarsky)
Attachment #335971 - Flags: superreview+
Attachment #335971 - Flags: review?(bzbarsky)
Attachment #335971 - Flags: review+
Looks like this has caused bustage on SeaMonkey :(
Checked in 1bdd5da49865. I also checked in a bustage fix for Seamonkey.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Depends on: 453661
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix v5Splinter Review
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.
Attachment #335971 - Attachment is obsolete: true
I'll reland this at some point.
Pushed f7414f6cb95b.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 453859
You need to log in before you can comment on or make changes to this bug.