Closed Bug 1120705 Opened 5 years ago Closed 5 years ago

select element (when multiple) skips item when clicking arrows up or down

Categories

(Core :: Layout: Form Controls, defect)

34 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 + wontfix
firefox37 + wontfix
firefox38 + fixed
firefox39 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: will, Assigned: kip)

References

Details

(Keywords: regression, ux-control)

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045

Steps to reproduce:

http://www.mtlindsayrealty.com.au/ has a search area. The suburbs select and property types select are both "multiple" row select elements with size equal to 4. I clicked on the down arrow of the suburbs select.


Actual results:

When clicking on the down arrow it will appear to move to the next 4 items, but it skips one item. I can drag the scrollbar or use the up or down buttons on the keyboard to show that clicking the arrow missed an item.


Expected results:

Clicking on the up or down arrow should move to the previous or next 4 items in my select without skipping an item.
Component: Untriaged → Layout: Form Controls
Keywords: regression
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
[Tracking Requested - why for this release]:
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ec4622e6e1fa&tochange=4ef03db1385b

Regressed by: Bug 957445
Blocks: 957445
Flags: needinfo?(kgilbert)
Keywords: ux-control
Attached file bug1120705.html
In local build,
Last Good: 9eabf947efc3
First Bad: 249dae95ea83

Regressed by: 	249dae95ea83	Miranda Emery — Bug 957445: Part 2 - nsHTML/XULScrollFrame implement nsIScrollbarMediator, r=mats
CNR on OSX 10.10..

