1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

STR:
1. Right click the bookmark star in the address bar and remove it.
2. Open the page actions menu.
3. Click the bookmark action.

The tooltip open now on the very left of the addressbar. Instead it should behave like to tooltip of the "ellipsis" button, or as a submenu like the "Send tab to device" action.

I can reproduce in 58, but most likely it affects 57 too.

Comment 1

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

One more note, this behavior is not the case for Pocket, which also opens a popup when clicked. But even this I would expect to behave like a submenu.

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible.

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

Comment 3

[Drew Willcoxon :adw]

There's no reason not to call BrowserPageActions.panelAnchorNodeForAction() to get the right anchor I think.

Re: the removal of the "Set the open=true attribute" part, AFAICT we now do not ever pass in a node other than the appropriate page action anchor.  That bit of code was added in https://hg.mozilla.org/mozilla-central/rev/b3ea0d746ee4 for Australis I'm guessing.

Comment 4

[Drew Willcoxon :adw]

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

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8911903 [details]
Bug 1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible.

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

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8911903 [details]
Bug 1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible.

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

Comment 7

[Drew Willcoxon :adw]

Set the "open" attribute only if the anchor is in the urlbar (i.e., a page action button or the site identity icon, which also styles the open attribute).  We fall back to showing the popup over the current browser if there is no anchor, and we probably shouldn't set/remove open on it.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8911903 [details]
Bug 1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible.

https://reviewboard.mozilla.org/r/183292/#review188470

r=me, though note that I'm a little confused about null-checking `action` here. Where do we not pass in an action?

::: browser/base/content/browser-pageActions.js:295
(Diff revision 1)
>              return node;
>            }
>          }
>        }
>      }
>      throw new Error(`PageActions: No anchor node for '${action.id}'`);

If `action` is really falsy (you're nullchecking it above) then this will throw while creating the error string...

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8911903 [details]
Bug 1402721 - Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible.

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

Comment 10

[Drew Willcoxon :adw]

Updated for the one `action` use I missed, thanks.

We don't pass in a falsey action, but since the fallback anchors (main page action button, site identity box) don't depend on the given action, it seems reasonable to continue to fall back to them if the action is falsey.  I can imagine using this method in the future for actions that aren't necessarily currently registered, maybe... although that would be weird probably.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86a27018d5bc
Add/edit bookmark panel should open anchored on the page action (ellipsis) button instead of on the identity block (left side of the address bar) when the bookmarks star action is not pinned/visible. r=Gijs

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/86a27018d5bc

Comment 13

[Drew Willcoxon :adw]

Attachment: Beta 57 patch

Approval Request Comment
[Feature/Bug causing the regression]:
It's not a regression, just a polish bug for the page actions feature introduced in Photon in 57.

[User impact if declined]:
When the bookmark star is removed from the urlbar, the edit-bookmark popup will be anchored on the site identity box.  It should be anchored on the "..." page action button.

[Is this code covered by automated tests?]:
Yes, but there's no automated test specifcally for this bug (anchoring the popup to the "..." button vs. the site identity box).

[Has the fix been verified in Nightly?]:
No

[Needs manual test from QE? If yes, steps to reproduce]:
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
It's a small polish fix.

[String changes made/needed]:
None

Comment 14

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

I can verify this to be working Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20170926100259.

Comment 15

[u554753]

Also verified on 58.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912074 [details] [diff] [review]
Beta 57 patch

Polish photon, taking it.
Should be in 57b4, gtb tomorrow Thursday

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f094ee5cc7b3

Comment 18

[Emil Ghitta, QA [:emilghitta]]

I have reproduced the issue mentioned in comment 0 using an affected Firefox 58.0a1 build (BuildId:20170924220116).

I have verified that the issue is not reproducible using Firefox 57.0b7 (Build Id:20171009192146) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.