1397501 - Page actions added to the urlbar should always come before the bookmark star

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement, P1)

Description

[Aaron Benson]

The attached spec defines the placement of the icons in the URL bar which were supposed to be fixed in bug 1378560

Spec here: https://mozilla.invisionapp.com/share/94CLGV8GN#/screens/243839921_Explainer_-_URL_Bar_Icon_Placement

The premise for these rules are that the Page Action Menu (•••) is all about the page, so it needs to be as close to the url as possible since there’s a relationship there. The bookmark star, when present, needs to be the right-most item because it’s our way of asserting it as the primary way of saving things. Anything else shows up between those items. We want to build a more robust customize mode for this after 57 but this spec was to provide some basic rules to follow in the interim.

Comment 1

[Drew Willcoxon :adw]

Aaron, AFAICT we're now following the spec.

(In reply to Aaron Benson from comment #0)
> The premise for these rules are that the Page Action Menu (•••) is all about
> the page, so it needs to be as close to the url as possible since there’s a
> relationship there.

There's a bunch of stuff between the ••• and the URL in the spec -- dropdown arrow, container label and icon, popup blocker button, reader button, zoom button -- and with bug 1378560 fixed, we're now following that.  Is there something you're seeing that's out of place?

> The bookmark star, when present, needs to be the right-most item

I incorrectly put the Pocket button after the bookmark star, but I fixed that in bug 1395743.  Unfortunately though if your profile had already been upgraded to Photon before that bug was fixed (i.e., a Nightly profile), Pocket will still be out of order because the ordering of those buttons is saved in a hidden pref so that we remember it across app restarts.  If you try on a new profile (and a Nightly build after that bug landed), you should see that the bookmark star now comes after the Pocket button.

Comment 2

[Aaron Benson]

> There's a bunch of stuff between the ••• and the URL in the spec -- dropdown arrow, container 
> label and icon, popup blocker button, reader button, zoom button -- and with bug 1378560 fixed, 
> we're now following that.  Is there something you're seeing that's out of place?

Yeah, the icons to the left of the ••• look correct but I'm still seeing the Copy URL, Email Page, Screenshot Page get added to the right of the bookmarks star (so, not in-between the ••• and the star). Could that be part of the profile issue as well? I just tested a brand-new profile and see things getting added to the right, including Pocket.

Comment 3

[Drew Willcoxon :adw]

Oh, even when you add actions from the menu to the urlbar, they're supposed to always go to the left of the bookmark star?  We just append them to the urlbar.  Could you describe in more detail how they should be added to the urlbar?

(In reply to Aaron Benson from comment #2)
> I just tested a brand-new profile and see things getting added to the
> right, including Pocket.

Pocket should be in the urlbar by default without your having to do anything, and it should come before the bookmark star.  Once you remove it and then add it back though, it will be appended, as I mentioned in the previous paragraph.

So it sounds like we should morph this bug to be: Page actions added to the urlbar should always come before the bookmark star.  Right?

Comment 4

[Aaron Benson]

Right. 

> Oh, even when you add actions from the menu to the urlbar, they're supposed to always go to the left 
> of the bookmark star?  We just append them to the urlbar.  Could you describe in more detail how they 
> should be added to the urlbar?

Yeah so if anything is added to the URL bar from the •••, it should show up to the immediate left of the bookmarks star. In the absense of the bookmark star, items would show up in the right-most position.

Comment 5

[Drew Willcoxon :adw]

Thanks.

Comment 6

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from bug 1395743 comment #3)
> Unfortunately though this patch won't fix the problem for people who are
> already using Nightly, since they'll have their urlbar ordering in their
> prefs already, and PageActions will continue to respect that.  We could
> version this persistent data and then bump the version for this bug, but
> it's not worth the complexity IMO, at least not right now, for this bug
> before Photon has shipped.

Between this bug and bug 1395743, I'm starting to think it might be worth it after all.  It'll probably be confusing for people who have already added actions to their urlbar if they now start appearing before the bookmark star.

Comment 7

[Drew Willcoxon :adw]

Attachment: Bug 1397501 - Page actions added to the urlbar should always come before the bookmark star.

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

Comment 8

[Drew Willcoxon :adw]

This introduces a small migration to a new version of the actions storage.  I'd like for everyone already using Photon to see the intended behavior, and I don't think that's possible otherwise.

As a consequence of this bug, we don't need _urlbarInsertBeforeActionID anymore, which was added in bug 1395743.  It was only used by Pocket, and now with this patch, Pocket will come before the bookmark star.

I took the opportunity to convert the `action ID => true` mapping that we store in prefs to an array, like you suggested in the original bug, Gijs.

This also consolidates the logic that determines where to insert IDs into _persistedActions.idsInUrlbar.  Before, that was happening in a couple of places (which was OK then because the logic was simple).

And it adds a test.

Comment 9

[Drew Willcoxon :adw]

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8905715 [details]
Bug 1397501 - Page actions added to the urlbar should always come before the bookmark star.

https://reviewboard.mozilla.org/r/177512/#review182584

This seems to work for me and the code looks great. Thanks!

::: browser/modules/PageActions.jsm
(Diff revision 1)
>    /**
>     * The list of actions in the urlbar, sorted in the order in which they should
>     * appear there.  Not live.  (array of Action objects)
>     */
>    get actionsInUrlbar() {
> -    if (!this._persistedActions) {

Was there a particular reason to remove this block here/now?

Comment 11

[:Gijs (he/him)]

Maybe it's worth getting Aaron to confirm that the behaviour is correct with the build from the trypush?

Comment 12

[Drew Willcoxon :adw]

Wow, thanks for the quick and late-at-night review, much appreciated.

(In reply to :Gijs from comment #10)
> ::: browser/modules/PageActions.jsm
> (Diff revision 1)
> >    /**
> >     * The list of actions in the urlbar, sorted in the order in which they should
> >     * appear there.  Not live.  (array of Action objects)
> >     */
> >    get actionsInUrlbar() {
> > -    if (!this._persistedActions) {
> 
> Was there a particular reason to remove this block here/now?

It's basically dead code, left over from a previous version before the initial _persistedActions value was hard-coded.  _persistedActions should never be falsey now.

Comment 13

[Drew Willcoxon :adw]

Aaron, would you mind testing this build before I land this?  Treeherder link + build artifacts here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fde66dbce40e

(Here's a Mac build: https://queue.taskcluster.net/v1/task/LoZPesyNSL2tEgNoAAhv1w/runs/0/artifacts/public/build/target.dmg  Windows 10 hasn't finished yet.  Don't know what OS you're using.)

Comment 14

[Drew Willcoxon :adw]

Hmm, on second thought I'd like to get this in ASAP since we're running out of time, so I'll land this now.  We can tweak it if need be.  I'll keep the needinfo on Aaron to check it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f4df3c6e697
Page actions added to the urlbar should always come before the bookmark star. r=Gijs

Comment 16

[Aaron Benson]

Cool, it looks great to me anyway! Thanks Drew!

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1f4df3c6e697

Comment 18

[Emil Ghitta, QA [:emilghitta]]

I can confirm that Page actions comes before the bookmark star on Firefox 57.0b7 (Build Id:20171009192146) using Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.