I will take this bug and investigate further on Win 8.1.
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
Kip, any news on this? Thanks
Flags: needinfo?(kgilbert)
(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> Kip, any news on this? Thanks
I will be setting up a Windows 8.1 dev environment next week, then will be investigating this further.
Flags: needinfo?(kgilbert)
Kip, Have you been able to install a 8.1 dev env and are you still planning to fix that for 36? Thanks
Flags: needinfo?(kgilbert)
I am still waiting on hardware to arrive for the 8.1 dev env.  It is past due, so hopefully I will be able to fix this shortly, for 36.
Flags: needinfo?(kgilbert)
Hmm, we build beta 9 today... I am going to mark it as wontfix.
I have reproduced this on Windows 8.1 on Nightly.  It appears to be a problem even prior to Bug 957445 landing; however, you would need to use a smaller listbox to be affected by it.

When paging down and up, no rows will be skipped.

When navigating with the scrollbar up/down arrow buttons, the scrolling advances by a fixed amount (controlled by the OS / OS preferences).  If the size of the list box is smaller than this fixed amount, rows can be skipped.

When Bug 957445 landed, the amount of scrolling in this case changed, possibly due to how rounding to the nearest line is performed.

Prior to Bug 957445, it was also possible to reproduce this issue, but would only affect smaller scroll boxes.
I have reviewed IE and Chrome's behavior on the attached HTML:
https://bugzilla.mozilla.org/attachment.cgi?id=8547885


IE scrolls by either one or two lines each time the user clicks the scroll down arrow, indicating that it is maintaining a non-rounded scroll position, which is incremented by a fixed amount and later rounded.

Chrome scrolls by approximately the same amount as Firefox; however, it appears to also clamp the scroll amount to a single page at a time, preventing any line items from being skipped when the scroll box is small.

Perhaps we should consider implementing a solution similar to Chrome's?  IMHO, IE's behavior in this case feels less ideal, scrolling by an inconsistent amount on every click.
Hi Kip, the final 37 beta is going to build this Thursday, so unless it's a simple fix, we may want to defer this to 38. What do you think?
Flags: needinfo?(kgilbert)
(In reply to Liz Henry (:lizzard) from comment #12)
> Hi Kip, the final 37 beta is going to build this Thursday, so unless it's a
> simple fix, we may want to defer this to 38. What do you think?
This will be a simple fix.  I expect it to be ready later today, if that is not too late.
Flags: needinfo?(kgilbert)
- When using the scrollbar buttons to scroll a very small scroll frame,
  it is possible to scroll more than one page.  When this occurs, we
  now scroll by a page instead to maintain context between scrolls.
- When scrolling with scrollbar buttons, the
  "toolkit.scrollbox.verticalScrollDistance" and
  "toolkit.scrollbox.horizontalScrollDistance" were swapped,
  resulting in too much scrolling when scrolling vertically and too
  little scrolling when scrolling horizontally.  This has been fixed in
  ScrollFrameHelper::ScrollByLine.
Attachment #8579620 - Flags: review?(roc)
Attached patch Bug 1120705 - Part 2: Tests (obsolete) — Splinter Review
- Ensure that the scrollbar button scrolling is limited
  by the page size, to maintain context when scrolling
  small scroll frames.
Attachment #8579739 - Flags: review?(roc)
Comment on attachment 8579739 [details] [diff] [review]
Bug 1120705 - Part 2: Tests

Review of attachment 8579739 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/tests/test_bug1120705.html
@@ +29,5 @@
> +  <option value="17">17</option>
> +  <option value="18">18</option>
> +  <option value="19">19</option>
> +  <option value="20">20</option>
> +</select> 

trailing whitespace

@@ +38,5 @@
> +
> +function clickDownButton() {
> +  var sel = document.getElementById("sel");
> +  var scrollbar_width = sel.offsetWidth - sel.clientWidth;
> +  synthesizeMouse(sel, 

trailing whitespace
Attachment #8579739 - Flags: review?(roc) → review+
- Ensure that the scrollbar button scrolling is limited
by the page size, to maintain context when scrolling
small scroll frames.

v2 Patch:
- Removed trailing whitespace
Attachment #8579739 - Attachment is obsolete: true
Comment on attachment 8579620 [details] [diff] [review]
Bug 1120705 - Part 1: Limit scrollbar button scrolls to one page per click

Approval Request Comment
[Feature/regressing bug #]: Bug 1120705, regressed by Bug 957445
[User impact if declined]: If declined, users that scroll using scrollbar arrow buttons may skip over content on very small scroll frames.
[Describe test coverage new/current, TreeHerder]: Tested manually on Windows 8.1, implemented test, and pushed to try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=88656d4545c1)
[Risks and why]: Low/Medium risk - The changes were all within one function (ScrollFrameHelper::ScrollByLine), which is used only for the affected button scrolling.  Behavior has been updated to include clamping to one page of scrolling, which is also used by Chrome.
[String/UUID change made/needed]: N/A
Attachment #8579620 - Flags: approval-mozilla-beta?
Comment on attachment 8579743 [details] [diff] [review]
Bug 1120705 - Part 2: Tests (v2 Patch), r=roc

Approval Request Comment
[Feature/regressing bug #]: Bug 1120705, regressed by Bug 957445
[User impact if declined]: If declined, users that scroll using scrollbar arrow buttons may skip over content on very small scroll frames.
[Describe test coverage new/current, TreeHerder]: Tested manually on Windows 8.1, implemented test, and pushed to try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=88656d4545c1)
[Risks and why]: Low/Medium risk - The changes were all within one function (ScrollFrameHelper::ScrollByLine), which is used only for the affected button scrolling.  Behavior has been updated to include clamping to one page of scrolling, which is also used by Chrome.
[String/UUID change made/needed]: N/A
Attachment #8579743 - Flags: approval-mozilla-beta?
The fix is just a bit too late for 37. But, we'll have this fixed in 38.
Comment on attachment 8579620 [details] [diff] [review]
Bug 1120705 - Part 1: Limit scrollbar button scrolls to one page per click

Beta-

Adding approval request for 38 Aurora.
Attachment #8579620 - Flags: approval-mozilla-beta?
Attachment #8579620 - Flags: approval-mozilla-beta-
Attachment #8579620 - Flags: approval-mozilla-aurora?
Attachment #8579743 - Flags: approval-mozilla-beta?
Attachment #8579743 - Flags: approval-mozilla-beta-
Attachment #8579743 - Flags: approval-mozilla-aurora?
Comment on attachment 8579620 [details] [diff] [review]
Bug 1120705 - Part 1: Limit scrollbar button scrolls to one page per click

We will have the whole beta cycle for this and the changes are located in a single place with tests, taking it.
Attachment #8579620 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8579743 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Carsten Book [:Tomcat] from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/97aff00e4250
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e48cd77481ac

Please be sure to double-check the commit info (including author) when pushing checkin-neededs.
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.