Closed Bug 1248780 Opened 9 years ago Closed 9 years ago

The Pocket toolbar icon moved when it was switched to a system addon

Categories

(Firefox :: Pocket, defect)

Unspecified
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- affected

People

(Reporter: RyanVM, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(2 files)

[Tracking Requested - why for this release]: Regression from the recent Pocket system addon work.

I noticed while bisecting bug 1243806 that the location of the Pocket icon on the toolbar changed when bug 1215694 landed (at least on Linux). It used to be adjacent to the bookmarks button, but is now all the way to the right just left of the customize button.
Flags: needinfo?(mixedpuppy)
I haven't been able to reproduce, in fact the button is keeping the same location for me even when customizing.  If I can get an STR that would be useful.  Here's what I did:

created new profile called testing
run 44, see icon next to bookmark btn
run 47, icon is in correct location
run 44, move pocket icon to another place (not at end)
run 47, icon is in correct location
run 44, move pocket to menu panel
run 47, icon is correct
run 44, move pocket to palette
run 47, icon is correct
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy) → needinfo?(ryanvm)
All of my tests were using mozregression, which creates a new profile every time (i.e. no migration from 44 first).
Flags: needinfo?(ryanvm)
ha!  That's it, I was testing upgrade but not simply starting with a new profile.

str:
create new profile
start fx46 using new profile
This is pretty complex because CUI makes a lack of placement mean the placement is the palette.  On a fresh profile I don't have a way to differentiate between the user having placed the widget into the palette or the widget simply never having been positioned in the UI.  I thought I had this working but perhaps that was not correct, or perhaps some timing changed in CUI that obsoleted what I did. 

a) not a release blocker.  Current users will get the right position.  New users wont know the difference.
b) the fix should really be in CUI rather than the addon

Gijs, what do you think?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> This is pretty complex because CUI makes a lack of placement mean the
> placement is the palette.  On a fresh profile I don't have a way to
> differentiate between the user having placed the widget into the palette or
> the widget simply never having been positioned in the UI.  I thought I had
> this working but perhaps that was not correct, or perhaps some timing
> changed in CUI that obsoleted what I did. 
> 
> a) not a release blocker.  Current users will get the right position.  New
> users wont know the difference.
> b) the fix should really be in CUI rather than the addon

Why? Add-on icons going at the end of the toolbar has been standard procedure since ~ Firefox 1.0. How would CUI know what to do here?

> Gijs, what do you think?

I think this is fixable. We try to determine the placement of the widget here:

  let seenWidget = CustomizableUI.getPlacementOfWidget("pocket-button", false, true);

https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/browser/extensions/pocket/bootstrap.js#141-143

and then do this:

https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/browser/extensions/pocket/bootstrap.js#174-180

  // placed is null if location is palette
  let placed = CustomizableUI.getPlacementOfWidget("pocket-button");

  // a first time install will always have placed the button somewhere, and will
  // not have a placement prior to creating the widget. Thus, !seenWidget &&
  // placed.
  if (reason == ADDON_ENABLE && !seenWidget && placed) {
    // initially place the button after the bookmarks button if it is in the UI
    let widgets = CustomizableUI.getWidgetIdsInArea(CustomizableUI.AREA_NAVBAR);
    let bmbtn = widgets.indexOf("bookmarks-menu-button");
    if (bmbtn > -1) {
      CustomizableUI.moveWidgetWithinArea("pocket-button", bmbtn + 1);
    }


This should be working, AIUI. If you add some logging it sounds like it would be trivially possible to figure out why it isn't working. Maybe the move isn't correct? Maybe seenWidget is true after all? Maybe placed is still false because the code runs too early, or something? Either way, probably fixable with some investigation.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> > This is pretty complex because CUI makes a lack of placement mean the
> > placement is the palette.  On a fresh profile I don't have a way to
> > differentiate between the user having placed the widget into the palette or
> > the widget simply never having been positioned in the UI.  I thought I had
> > this working but perhaps that was not correct, or perhaps some timing
> > changed in CUI that obsoleted what I did. 
> > 
> > a) not a release blocker.  Current users will get the right position.  New
> > users wont know the difference.
> > b) the fix should really be in CUI rather than the addon
> 
> Why? Add-on icons going at the end of the toolbar has been standard
> procedure since ~ Firefox 1.0. How would CUI know what to do here?

