242833 - mouse not scrolling properly in absolutely positioned iframes

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[Brandon Amedee]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040506

The mouse is not working properly with the scrollbar on the absolutely
positioned iframe at http://exonyte.dyndns.org/scrollbug.html, because it is
layered over a fixed position div.

It seems the mouse coordinates are being read incorrectly whenever it is moved
over the iframe, causing odd scrolling and context menu problems as described in
the iframe itself.  The scrollbar will not scroll corretly if you grab the box
and move up or down, and the context menu is not appearing near the mouse as it
should.

None of the problems occur on this page outside of the iframe.

I apologize if any of this sounds confusing.  I'll try to attach some
screenshots later on if nobody else can see the problem.

Reproducible: Always
Steps to Reproduce:
1. Go to http://exonyte.dyndns.org/scrollbug.html
2. Try to scroll the iframe in the absolutely positioned div by grabbing the
scrollbar's box with the mouse, and dragging it downward.

For more testing:
1. Go to http://exonyte.dyndns.org/scrollbug.html
2. Resize the Mozilla window so that there are scrollbars on the main document.
3. Scroll the main document to another position.
4. Scroll the iframe again as described above.

Actual Results:  
The iframe will treat the mouse as if it were in a different position and,
depending on the position of the main document, will not scroll until you have
moved the mouse accordingly.


Expected Results:  
The iframe should scroll like any other window, with the scrollbar's box keeping
up with the mouse.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Could we be using the wrong widget here?

Comment 2

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

probably

Comment 3

[R.K.Aa.]

A real site here:
http://www7.nrk.no/nrkplayer/testnettleser.aspx?hovedkategori_id=2&prosjekt_id=0&kategori_id=0&klipp_id=0&indeks_id=0&oppgave_id=0&sok=&bhcp=1

Comment 4

[Brandon Amedee]

Attachment: a screenshot of scrolling bug in action

In this screenshot, the iframe has been scrolled as described, and this is the
result.  The mouse button is still held down at this point.

Comment 5

[Brandon Amedee]

