290428 - HTML Select Object Scrolls on Mouseover

Status: RESOLVED WORKSFORME

(Core :: Layout: Form Controls, defect)

Description

[Jerry Baker]

20050408 trunk.

If you open a dropdown menu that has an onChange event, mousing in and out of
the expanded menu causes the menu to scroll one time for each mouseover/out.

Comment 1

[Jerry Baker]

Attachment: Testcase

To replicate: Open dropdown menu and repeatedly move mouse from the box into
the menu list and back over the box.

Comment 2

[Jerry Baker]

This isn't limited to Firefox.

Comment 3

[Kevin Brosnan]

If a bug is in more than one product the it is a product: core bug.
Layout: Form Controls
XP Toolkit/Widgets
Widget: Win32
all look like possible components

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is the onchange relevant?  Or do we still scroll without it?

Also, do you happen to know whether this is a regression, and if so from when?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This regressed between the 0403 and the 0404 nightlies.

Range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&sortby=Date&date=explicit&mindate=2005-04-03+05%3A00%3A00&maxdate=2005-04-04+07%3A00%3A00&cvsroot=%2Fcvsroot

Likely candidates:
bug 287730
bug 288888
bug 288821
bug 288117

The onchange handler isn't necessary to reproduce the bug.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: Testcase 2 (no onchange)

Comment 7

[Boris Zbarsky [:bzbarsky]]

roc, this is probably yours....

Comment 8

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

Attachment: fix

There are a couple of things going on here.

One is that as you drag inside the dropdown list we try to select the item your
mouse would be over if the list wasn't clipped. But if there's only one item
scrolled off the top, and your mouse is say 3 items above the top of the list,
then it's not over any item even ignoring clipping. So we don't scroll. This is
why when you drag the mouse around above or below the list scrolling tends to
stop before the start (or end) of the list. So part of this patch checks the
position of the mouse; if it's above the first item or below the last item,
then we pretend that the mouse is over the first/last item.

Another thing that's going on is that if the mouse is over the combobox frame
(the "display the currently selected item" frame in the page), then we tend to
ignore the event. This is to avoid problems when the user has just clicked on
the combobox frame to show the dropdown, and the mouse then moves through the
combobox frame to the dropdown frame. This is the real cause of this bug;
during the transition between frames we sometimes get an event over the border
of the listcontrolframe; this isn't in the comboboxframe, so we select the
element under the mouse, which is the element above the current top element.
So, this patch changes IsClickingInCombobox to be called
IgnoreMouseEventForSelection to reflect its real purpose. I change the
implementation so that we ignore mouse events (for selection purposes) until
the mouse has moved below the listframe's inner-border-edge. Once that happens
we use mouse events for selection even if the mouse moves back above that edge,
which seems much more intuitive than ignoring them while the mouse is in the
comboboxframe area, as we currently do. Note that padding in the listframe is
not an issue because if any of the padding-top is visible, we must already be
scrolled to the top of the list.

To get this to work I implemented a helper in nsLayoutUtils to get the frame
coordinates from a DOM event. I think we'll find that useful in many places.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

r+sr=bzbarsky

Comment 10

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

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

we probably should fix this for b3. It's a bad user experience.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

a=shaver

Comment 12

[Caleb]

This patch was checked in. Marking Fixed unless anyone objects.

Comment 13

[Jerry Baker]

I still see this in 20050609 trunk. The thing is now that it doesn't happen with
each mouseover. If you load the test case and do it a half-dozen times, you'll
see it happen.

Comment 14

[Mats Palmgren (inactive)]

I can reproduce the bug with Mozilla nightly build 2005-06-15-02 Linux gtk2+xft.

Comment 15

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

Ahhh, when the mouse is *up* this still can happen because the mouse cursor
moves over the border area of the frame.

Comment 16

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

Attachment: more fix

This fixes the mouse-up-over-border issue.

Comment 17

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

checked in

Comment 18

[Jerry Baker]

I am still seeing this with 20050618 trunk. Will attach another test case.

Comment 19

[Jerry Baker]

Attachment: Another test case

This test case still shows the issue after the patch. Scroll far down the list
and then move the mouse over the top edge of the list. Also works on the bottom
edge of the list.

Comment 20

[Jerry Baker]

Also verified to still occur in 20050621 Firefox trunk.