Being an addon is an implementation detail here.  This is a Firefox feature where there was a specific decision by someone that the icon should be placed next to the bookmark button.

> > Gijs, what do you think?
> 
> I think this is fixable. We try to determine the placement of the widget
> here:
> 

> This should be working, AIUI. If you add some logging it sounds like it
> would be trivially possible to figure out why it isn't working. Maybe the
> move isn't correct? Maybe seenWidget is true after all? Maybe placed is
> still false because the code runs too early, or something? Either way,
> probably fixable with some investigation.

That section of code is specifically ADDON_ENABLED which is never the case when the addon is already enabled on startup.  You get something like APP_STARTUP instead.  Essentially that code will never run with the addon used as a system addon, but will for out-of-band updates.

Aside from that, on startup !seenWidget is always true and placed is always null.  So there may be some timing issue there.

I'm sure it's fixable, but it would be much easier on future system addons if the api had a way to specific desired placement.

On the other hand, I personally dont care about the initial placement.  If you don't know Pocket, having it next to bookmarks is an incredibly subtle indicator of what the button does.  If you do, placement doesn't matter.
(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> > > This is pretty complex because CUI makes a lack of placement mean the
> > > placement is the palette.  On a fresh profile I don't have a way to
> > > differentiate between the user having placed the widget into the palette or
> > > the widget simply never having been positioned in the UI.  I thought I had
> > > this working but perhaps that was not correct, or perhaps some timing
> > > changed in CUI that obsoleted what I did. 
> > > 
> > > a) not a release blocker.  Current users will get the right position.  New
> > > users wont know the difference.
> > > b) the fix should really be in CUI rather than the addon
> > 
> > Why? Add-on icons going at the end of the toolbar has been standard
> > procedure since ~ Firefox 1.0. How would CUI know what to do here?
> 
> Being an addon is an implementation detail here.

Not from CUI's perspective.

>  This is a Firefox feature
> where there was a specific decision by someone that the icon should be
> placed next to the bookmark button.

And very similar people said that add-ons should never be able to "always come back" in the same place after a reset, which is the only reason this works the way it does and add-ons are treated differently from inside-CUI widgets. There is no sensible way to determine in some other way whether or not code is "builtin" because we have no clean privilege separation for add-ons.

Anyway, all of this is covered by bug 1235627 and friends, where we're actually considering the solution to this issue. There are already too many bugs filed on this.

> That section of code is specifically ADDON_ENABLED which is never the case
> when the addon is already enabled on startup.  You get something like
> APP_STARTUP instead.  Essentially that code will never run with the addon
> used as a system addon, but will for out-of-band updates.

But no out-of-band updates are planned for pocket yet? Anyway, that sounds like that code just needs changing.

> Aside from that, on startup !seenWidget is always true and placed is always
> null.  So there may be some timing issue there.
> 
> I'm sure it's fixable, but it would be much easier on future system addons
> if the api had a way to specific desired placement.

Fine; do propose an API.

> On the other hand, I personally dont care about the initial placement.  If
> you don't know Pocket, having it next to bookmarks is an incredibly subtle
> indicator of what the button does.  If you do, placement doesn't matter.

The same folks who you earlier said made a "specific decision" would disagree, no?
Regression from 46, tracking. 
Shane what's the word on this? Are you aiming to fix this for 46 or 47? 
=
Flags: needinfo?(mixedpuppy)
Keywords: regression
I'll update later today if fixing for 46 is necessary.
Flags: needinfo?(mixedpuppy)
It was decided in earlier meetings (Firefox PM and discussion with Pocket) that it is not necessary to have the button in a specific location.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: