1311279 - [e10s] Can not scroll the drop-down list with click-and-drag

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[blinky]

Steps to reproduce:

1. Go to https://dl.dropboxusercontent.com/u/95157096/85f61cf7/t1o2xfgmes.html.
2. Click-and-hold on the <select> element, and drag down the drop-down list.


Actual results:

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

Comment 1

[Neil Deakin]

Attachment: Part 1 - add a chrome-only setCapture method that can ignore the allowed state

This is needed as we need to capture the mouse after the event has been sent to the child process and the popup has opened, and the normal setCapture only works during mousedown.

Comment 2

[Neil Deakin]

Attachment: Part 2 - scroll with click+drag on select

The non-e10s select uses a model where scrolling occurs when moving the mouse outside the top and bottom edge of the popup, however it responds only to mousemove events, so stops scrolling when you aren't moving the mouse. I don't think that seems quite right and makes the scrolling a bit jumpy, so I included a timer to scroll while outside the top and edge whether moving or not.

Native Windows dropdowns appear to use a model where the scroll speed is dependent on the mouse pointer distance from the popup, but I don't think it is worth adding extra code to emulate that detail.

Comment 3

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

Attachment: Test case

Moving the test mentioned in comment 0 from Dropbox to Bugzilla.

Comment 4

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

Comment on attachment 8811235 [details] [diff] [review]
Part 2 - scroll with click+drag on select

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

Thanks for the great test too! This looks okay to me (modulo notes below).

::: browser/base/content/test/general/browser_selectpopup.js
@@ +425,5 @@
> +  // Check if a drag-select works and scrolls the list.
> +  yield openSelectPopup(selectPopup, "mousedown", "select", win);
> +
> +  let scrollPos = selectPopup.scrollBox.scrollTop;
> +  let popupRect = selectPopup.getBoundingClientRect()

Nit: missing semicolon

::: toolkit/modules/SelectParentHelper.jsm
@@ +65,5 @@
>      menupopup.setConstraintRect(constraintRect);
>      menupopup.openPopupAtScreenRect("after_start", rect.left, rect.top, rect.width, rect.height, false, false);
> +
> +    // Set up for dragging
> +    menupopup.setCaptureAlways();

I'm not 100% privy to how nsIPresShell content capturing works, but is it necessary to undo this setCaptureAlways in popuphidden?

@@ +76,5 @@
>        menulist.menupopup.hidePopup();
>      }
>    },
>  
> +  clearScrollTimer: function() {

The user can close the popup without mouseup'ing (example, pressing Esc during a click-hold). Should we also clear this timer on popuphidden?

@@ +102,5 @@
>          break;
>  
> +      case "mousemove":
> +        let menupopup = currentMenulist.menupopup;
> +        let popuprect = menupopup.getOuterScreenRect();

Nit: "popuprect" -> "popupRect".

Comment 5

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba0d328831864d9302b01da2db11a85713df9a1
Bug 1311279, add a chrome-only setCapture method that can ignore the allowed state, r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/09093d38540e133c7d47061e8ab65b2a9b251758
Bug 1311279, scroll the select popup when click+drag is used, r=mconley

Comment 6

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a925f48bcc829d19b1c9f35d15c5a5f03b23b91c for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39364546&repo=mozilla-inbound

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7572a92928
Backed out changeset 09093d38540e 
https://hg.mozilla.org/integration/mozilla-inbound/rev/04c9c3dfa5ed
Backed out changeset 1ba0d3288318

Comment 8

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23ec12a251ee2bcfdd0082961497a2b7399b209d
Bug 1311279, add a chrome-only setCapture method that can ignore the allowed state, r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/aba885aafee0210b1f063a423b1c0b3ce28d15f7
Bug 1311279, scroll the select popup when click+drag is used, r=mconley

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/23ec12a251ee
https://hg.mozilla.org/mozilla-central/rev/aba885aafee0

Comment 10

[areya]

https://hg.mozilla.org/mozilla-central/rev/23ec12a251ee
https://hg.mozilla.org/mozilla-central/rev/aba885aafee0

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

backed out part 2 for intermittent failure from bug 1272834:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fcd8f50264be9c64aa85ac39097808f5bd59b30

Comment 12

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d65312d09c3

Comment 13

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f37a79affb5c

Comment 14

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a623c8e4e5c

Comment 15

[Neil Deakin]

Attachment: Part 2 - scroll with click+drag on select, v2

This is just an updated version of the same patch.

Comment 16

[Neil Deakin]

Attachment: Part 3 - additional patch to handle click event

This patch checks if the mouse was released over the select button area (in the mouseup event handler) and indicates in the message sent to the child process that mouseup/click events should be sent to the select element.

Most of the changes in SelectContentHelper.jsm are refactoring the event dispatch to a separate function dispatchMouseEvent. The real change is in "Forms:MouseUp" handler.

An additional check is added to the mousemove handler to cancel the drag processing if the mouse button is not pressed. This can happen if the mouse wasn
t used to open the dropdown or in some cases the timing of the events/messages between the processes can cause the mouseup to occur while the popup is opening and confuse things. The check isn't ideal but handles this well enough.

The test doesn't actually verify this change; but maybe one day BrowserTestUtils.synthesizeMouse will route through the parent process event handling and it will test this.

Comment 17

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e90b75db5165

Comment 18

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d467b7a7a496

Comment 19

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

Comment on attachment 8824234 [details] [diff] [review]
Part 3 - additional patch to handle click event

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

Looks okay to me (though I haven't reviewed part 2, which this seems to rely on). Thanks!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/442d408c09b3
scroll the select popup when click+drag is used, r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a03c87a1725
when the mouse is released, check if it should be retargetted at the select element in the content process, so that a click event is received, r=mconley

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/442d408c09b3
https://hg.mozilla.org/mozilla-central/rev/5a03c87a1725