1321472 - [e10s] Can not scroll the drop-down list when click on option and with drag

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[blinky]

Steps to reproduce:

1. Go to https://bugzilla.mozilla.org/attachment.cgi?id=8811257.
2. Open the <select> element.
3. Click-and-hold on one of the options, and drag down the drop-down list.


Actual results:

can not scroll the drop-down list when click on option and with drag.

Comment 1

[Neil Deakin]

Attachment: Drag when pressing on menuitem

Comment 2

[Neil Deakin]

Attachment: Part 1 - handle capture in popups

The test in the patch shows the issue fixed by it. Capturing the mouse while showing the popup causes the mousemove events to be targeted incorrectly as the contents of the popup are not included in the display list used for hit testing. Instead, when a popup is capturing the mouse, use the popup as the root frame.

Comment 3

[Neil Deakin]

Attachment: Part 2 - allow drag-scrolling the select popup when the mouse is pressed on an option in the list

Comment 4

[Neil Deakin]

Attachment: Part 3 - move the drag-scrolling behaviour into the menulist binding so that both remote <select> elements and <menulist> can share this behaviour

Comment 5

[Neil Deakin]

Attachment: Part 4 - menulist test

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8825797 [details] [diff] [review]
Part 1 - handle capture in popups

I guess we can do this. Popups effectively prevent capturing.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8825798 [details] [diff] [review]
Part 2 - allow drag-scrolling the select popup when the mouse is pressed on an option in the list

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

I have some suggestions on how to improve the readability of this - but the logic looks good.

::: browser/base/content/test/general/browser_selectpopup.js
@@ +482,5 @@
> +  ok(selectPopup.scrollBox.scrollTop, scrollPos, "scroll position at drag up from option");
> +
> +  scrollPos = selectPopup.scrollBox.scrollTop;
> +  EventUtils.synthesizeMouseAtPoint(popupRect.left + 20, popupRect.bottom + 25, { type: "mouseup" }, win);
> +  is(selectPopup.scrollBox.scrollTop, scrollPos, "scroll position at mouseup from option");

Probably should update the message to make it clearer that we expect it to have not changed, ie:

"scroll position at mouseup from option should not change"

@@ +485,5 @@
> +  EventUtils.synthesizeMouseAtPoint(popupRect.left + 20, popupRect.bottom + 25, { type: "mouseup" }, win);
> +  is(selectPopup.scrollBox.scrollTop, scrollPos, "scroll position at mouseup from option");
> +
> +  EventUtils.synthesizeMouseAtPoint(popupRect.left + 20, popupRect.bottom + 20, { type: "mousemove" }, win);
> +  is(selectPopup.scrollBox.scrollTop, scrollPos, "scroll position at mouseup from option again");

Same as above-ish, something like:

"scroll position after mouseup and mousemove from option should not change"

::: toolkit/modules/SelectParentHelper.jsm
@@ +26,5 @@
>  this.SelectParentHelper = {
> +  // Indicates whether dragging over the popup:
> +  //   0 - not dragging
> +  //  -1 - dragging, but mouse is still over select button
> +  //   1 - dragging over popup

Can we add some consts for these?

like,

const NOT_DRAGGING = 0;
const DRAGGING_OVER_SELECT = -1;
const DRAGGING_OVER_POPUP = 1;

and referring to those below? I think that'd improve readability.

@@ +27,5 @@
> +  // Indicates whether dragging over the popup:
> +  //   0 - not dragging
> +  //  -1 - dragging, but mouse is still over select button
> +  //   1 - dragging over popup
> +  draggingOverPopup: 0,

It's a little confusing to have this be called draggingOverPopup still, when sometimes we aren't actually dragging over the popup. Maybe make this draggingState ?

@@ +163,4 @@
>              }
>            }
>  
> +          if (this.draggingOverPopup > 0 &&

Probably should make this something like:

if (this.draggingState == DRAGGING_OVER_POPUP && ...

Unless there's a good reason to use greater than / less than semantics for these flags...

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8825799 [details] [diff] [review]
Part 3 - move the drag-scrolling behaviour into the menulist binding so that both remote <select> elements and <menulist> can share this behaviour

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

Seems okay - but a lot of what I recommended in the other patch should probably be transferred as well.

::: toolkit/content/widgets/popup.xml
@@ +648,5 @@
> +      <!--
> +        Indicates whether dragging over the popup:
> +          0 - not dragging
> +         -1 - dragging, but mouse hasn't yet moved over the popup; don't scroll
> +          1 - dragging and allow scrolling over popup

I guess my advice for the other patch holds here, re: naming of _draggingOverPopup and these integer values having more readable names.

@@ +739,5 @@
> +              this._draggingOverPopup = 1;
> +            }
> +          }
> +
> +          if (this._draggingOverPopup > 0 &&

Same advice as in the other patch regarding this and checking for equivalence instead.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8825800 [details] [diff] [review]
Part 4 - menulist test

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

Looks good. Should probably just make some of those assertion messages clearer about what they're checking.

::: toolkit/content/tests/chrome/test_menulist.xul
@@ +321,5 @@
> +
> +    // First, check that scrolling does not occur when the mouse is moved over the
> +    // anchor button but not the popup yet.
> +    synthesizeMouseAtPoint(popupRect.left + 5, popupRect.top - 10, { type: "mousemove" });
> +    is(popup.scrollBox.scrollTop, scrollPos, "scroll position after mousemove over button");

These messages should probably indicate that we don't expect the scroll position to have changed yet.

@@ +340,5 @@
> +    synthesizeMouseAtPoint(popupRect.left + 20, popupRect.bottom + 25, { type: "mouseup" });
> +    is(popup.scrollBox.scrollTop, scrollPos, "scroll position at mouseup");
> +
> +    synthesizeMouseAtPoint(popupRect.left + 20, popupRect.bottom + 20, { type: "mousemove" });
> +    is(popup.scrollBox.scrollTop, scrollPos, "scroll position at mouseup again");

Message should probably indicate that scroll should not have changed.

@@ +356,5 @@
> +    is(popup.scrollBox.scrollTop, scrollPos, "scroll position at drag up from item");
> +
> +    scrollPos = popup.scrollBox.scrollTop;
> +    synthesizeMouseAtPoint(popupRect.left + 20, popupRect.bottom + 25, { type: "mouseup" });
> +    is(popup.scrollBox.scrollTop, scrollPos, "scroll position at mouseup from item");

Messages here down should probably indicate that we're expecting the scroll position to not change.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f0daf45683
when the capturing content is inside a popup, use the popup as the root frame when searching for a mouse target rather than the root frame, otherwise the capturing content's frame won't be found, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a0dc12ec71f
allow drag-scrolling the select popup when the mouse is pressed on an option in the list, r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e79c2a8338
move the drag-scrolling behaviour into the menulist binding so that both remote <select> elements and <menulist> can share this behaviour, r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/efeda43ab7b2
test for drag scrolling in a menulist, r=mconley

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/86f0daf45683
https://hg.mozilla.org/mozilla-central/rev/8a0dc12ec71f
https://hg.mozilla.org/mozilla-central/rev/30e79c2a8338
https://hg.mozilla.org/mozilla-central/rev/efeda43ab7b2

Comment 12

[Fahima Zulfath]

[testday-20170317] The drop down menu is scrolling when clicked and scroll. The bug is verified.
BROWSER: Firefox Beta 52.0
OS: Windows 10.0

Comment 13

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 53.0a1 (Build ID: 20161130030206) on 
Windows 8.1, 64-bit.

Verified as fixed with Latest Firefox beta 53.0b5 (Build ID: 20170320143328)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0