186229 - BoxObject incorrect when scrollable ancestors

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[Erik Arvidsson]

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.

Comment 1

[Erik Arvidsson]

Attachment: Paints an outline on top of the element given the info from the BoxObject

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.

Comment 2

[James Ross]

Attachment: Shows the values of various boxObject properties for nested scrolling container.

Comment 3

[James Ross]

Attachment: Fixed boxObject for boxes inside scrolled containers

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.
:)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

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

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.

Comment 7

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

James, do you want to have a go at reorganizing the functions like I said in my
last comment?

Comment 8

[James Ross]

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 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 137734 [details] [diff] [review]
Fixed boxObject for boxes inside scrolled containers

Marking review- based on above comments from bz and roc.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The function reorganization Robert wanted happened in bug 268576.  This is fixed.

Comment 11

[Martijn Wargers (dead)]

(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?

Comment 12

[Martijn Wargers (dead)]

(In reply to comment #11)
> So this bug should be marked FIXED, right?
Ugh, ignore this comment please.