962069 - Dragging an item on to the toolbar overflow chevron should open the overflow popup

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Place enough items in the navigation toolbar and resize the window so that items are overflowed and the chevron appears.
Drag a link from a webpage to the overflow chevron.

ER:
The chevron should open and show the overflowed items so the dragged item can be dropped on an overflowed item.

AR:
Nothing happens (circle with line through it showing that the item cannot be dropped there).

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8363002 [details] [diff] [review]
Patch

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +3177,5 @@
> +      case "customizationstarting":
> +        this._disable();
> +        break;
> +      case "dragover":
> +        this.show();

It looks like this never hides the panel. That doesn't seem right. When the drag leaves the panel and chevron for more than ~500ms, we should hide the panel again.

::: browser/components/customizableui/test/browser_962069_drag_to_overflow_chevron.js
@@ +18,5 @@
> +  let widgetOverflowPanel = document.getElementById("widget-overflow");
> +  let panelShownPromise = promisePanelElementShown(window, widgetOverflowPanel);
> +  let identityBox = document.getElementById("identity-box");
> +  let overflowChevron = document.getElementById("nav-bar-overflow-button");
> +  ChromeUtils.synthesizeDrop(identityBox, overflowChevron, [], null);

Equally, after a drop, I'd expect the panel to be hidden already.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8409163 [details] [diff] [review]
Patch v2

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

Jared, thanks for working on this - looking good!

One problem I noticed is that after performing the following steps...

1. Drag a link on the overflow chevron, which opens the overflow panel.
2. Drag it out of the overflow popup and let it go to hide the overflow panel.
3. Click on the overflow chevron to open the panel again
4. Click on the overflow chevron again to hide it.

... the overflow button stays in the [open] state.

I *think* this has something to do with you setting `this._chevron.open` on a timed hiding of the panel.

Can you upload a patch with this issue fixed? I think that'd be all then.

One other thing/ question: do you always need the `dragover` and `dragend` listeners on the overflow panel? If not, is possible to add them upon chevron dragover?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

<jaws> mikedeboer: hey, do you think you could do a short screencast showing that bug you found with the patch in bug 962069? i don't think i have the right timing to reproduce it
<mikedeboer> jaws: I can't reproduce it anymore! :/
<jaws> mikedeboer: heh, well that makes me feel better because i couldn't either!

However I did notice that the panel would hide if the mouse stopped for over the timeout duration (500ms) while on top of the panel. This is now fixed.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8426384 [details] [diff] [review]
Patch v3

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

Apart from a few minor things, this looks very good to me.

Thanks for the test!

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +4096,5 @@
> +    this._hideTimeoutId = window.setTimeout(() => {
> +      if (!this._panel.firstChild.mozMatchesSelector(":hover")) {
> +        this._panel.hidePopup();
> +      }
> +    }, 500);

please put this timeout value in a constant at the top of this file.

::: browser/components/customizableui/test/browser_962069_drag_to_overflow_chevron.js
@@ +6,5 @@
> +
> +let originalWindowWidth;
> +
> +// Drag to overflow chevron should open the overflow panel.
> +add_task(function() {

nit: can you land this with an asterisk?

@@ +30,5 @@
> +  EventUtils.synthesizeKey("VK_ESCAPE", {});
> +  yield panelHiddenPromise;
> +});
> +
> +add_task(function() {

nit: same here.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3 (nits fixed)

Thanks, nits addressed and carrying forward r+

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please run this through Try first :)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=f20c57e75667

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3.1

My try push showed that after the nits fixed the event listeners weren't being added correctly. Tested locally and pushed to tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=aa38f77b69d5

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Review ping?

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8426777 [details] [diff] [review]
Patch v3.1

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +4096,5 @@
> +      window.clearTimeout(this._hideTimeoutId);
> +      this._hideTimeoutId = null;
> +    }
> +    this._hideTimeoutId = window.setTimeout(() => {
> +      if (!this._panel.firstChild.mozMatchesSelector(":hover")) {

if this is true, shouldn't this re-trigger the delayed-hiding of the panel?
IOW, try again later?

Comment 13

[Mike de Boer [:mikedeboer]]

Jared, sorry for noticing your ping earlier... I already signed off for the day :/

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8426777 [details] [diff] [review]
Patch v3.1

After pondering over my previous comment, I conclude that the behavior as currently implemented makes more sense and is preferable over retriggering the timeout.

Comment 15

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

https://hg.mozilla.org/integration/fx-team/rev/8132875a4515

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/8132875a4515

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.3; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0

Got the expected result on latest Nightly, build ID: 20140528030219.
Marking issue verified.