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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[will]

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.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]:
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ec4622e6e1fa&tochange=4ef03db1385b

Regressed by: Bug 957445

Comment 2

[Alice0775 White]

Attachment: bug1120705.html

Comment 3

[Alice0775 White]

In local build,
Last Good: 9eabf947efc3
First Bad: 249dae95ea83

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

Comment 4

[:kip (Kearwood Gilbert)]

CNR on OSX 10.10..

I will take this bug and investigate further on Win 8.1.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Kip, any news on this? Thanks

Comment 6

[:kip (Kearwood Gilbert)]

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

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Kip, Have you been able to install a 8.1 dev env and are you still planning to fix that for 36? Thanks

Comment 8

[:kip (Kearwood Gilbert)]

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.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Hmm, we build beta 9 today... I am going to mark it as wontfix.

Comment 10

[:kip (Kearwood Gilbert)]

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.

Comment 11

[:kip (Kearwood Gilbert)]

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.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 13

[:kip (Kearwood Gilbert)]

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

Comment 14

[:kip (Kearwood Gilbert)]

Attachment: Bug 1120705 - Part 1: Limit scrollbar button scrolls to one page per click

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

Comment 15

[:kip (Kearwood Gilbert)]

Attachment: Bug 1120705 - Part 2: Tests

- Ensure that the scrollbar button scrolling is limited
  by the page size, to maintain context when scrolling
  small scroll frames.

Comment 16

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

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

Comment 17

[:kip (Kearwood Gilbert)]

Attachment: Bug 1120705 - Part 2: Tests (v2 Patch), r=roc

- 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

Comment 18

[:kip (Kearwood Gilbert)]

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

Comment 19

[:kip (Kearwood Gilbert)]

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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97aff00e4250
https://hg.mozilla.org/integration/mozilla-inbound/rev/e48cd77481ac

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97aff00e4250
https://hg.mozilla.org/integration/mozilla-inbound/rev/e48cd77481ac

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97aff00e4250
https://hg.mozilla.org/mozilla-central/rev/e48cd77481ac

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

The fix is just a bit too late for 37. But, we'll have this fixed in 38.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1126b4e55257
https://hg.mozilla.org/releases/mozilla-aurora/rev/127ecffe3239