Closed Bug 1321472 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

53 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- verified
firefox53 --- fixed

People

(Reporter: over68, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

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.
Blocks: 1311279
Blocks: e10s-select
Attached patch Drag when pressing on menuitem (obsolete) — Splinter Review
Assignee: nobody → enndeakin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8817470 - Attachment is obsolete: true
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)
Attachment #8825800 - Flags: review?(mconley)
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+
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
Depends on: 1341031
QA Whiteboard: [good first verify]
[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
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.