Closed
Bug 376662
Opened 18 years ago
Closed 16 years ago
Convert nsIFrame::GetOffsetTo to not explicitly use views
Categories
(Core :: Layout, defect)
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)
394 bytes,
image/svg+xml
|
Details | |
24.13 KB,
patch
|
Details | Diff | Splinter Review |
See bug 365294 comment 12.
Note that this bug depends on bug 366689 getting fixed first.
Reporter | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•16 years ago
|
||
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)
Reporter | ||
Comment 3•16 years ago
|
||
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+
Assignee | ||
Comment 4•16 years ago
|
||
Yes, nsScrollPortView should die ASAP. moving scrolling to layout is probably the first step of compositor.
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 9•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #335453 -
Attachment is patch: true
Attachment #335453 -
Attachment mime type: application/text → text/plain
Comment 10•16 years ago
|
||
Perhaps that patch would fix bug 444988 too?
Assignee | ||
Comment 11•16 years ago
|
||
I doubt it.
Reporter | ||
Updated•16 years ago
|
Attachment #335453 -
Flags: superreview?(bzbarsky)
Attachment #335453 -
Flags: superreview+
Attachment #335453 -
Flags: review?(bzbarsky)
Attachment #335453 -
Flags: review+
Comment 12•16 years ago
|
||
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?
Assignee | ||
Comment 13•16 years ago
|
||
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)
Reporter | ||
Comment 14•16 years ago
|
||
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.
Assignee | ||
Comment 15•16 years ago
|
||
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)
Reporter | ||
Updated•16 years ago
|
Attachment #335971 -
Flags: superreview?(bzbarsky)
Attachment #335971 -
Flags: superreview+
Attachment #335971 -
Flags: review?(bzbarsky)
Attachment #335971 -
Flags: review+
Comment 16•16 years ago
|
||
Looks like this has caused bustage on SeaMonkey :(
Assignee | ||
Comment 17•16 years ago
|
||
Checked in 1bdd5da49865. I also checked in a bustage fix for Seamonkey.
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 18•16 years ago
|
||
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.
Assignee | ||
Comment 19•16 years ago
|
||
I had to check in to CVS because that's where extensions/typeaheadfind currently lives :-(
Reporter | ||
Comment 20•16 years ago
|
||
Oh, ick. OK.
Assignee | ||
Comment 21•16 years ago
|
||
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.
Assignee | ||
Comment 22•16 years ago
|
||
Backed out to fix regression bug 453661.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 23•16 years ago
|
||
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
Assignee | ||
Comment 24•16 years ago
|
||
I'll reland this at some point.
Assignee | ||
Comment 25•16 years ago
|
||
Pushed f7414f6cb95b.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•