Comment 21

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

This is just us selecting the topmost visible or bottommost visible element,
which scrolls it fully into view. I'm calling this "not a bug".

Comment 22

[Jerry Baker]

No it isn't. I can scroll from the bottom of the list to the top just using
mouseovers. Please try the testcase before changing the status.

Comment 23

[Jerry Baker]

In fact, if you get the mouse in just the right place between the dropdown and
the box, you can wiggle it side-to-side and scroll the whole list very rapidly.

Comment 24

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

What's happening now is pretty nasty. It's a subpixel positioning problem. It
boils down to something like this: suppose we have an option covering the
vertical interval [0,10.3) and another option covering [10.3,20). When we scroll
the second option into view the scroll ends up positioned at vertical offset 10
because scrolling can only be in pixel increments. Now when the mouse is at
offset zero in the scrolling area, 10 in the scrolled area, it's actually over
the first option, so we scroll the first option into view.

I *think* that what we should do is, when we scroll something into view, we
scroll so that it's first displayed pixel is at the top. In the above example we
should scroll to offset 11 because that's the topmost pixel that the second
option actually covers.

Comment 25

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

Attachment: real fix

Okay, here's the right fix. Almost all our twips to pixel conversion code does
rounding. That means we effectively say that a pixel belongs to whatever frame
includes its center point. THAT means when we translate a mouse coordinate from
pixels to twips, we should actually translate it to the center of the pixel by
adding 0.5 to its x and y. Doing that fixes this bug.

This is definitely the right thing to do, but it could be risky if it runs into
some incorrect handling of subpixel positions in some other part of the code.
So I'm not sure it's worth putting this into 1.8.

Comment 26

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

Comment on attachment 187088 [details] [diff] [review]
real fix

>Index: view/src/nsViewManager.cpp
>+          // Set the mouse cursor to the middle of the pixel, becaus

trailing "e" missing

>+          // that's how we paint --- a frame paints a pixel if it covers
>+          // the center of the pixel
>+          aEvent->point.x = baseViewDimensions.x +
>+            NSFloatPixelsToTwips(aEvent->point.x + 0.5, p2t);
>+          aEvent->point.y = baseViewDimensions.y +
>+            NSFloatPixelsToTwips(aEvent->point.y + 0.5, p2t);

Any chance you could make some of the conversions here a little more explicit? 
You're adding an int and a double and converting the result (which I'd hope
would be a double, but I'm not sure) to a float.  Maybe you want something more
like

NSFloatPixelsToTwips(float(aEvent->point.x) + 0.5f, p2t);

?  (Explicit float cast and a float constant rather than a double constant.)



This seems vaguely scary to be taking at this stage, but r+sr=dbaron.  I guess
it should be pretty obvious if it breaks things.

Comment 27

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

Yes, good idea. I'll do that.

Comment 28

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

Comment on attachment 187088 [details] [diff] [review]
real fix

regression fix to a minor UI polish issue. The risk here is unknown.

Comment 29

[Jerry Baker]

Is there any chance of getting this checked in? Still seeing the issue in
20050707 builds.

Comment 30

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

checked in. Sorry, this one slipped past me. Thanks for the heads-up.

Comment 31

[Jerry Baker]

I'm still seeing this in 20050713 trunk. I only seem to notice on lists that are
at the bottom of the page so that the list opens upward.

Go to
http://data1.cde.ca.gov/dataquest/SearchName.asp?rbTimeFrame=oneyear&rYear=2004-05&cName=&Topic=Profile&Level=District&submit1=Submit
and play with the list at the bottom of the page.

Comment 32

[Martijn Wargers (dead)]

Could this have caused bug 301439?

Comment 33

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

Jerry, I'd need a better testcase, that page doesn't give me anything. In any
case I think this may be "good enough" for 1.8 now.

Comment 34

[Jerry Baker]

I'm not sure what criteria need to be satisfied in the testcase. The linked page
scrolls the list when you mouseover the list. I'll be happy to try and make a
testcase as soon as I know what it needs.

Comment 35

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

Sorry, when I visited that page the first time, I didn't get anything. Now I see
the problem.

Comment 36

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

There is still a problem here but it's pretty minor now. I'll leave this for
post-1.8.

Comment 37

[Jerry Baker]

WFM.