Closed Bug 186229 Opened 22 years ago Closed 20 years ago

BoxObject incorrect when scrollable ancestors

Categories

(Core :: Layout, defect, P4)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: erik, Unassigned)

References

Details

(Keywords: testcase)

Attachments

(3 files)

The BoxObject for an element works fine in the simple scenario where only the root element has a scrollable overflow. Whenever one or more ancestor elements has been scrolled the returned box is incorrect and returns the value that would have been correct when all the scrollable elements had scrollTop 0. #e1, #e2 { overflow: auto; } <element id="e1"> ... <element id="e2"> ... <element id="e3">...</element> ... </element> ... </element> var y = document.getBoxObjectFor(document,getElementById("e3")).y y is only correct if the elements e1 and e2 have not been scrolled.
Move the mouse around and note how the outline paints on top of the elements. Then scroll any of the scrollable elements (you might need to resize your widnow to see the scrollbars). Note now that the result from getBoxObjectFor ais incorrect because the scrollTop of the ancestors are note taken into account. I'm sorry for the flickery nature of the test case but I did not want to waste time on making it look nice.
Keywords: testcase
Priority: -- → P4
Target Milestone: --- → Future
This patch adjusts the algorithm for calculating the boxObject screenX and screenY so that it includes the scroll offsets for any scrolled views as it works up the heirachy. This does not affect the boxObject values for the item actually being scrolled, only it's children (and I think this is what it should do, IMHO). This is my first C++ patch, so any "you shouldn't do THAT!" comments are fine. :)
Attachment #137734 - Flags: review?(dbaron)
Shouldn't this code just do what other similar callers do and walk the view chain instead of the frame chain? For that matter, do we have enough people needing this yet (I can think of at least two or three) to have a way to get this info off an nsIView? Especially since people seem to screw it up 99% of the time when they try to implement it?
Actually, we could even put this on nsIFrame (have a ScreenRect() accessor or something that properly walks the view tree to a widget, etc), since half the magic is properly getting hold of a view...
Check out nsIView::GetNearestWidget in this patch (currently undergoing review :-) ) http://bugzilla.mozilla.org/attachment.cgi?id=135391&action=view I think this is the right way to write this API. I want to replace nsIFrame::GetOffsetFromView, nsIFrame::GetOriginToViewOffset and nsIFrame::GetClosestView with a similar GetNearestView method. We should also replace nsIFrame::GetWindow with an nsIFrame::GetNearestWidget and use it here.
James, do you want to have a go at reorganizing the functions like I said in my last comment?
Robert, I could have a go, but I'm not too familiar with the frame/view/widget stuff Mozilla uses, or C++ (not that that stopped me making a patch anyway :) ).
Comment on attachment 137734 [details] [diff] [review] Fixed boxObject for boxes inside scrolled containers Marking review- based on above comments from bz and roc.
Attachment #137734 - Flags: review?(dbaron) → review-
Keywords: helpwanted
Whiteboard: [good first bug]
The function reorganization Robert wanted happened in bug 268576. This is fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Depends on: 268576
(In reply to comment #10) > The function reorganization Robert wanted happened in bug 268576. This is > fixed. So this bug should be marked FIXED, right?
(In reply to comment #11) > So this bug should be marked FIXED, right? Ugh, ignore this comment please.
Keywords: helpwanted
Whiteboard: [good first bug]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: