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

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: blinky, Assigned: Neil Deakin)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla53
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified, firefox53 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 months ago
str
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.
(Reporter)

Updated

5 months ago
Blocks: 1311279
(Reporter)

Updated

5 months ago
Blocks: 1154677
(Assignee)

Comment 1

5 months ago
Created attachment 8817470 [details] [diff] [review]
Drag when pressing on menuitem
Assignee: nobody → enndeakin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

4 months ago
Attachment #8817470 - Attachment is obsolete: true
(Assignee)

Comment 2

4 months ago
Created attachment 8825797 [details] [diff] [review]
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.
Attachment #8825797 - Flags: review?(bugs)
(Assignee)

Comment 3

4 months ago
Created attachment 8825798 [details] [diff] [review]
Part 2 - allow drag-scrolling the select popup when the mouse is pressed on an option in the list
Attachment #8825798 - Flags: review?(mconley)
(Assignee)

Comment 4

4 months ago
Created 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
Attachment #8825799 - Flags: review?(mconley)
(Assignee)

Comment 5

4 months ago
Created attachment 8825800 [details] [diff] [review]
Part 4 - menulist test
Attachment #8825800 - Flags: review?(mconley)

Comment 6

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

I guess we can do this. Popups effectively prevent capturing.
Attachment #8825797 - Flags: review?(bugs) → review+
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...
Attachment #8825798 - Flags: review?(mconley) → review+
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.
Attachment #8825799 - Flags: review?(mconley) → review+
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.
Attachment #8825800 - Flags: review?(mconley) → review+

Comment 10

3 months ago
Pushed by neil@mozilla.com:
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

3 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

3 months ago
Depends on: 1332708
Depends on: 1341031
QA Whiteboard: [good first verify]

Comment 12

a month ago
[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
status-firefox52: --- → verified
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
QA Whiteboard: [good first verify] → [good first verify][bugday-20170322]
You need to log in before you can comment on or make changes to this bug.