Closed Bug 1397501 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Menus, enhancement, P1)

57 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: abenson, Assigned: adw)

References

()

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 file)

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.
Flags: needinfo?(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.
Flags: needinfo?(adw) → needinfo?(abenson)
> 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.
Flags: needinfo?(abenson) → needinfo?(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?
Flags: needinfo?(adw) → needinfo?(abenson)
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.
Flags: needinfo?(abenson)
Thanks.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Summary: URL bar icon placement do not follow specifications → Page actions added to the urlbar should always come before the bookmark star
(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.
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure] → [reserve-photon-structure]
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 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?
Attachment #8905715 - Flags: review?(gijskruitbosch+bugs) → review+
Maybe it's worth getting Aaron to confirm that the behaviour is correct with the build from the trypush?
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.
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.)
Flags: needinfo?(abenson)
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.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f4df3c6e697
Page actions added to the urlbar should always come before the bookmark star. r=Gijs
Cool, it looks great to me anyway! Thanks Drew!
Flags: needinfo?(abenson)
https://hg.mozilla.org/mozilla-central/rev/1f4df3c6e697
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1398375
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.