Closed Bug 290428 Opened 15 years ago Closed 14 years ago

HTML Select Object Scrolls on Mouseover

Categories

(Core :: Layout: Form Controls, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: mozilla, Assigned: roc)

References

Details

(Keywords: regression, testcase)

Attachments

(5 files, 1 obsolete file)

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.
Attached file Testcase (obsolete) —
To replicate: Open dropdown menu and repeatedly move mouse from the box into
the menu list and back over the box.
This isn't limited to Firefox.
Product: Firefox → Mozilla Application Suite
Component: General → XP Apps: GUI Features
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
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?
Assignee: firefox → nobody
Component: XP Apps: GUI Features → Layout: Form Controls
Product: Mozilla Application Suite → Core
QA Contact: general → layout.form-controls
Flags: blocking1.8b3?
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.
Summary: HTML Select Object with OnChange Event Scrolls on Mouseover → HTML Select Object Scrolls on Mouseover
Attachment #180774 - Attachment is obsolete: true
Keywords: regression
roc, this is probably yours....
Assignee: nobody → roc
Attached patch fixSplinter Review
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.
Attachment #184039 - Flags: superreview?(bzbarsky)
Attachment #184039 - Flags: review?(bzbarsky)
Comment on attachment 184039 [details] [diff] [review]
fix

r+sr=bzbarsky
Attachment #184039 - Flags: superreview?(bzbarsky)
Attachment #184039 - Flags: superreview+
Attachment #184039 - Flags: review?(bzbarsky)
Attachment #184039 - Flags: review+
Comment on attachment 184039 [details] [diff] [review]
fix

we probably should fix this for b3. It's a bad user experience.
Attachment #184039 - Flags: approval1.8b3?
Comment on attachment 184039 [details] [diff] [review]
fix

a=shaver
Attachment #184039 - Flags: approval1.8b3? → approval1.8b3+
Depends on: 296803
This patch was checked in. Marking Fixed unless anyone objects.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
I can reproduce the bug with Mozilla nightly build 2005-06-15-02 Linux gtk2+xft.
Status: RESOLVED → REOPENED
Keywords: testcase
OS: Windows XP → All
Resolution: FIXED → ---
Ahhh, when the mouse is *up* this still can happen because the mouse cursor
moves over the border area of the frame.
Attached patch more fixSplinter Review
This fixes the mouse-up-over-border issue.
Attachment #186421 - Flags: superreview?(bzbarsky)
Attachment #186421 - Flags: review?(bzbarsky)
Attachment #186421 - Flags: superreview?(bzbarsky)
Attachment #186421 - Flags: superreview+
Attachment #186421 - Flags: review?(bzbarsky)
Attachment #186421 - Flags: review+
Attachment #186421 - Flags: approval1.8b3?
Attachment #186421 - Flags: approval1.8b3? → approval1.8b3+
checked in
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Flags: blocking1.8b3?
I am still seeing this with 20050618 trunk. Will attach another test case.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file 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.
Also verified to still occur in 20050621 Firefox trunk.
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".
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Attached patch real fixSplinter Review
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.
Attachment #187088 - Flags: superreview?(dbaron)
Attachment #187088 - Flags: review?(dbaron)
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.
Attachment #187088 - Flags: superreview?(dbaron)
Attachment #187088 - Flags: superreview+
Attachment #187088 - Flags: review?(dbaron)
Attachment #187088 - Flags: review+
Yes, good idea. I'll do that.
Comment on attachment 187088 [details] [diff] [review]
real fix

regression fix to a minor UI polish issue. The risk here is unknown.
Attachment #187088 - Flags: approval1.8b3?
Attachment #187088 - Flags: approval1.8b3? → approval1.8b3+
Is there any chance of getting this checked in? Still seeing the issue in
20050707 builds.
checked in. Sorry, this one slipped past me. Thanks for the heads-up.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Could this have caused bug 301439?
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.
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.
Sorry, when I visited that page the first time, I didn't get anything. Now I see
the problem.
There is still a problem here but it's pretty minor now. I'll leave this for
post-1.8.
WFM.
Status: REOPENED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.