1395154 - If Send Page to Device pageAction is Pinned, it should dismiss when a user sends a tab

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[John Gruen [:jgruen]]

Attachment: pa-no-dismiss.mov

Attached a demo video

STR
1. Pin the Send Page to Device pageAction
2. Send to device

Expected
Dialog dismisses and i see confirmation UI

Actual
Dialog stays in place and confirmation appears behind

Comment 1

[Stephen Horlander [:shorlander]]

Probably a structure bug.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1395154 - If Send Page to Device pageAction is Pinned, it should dismiss when a user sends a tab.

Review commit: https://reviewboard.mozilla.org/r/175062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175062/

Comment 3

[Drew Willcoxon :adw]

The problem is that gSync always assumes the panel that contains the Send to Device view is the main page action panel.

This is based on the patch in bug 1395398 and includes a test.

Comment 4

[Drew Willcoxon :adw]

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

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8903268 [details]
Bug 1395154 - If Send Page to Device pageAction is Pinned, it should dismiss when a user sends a tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175062/diff/1-2/

Comment 6

[Drew Willcoxon :adw]

eslint fix.

Comment 7

[Drew Willcoxon :adw]

Try looks OK.  There was one "test exceeded the timeout threshold" for this test, but there's bug 1384464 for that, and I'd like to fix that there by splitting the test up.

There were also a bunch of en-US failures, but in an unrelated test, so I'm guessing I happened to pull a bad tree?

Comment 8

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #7)
> There were also a bunch of en-US failures, but in an unrelated test, so I'm
> guessing I happened to pull a bad tree?

Lots of them happening on latest m-c changeset too, over on treeherder.

Comment 9

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

Comment on attachment 8903268 [details]
Bug 1395154 - If Send Page to Device pageAction is Pinned, it should dismiss when a user sends a tab.

https://reviewboard.mozilla.org/r/175062/#review180158

::: browser/base/content/browser-pageActions.js:810
(Diff revision 2)
> +    let panelNode = panelViewNode.parentNode;
> +    while (panelNode && panelNode.localName != "panel") {
> +      panelNode = panelNode.parentNode;
> +    }

This can be replaced with `let panelNode = panelViewNode.closest("panel");`

::: browser/base/content/test/urlbar/browser_page_action_menu.js:496
(Diff revision 2)
> +    let panelPromise =
> +      promisePanelShown(BrowserPageActions._activatedActionPanelID);
> +    EventUtils.synthesizeMouseAtCenter(urlbarButton, {});
> +    await panelPromise;
> +
> +    await new Promise(r => setTimeout(r, PANEL_VIEW_TIMEOUT));

Instead of the setTimeout, can you use waitForCondition() and just check to see if an attribute has been set? See note about this for head.js.

The other way would be to implement `checkSendToDeviceItems` to return true or false if it finds the items (and only assert if an extra argument is passed in), and then use `waitForCondition(checkSendToDeviceItems)`.

::: browser/base/content/test/urlbar/browser_page_action_menu.js:540
(Diff revision 2)
> +    // And then the "Sent!" notification panel should open and close by itself
> +    // after a moment.
> +    info("Waiting for the Sent! notification panel to open");
> +    await promisePanelShown(BrowserPageActionFeedback.panelNode.id);

Thanks for writing this test!

::: browser/base/content/test/urlbar/head.js:14
(Diff revision 2)
> +// Intermittently, when the page action panel shows a subview, it can take a
> +// moment for subview to appear and the panel to resize.  So we wait this number
> +// of ms after showing a subview to avoid intermittent timeouts.
> +const PANEL_VIEW_TIMEOUT = 5000;

After the panel contents have been filled in, can we set some attribute to mark that the work is done, and then our test code can use `await waitForCondition(() => element.hasAttribute(...))` ?

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8903268 [details]
Bug 1395154 - If Send Page to Device pageAction is Pinned, it should dismiss when a user sends a tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175062/diff/2-3/

Comment 11

[Drew Willcoxon :adw]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> This can be replaced with `let panelNode = panelViewNode.closest("panel");`

Done

> ::: browser/base/content/test/urlbar/browser_page_action_menu.js:496
> (Diff revision 2)
> > +    let panelPromise =
> > +      promisePanelShown(BrowserPageActions._activatedActionPanelID);
> > +    EventUtils.synthesizeMouseAtCenter(urlbarButton, {});
> > +    await panelPromise;
> > +
> > +    await new Promise(r => setTimeout(r, PANEL_VIEW_TIMEOUT));
> 
> Instead of the setTimeout, can you use waitForCondition() and just check to
> see if an attribute has been set? See note about this for head.js.

This timeout isn't waiting in order to make checkSendToDeviceItems pass, it's waiting for the synthesized mouse click to work.  Without waiting, nothing happens on the click, like the button isn't fully visible.

The problem seems to be the panel's opening animation.  So what I did instead was add a way for tests to disable it.  That seems to have fixed it.  Before I tried that though, I tried waiting until the clicked menu item's bounds to become non-zero, but that didn't work.  And it doesn't seem to be necessary with the animation disabled, at least for me locally.  But I left that in because it seems like a good idea anyway.

> ::: browser/base/content/test/urlbar/head.js:14
> (Diff revision 2)
> > +// Intermittently, when the page action panel shows a subview, it can take a
> > +// moment for subview to appear and the panel to resize.  So we wait this number
> > +// of ms after showing a subview to avoid intermittent timeouts.
> > +const PANEL_VIEW_TIMEOUT = 5000;
> 
> After the panel contents have been filled in, can we set some attribute to
> mark that the work is done, and then our test code can use `await
> waitForCondition(() => element.hasAttribute(...))` ?

To elaborate, the panel gets filled in synchronously, so I don't think setting an attribute would solve the problem.

Comment 12

[Drew Willcoxon :adw]

BTW there are test tasks that open the main page action panel and click buttons, and they work fine, so I don't know what the deal is in this case.

Comment 13

[Drew Willcoxon :adw]

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

Comment 14

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

Comment on attachment 8903268 [details]
Bug 1395154 - If Send Page to Device pageAction is Pinned, it should dismiss when a user sends a tab.

https://reviewboard.mozilla.org/r/175062/#review180518

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3357426af984
If Send Page to Device pageAction is Pinned, it should dismiss when a user sends a tab. r=jaws

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/3357426af984

Comment 17

[Kanchan Kumari QA]

This is to verify that this issue has been fixed on the latest nightly.