will Reporter
	Description • 10 years ago

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.

	Alice0775 White
	Comment 1 • 10 years ago

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

Regressed by: Bug 957445

	Alice0775 White
	Comment 3 • 10 years ago

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

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

	:kip (Kearwood Gilbert) Assignee
	Comment 4 • 10 years ago

CNR on OSX 10.10..

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

	Sylvestre Ledru [:Sylvestre]
	Comment 5 • 10 years ago

Kip, any news on this? Thanks

	:kip (Kearwood Gilbert) Assignee
	Comment 6 • 10 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 7 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 8 • 10 years ago

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.

	Sylvestre Ledru [:Sylvestre]
	Comment 9 • 10 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 10 • 9 years ago

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.

	:kip (Kearwood Gilbert) Assignee
	Comment 11 • 9 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 12 • 9 years ago

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?

	:kip (Kearwood Gilbert) Assignee
	Comment 13 • 9 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 14 • 9 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 15 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 9 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 17 • 9 years ago

- 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

	:kip (Kearwood Gilbert) Assignee
	Comment 18 • 9 years ago

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

	:kip (Kearwood Gilbert) Assignee
	Comment 19 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 20 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 21 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 9 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 23 • 9 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 24 • 9 years ago

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.

	Sylvestre Ledru [:Sylvestre]
	Comment 25 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

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