(In reply to comment #3)
> A real site here:
>
http://www7.nrk.no/nrkplayer/testnettleser.aspx?hovedkategori_id=2&prosjekt_id=0&kategori_id=0&klipp_id=0&indeks_id=0&oppgave_id=0&sok=&bhcp=1

That site doesn't seem to trigger the bug in my Firefox 0.8, it does in Mozilla
Nightly 2004050609.  My test case does trigger it in Firefox 0.8 though.

I discovered this bug yesterday (May 5) while working on this website:
http://exonyte.dyndns.org/website41/

The faded piece of the logo in the top left is a fixed div that is sized by
width: 100%; height: 100%, and the "shoutbox" on the right is inside of an
absolute div.  The site is still in development, but that part of the design is
finished.

Should I post a seperate bug for the context menu issue that I'm assuming is
related?

Comment 6

[R.K.Aa.]

rv:1.8a Gecko/20040503 Firefox/0.8.0+ on Linux shows the bug also in the comment
7 site

Comment 7

[Keith Gable]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

Same problem. Full screen at 1280x1024, it seems as if the coordinates are off
by about the same distance the iframe is from the top of the div it's nested in.
Perhaps Mozilla is reading the coords for the parent div?

Comment 8

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

same as bug 213276?

Comment 9

[Brandon Amedee]

(In reply to comment #8)
> same as bug 213276?

That most definitely sounds like the context menu bug I ran across.  The test
case is down so I can't verify though.

Comment 10

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

Looks like frame events (selection) work properly, DOM events (scrollbars,
context menu) don't ... but it looks like DOM events only don't work when the
mouse is grabbed. That suggests they're not handling event->refPoint correctly.

Comment 11

[R.K.Aa.]

Releasing the grab when cursor is ouside of the scrollbar area makes the grab
"persistant":
Then slide mouse in over the scrollbar again: the slider moves up/down by just
moving the mouse vertically, no buttons pressed.

Comment 12

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

Yeah. DOM assumes that event->refPoint is in pixels and relative to the origin
of event->widget. Not an unreasonable assumption. The question is, what is the
best way to fix this. For one thing, it doesn't make a whole lot of sense for
nsEvent to have a refPoint field that's relative to the widget which is only
contained in nsGUIEvents. (It doesn't make a whole lot of sense for
non-nsGUIEvents to have a point field, either...)

Comment 13

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

I think I'll try changing refPoint to just be screenPoint and always be in
screen coordinates in pixels. I'll also try moving it to nsMouseEvent. That
should simplify things.

Comment 14

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

Since WidgetToScreen is really expensive on X11, we can't have screenPoint
always be in screen coordinates.

Instead of trying to clean up I'll just fix the underlying bug, which is that
nsDOMEvent::GetClientX/Y assumes the widget for the document root is a parent of
the widget that received the event. This isn't necessarily true when the
document is a subdocument and the the event came from outside the subdocument.

Comment 15

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

Attachment: fix

Here's the fix.

We fix GetClientX/Y so that it can deal with the situation where the document
root widget is not an ancestor of the event widget. I took the chance to factor
out the code for these two methods (and for GetScreenX/Y) to reduce code
duplication and simplify the code.

I also noticed that after that fix, mouse capturing doesn't work in the IFRAME.
The problem is that mouse capturing is currently per-view-manager. We set mouse
capturing in the IFRAME view manager but all the mouse events are dispatched to
the widget for the fixed DIV outside the IFRAME, which has a different view
manager, which doesn't know about the mouse capturing. So I changed
nsViewManager to forward mouse capture control to the root of the view manager
tree. That seems to make things work great.

Comment 16

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

Attachment: fix

Oops, attached wrong file.

Comment 17

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

Comment on attachment 148374 [details] [diff] [review]
fix

fairly straightforward stuff.

Comment 18

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

Either the diff is very confusing or GetClientPoint does lots of work and then
just returns |pt| unmodified from being aEvent->refPoint.

Comment 19

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

Attachment: fix!!

Ah, that's because I am very stupid. Here is the real patch.

Comment 20

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

Comment on attachment 148378 [details] [diff] [review]
fix!!

okay. I think this is it :-)

Comment 21

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

checked in.

Comment 22

[Andreas Kunz]

This caused bug 243720.

Comment 23

[Johnny Stenback (:jst)]

This caused bug 244216.

Comment 24

[Alfred Kayser]

A maybe silly comment regarding:
>We fix GetClientX/Y so that it can deal with the situation where the document
>root widget is not an ancestor of the event widget. I took the chance to factor
>out the code for these two methods (and for GetScreenX/Y) to reduce code
>duplication and simplify the code.

Here is an opportunity to improve even further:
It seems to me that when calling GetClientX and GetClientY right after each
other the GetClientPoint function gets called twice to calculate the some point.
For example in:
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditorMouseListener.cpp#103
103     mouseEvent->GetClientX(&clientX);
104     mouseEvent->GetClientY(&clientY);

In fact this happens at:
/editor/libeditor/html/nsHTMLEditorMouseListener.cpp, line 103, 274
/editor/libeditor/html/nsHTMLObjectResizer.cpp, line 889, 914, 933 
/embedding/browser/cocoa/src/CHClickListener.mm, line 191 
/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 1238
/content/xul/content/src/nsXULPopupListener.cpp, line 421
/layout/xul/base/src/nsXULTooltipListener.cpp, line 175, 325

Only in nsDOMEvent.cpp, nsSliderFrame.cpp, nsSplitterFrame.cpp use only the X or
Y value.

As the IDOMMouseEvent is frozen, we cannot expose 'GetClientPoint(&x,&y)' or
something like that. 
But at least the calculated point (for which the code went through a lot
trouble) seems worth of caching (at least for the not-so-simple cases) in maybe
mClientPoint itself?

Comment 25

[Hermann Schwab]

check in for this bug seems to have caused regression in
Bug 245487 Select inputs with many Options "jump" when being scrolled.

BuildID 2003051308 was buggy here and working there
BuildID 2003051315 was fixed here and regressed there, showing the scrolling
behaviour seen here in a iframe there in a select input in an normal frame, with
a frame above, see simple testcase there.

Comment 26

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

Attachment: fix leak regression

Comment 27

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

Comment on attachment 153270 [details] [diff] [review]
fix leak regression

(I filed bug 251527 on making nsIWidget::GetParent return
already_AddRefed<nsIWidget>.)

Comment 28

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

Comment on attachment 153270 [details] [diff] [review]
fix leak regression

Checked in to trunk, 2004-07-15 12:58 -0700.

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)
> BuildID 2003051308 was buggy here and working there
> BuildID 2003051315 was fixed here and regressed there, showing the scrolling

This could have caused bug 244385 comment 10 too...