Closed Bug 1311279 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

52 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: over68, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

(Keywords: multiprocess)

Attachments

(4 files, 1 obsolete file)

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.
Blocks: e10s-select
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: multiprocess
Summary: [e10s] can not scroll the drop-down list with click-and-drag → [e10s] Can not scroll the drop-down list with click-and-drag
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
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.
Attachment #8811233 - Flags: review?(bugs)
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.
Attachment #8811235 - Flags: review?(mconley)
Attached file Test case
Moving the test mentioned in comment 0 from Dropbox to Bugzilla.
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".
Attachment #8811235 - Flags: review?(mconley) → review+
Attachment #8811233 - Flags: review?(bugs) → review+
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
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
Flags: needinfo?(enndeakin)
https://hg.mozilla.org/mozilla-central/rev/23ec12a251ee
https://hg.mozilla.org/mozilla-central/rev/aba885aafee0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1320565
Depends on: 1321472
backed out part 2 for intermittent failure from bug 1272834:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fcd8f50264be9c64aa85ac39097808f5bd59b30
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is just an updated version of the same patch.
Attachment #8811235 - Attachment is obsolete: true
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.
Attachment #8824234 - Flags: review?(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!
Attachment #8824234 - Flags: review?(mconley) → review+
Pushed by neil@mozilla.com:
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
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.