Closed Bug 1454705 Opened 6 years ago Closed 6 years ago

Can't use share menu from within the address bar

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: daleharvey)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

(In reply to Madhava Enros [:madhava] from bug 1363168 comment #54)
> When I right click on share in the menu and "Add to Address Bar," the
> resulting spot in the Address Bar has no icon.
> 
> Screenshot: https://screenshots.firefox.com/mRrGL6UlmV5is1Pm/null

I can reproduce this. The new entry also doesn't work. Sam, can you take a look?
Flags: needinfo?(sfoster)
(In reply to :Gijs (he/him) from comment #0)
> I can reproduce this. The new entry also doesn't work. Sam, can you take a
> look?

Err, I meant Dale, of course...
Flags: needinfo?(sfoster) → needinfo?(dharvey)
Will take a look
Assignee: nobody → dharvey
Flags: needinfo?(dharvey)
Comment on attachment 8968913 [details]
Bug 1454705 - Fix display of share panel in Address bar.

https://reviewboard.mozilla.org/r/237636/#review243426

Some issues in the test but generally this looks fine, so r=me . I haven't tested this, I assume you have. :-)

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:71
(Diff revision 1)
> +    // Add the Share button to the Address bar
> +
> +    // Open pageAction panel
> +    await promisePageActionPanelOpen();

Would it be easy to make this its own task with its own name?

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:103
(Diff revision 1)
> +    viewPromise = promisePageActionPanelShown();
> +    EventUtils.synthesizeMouseAtCenter(shareButton, {});
> +
> +    // Wait for panel to be shown
> +    await BrowserTestUtils.waitForCondition(() => {
> +      return !BrowserPageActions.mainViewBodyNode.hidden;

This doesn't seem right... what do other tests do? I think there's a helper method around to check for the size of at least 1 item in the menu to be non-0 or something, to help deal with transitions etc. Paolo may also have a better idea.

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:110
(Diff revision 1)
> +    // Remove the Share URL button from the Address bar so we dont interfere
> +    // with future tests
> +    await promisePageActionPanelOpen();

I don't understand why we are opening the panel here. We can remove the item from the url bar item's own context menu directly, right? :-)

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:123
(Diff revision 1)
> +      button: 2,
> +    });
> +    await contextMenuPromise;
> +
> +    contextMenuPromise = promisePanelHidden("pageActionContextMenu");
> +    EventUtils.synthesizeMouseAtCenter(collectContextMenuItems()[0], {});

This seems like it could just use querySelector, e.g. 

```js
document.querySelector("#pageActionContextMenu .pageActionContextMenuItem.builtInPinned")
```

or something like that. Same above where we add the item, of course. Or we could just give the context menu items IDs...
Attachment #8968913 - Flags: review?(gijskruitbosch+bugs) → review+
Priority: -- → P2
Whiteboard: [fxsearch]
Comment on attachment 8968913 [details]
Bug 1454705 - Fix display of share panel in Address bar.

https://reviewboard.mozilla.org/r/237636/#review243634

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:71
(Diff revision 1)
> +    // Add the Share button to the Address bar
> +
> +    // Open pageAction panel
> +    await promisePageActionPanelOpen();

Yup, done

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:103
(Diff revision 1)
> +    viewPromise = promisePageActionPanelShown();
> +    EventUtils.synthesizeMouseAtCenter(shareButton, {});
> +
> +    // Wait for panel to be shown
> +    await BrowserTestUtils.waitForCondition(() => {
> +      return !BrowserPageActions.mainViewBodyNode.hidden;

I couldnt find other tests that have a panel opened from the address bar, send to device ones particular, but did realise that promisePageActionViewShown does work here, it just doesnt give me the same view to count the share receivers but thats fine since I do that directly, so switched to promisePageActionViewShown

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:110
(Diff revision 1)
> +    // Remove the Share URL button from the Address bar so we dont interfere
> +    // with future tests
> +    await promisePageActionPanelOpen();

I didnt realise it could be removed directly, done now

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:123
(Diff revision 1)
> +      button: 2,
> +    });
> +    await contextMenuPromise;
> +
> +    contextMenuPromise = promisePanelHidden("pageActionContextMenu");
> +    EventUtils.synthesizeMouseAtCenter(collectContextMenuItems()[0], {});

Switched
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64cb5a4a986e
Fix display of share panel in Address bar. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/64cb5a4a986e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.