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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jgruen, Assigned: adw)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments)

Posted video 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
Whiteboard: [photon-animation]
Probably a structure bug.
Flags: needinfo?(adw)
Whiteboard: [photon-animation] → [photon-structure]
Flags: qe-verify?
Whiteboard: [photon-structure] → [photon-structure] [triage]
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure] [triage] → [reserve-photon-structure]
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Iteration: --- → 57.3 - Sep 19
Priority: P3 → P1
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.
eslint fix.
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?
(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 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(...))` ?
(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.
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 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
Attachment #8903268 - Flags: review?(jaws) → review+
Pushed by dwillcoxon@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/3357426af984
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
This is to verify that this issue has been fixed on the latest nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.