1136300 - Hello (and Share) buttons are returned to the toolbar after latest upgrade despite having previously removed them

Status: RESOLVED WONTFIX

(Hello (Loop) :: Client, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

I'm seeing reports on various parts of the web where people intentionally removed the Hello and Share buttons from their toolbar, but the latest release (Fx 36) puts them back.

Asking around in #loop, this was definitely not intentional. We should find out what happened here.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

This might be fallout from bug 1073218, which changed the ID of the Hello button from "loop-call-button" to "loop-button".

We don't have built-in support for re-id'ing buttons, so CustomizableUI would not notice that this is a rename. Instead it would assume that "loop-call-button" has been removed, and that a new built-in button called "loop-button" has been introduced, and defaulting to the toolbar.

That explains the Hello button, and I don't think there's much we can do about it at this point. :/

Not sure why the Share button might have been re-added. Looks like the button hasn't been renamed. Shane, do you have any ideas?

Comment 2

[Matthew N. [:MattN]]

[Tracking Requested - why for this release]: I can reproduce this upgrading from 35 and it's seems like a very visible change for users who removed the button and think we're no longer respecting their choice. We may want to address in a UI migration and/or relnote.

Comment 3

[Mark Banner (:standard8)]

I thought the button id was changed for a reason which was to do with a complex issue regarding the previous throttling and getting the button displayed properly. I suspect Mike can remember the issues.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #1)
> Not sure why the Share button might have been re-added. Looks like the
> button hasn't been renamed. Shane, do you have any ideas?

The only time we specifically move the button into the toolbar (if it is not already in toolbar or menu) is when a user activates a share provider.  I don't see any other reason it would have moved back, haven't renamed the id or anything.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

I don't think it is worth doing a dot release but I agree it is interesting to add an item in the release notes.
Added to the 36 release notes using "For users who removed the Share & Hello buttons, this new version brings them back unexpectedly" as wording. Don't hesitate if you have a better wording.
(I am selecting 37+ because of bug 1136611)

Comment 6

[Mike de Boer [:mikedeboer]]

For the Loop button, bug 1073218 would indeed cause this inconvenience that any prior customizations of the 'loop-call-button' will be forgotten. So that would have to be re-done by users. This will not be a problem for any subsequent release, like Fx 37.

I can work on a fix for this, if comment 5 is withdrawn in the near future.

This does not, however, provide any explanation of the Share button(s) re-appearing on the navbar. Very strange.

Comment 7

[Tyler Downer [He/Him]]

There have been several issues like this in recent releases, we should make sure to discuss this in the post-mortem to make sure this never happens again.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Mike, we are going to make a 36.0.1. If you have a safe patch, we could take it before enabling updates to 100 % users. Thanks

Comment 10

[Mike de Boer [:mikedeboer]]

Alright, I will supply a patch tomorrow morning (CET).

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: respect customizations made to the throttled Loop button and put the unthrottled button at the position of the throttled version

There's no way to keep this simple anymore, I'm afraid. I verified that this patch works for an upgrade from 35 to 36, with and without the throttled version of the Loop button.

There's no easy way to test this: you have to download and use an older version of Fx, starting with 35 - http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/35.0/ - and open each version with an empty profile.
Customize the loop button to be placed in the panel or remove it by dragging it to the palette.
Then open an Fx version with this patch applied and the profile used previously with the older Fx.
Make sure that the Loop button is placed at the correct position; either in the panel or the customization palette.

If no customizations were made with that profile, the button should appear on the navbar.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8570436 [details] [diff] [review]
Patch v1: respect customizations made to the throttled Loop button and put the unthrottled button at the position of the throttled version

Review of attachment 8570436 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sadly not going to be around for much longer today, so YMMV for a re-review. I've not tested this yet because I see issues with the patch, see below.

Also, how does this work if you migrate 35 -> 36 -> 36.0.1 ?

::: browser/components/customizableui/CustomizableUI.jsm
@@ +322,5 @@
> +      // which is the first one to appear in the navbar directly. This is
> +      // certain for this version, because the throttling of Loop/ Hello went
> +      // up to 100% before the throttling mechanism was removed.
> +      if (currentVersion == 3) {
> +        // Remove the loop-button widget from the future placements in this case.

Why? Seems like we only need to do this if the throttled version was removed?

@@ +330,5 @@
> +        }
> +        // Allow the areas to register (see CustomizableUIInternal#initialize)
> +        // before we try to fetch the placement info of the loop-button-throttled
> +        // widget.
> +        Services.tm.mainThread.dispatch(() => {

You don't need this. You should use:

this.getPlacementOfWidget(kLoopWidgetThrottled, false, true);

see the definition of getPlacementOfWidget.

This won't need the areas to be registered yet.

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #12)
> I'm sadly not going to be around for much longer today, so YMMV for a
> re-review. I've not tested this yet because I see issues with the patch, see
> below.

Thanks for the quick response!

> Also, how does this work if you migrate 35 -> 36 -> 36.0.1 ?

It doesn't, unfortunately. It's too late for ppl that already upgraded to 36 (throttled).

I'll update the patch with your suggestions.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Mike, do you have an ETA for this? Thanks

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #12)
> You don't need this. You should use:
> 
> this.getPlacementOfWidget(kLoopWidgetThrottled, false, true);
> 
> see the definition of getPlacementOfWidget.
> 
> This won't need the areas to be registered yet.

This appears to be untrue. I tried each a every variation of the boolean arguments and in each case I get NULL when the `kLoopWidgetThrottled` widget was placed in the menu-panel.
It only returns valid placement info _after_ the areas have registered.

Do you have any idea? Otherwise I'd propose the current patch to be eligible for review again.

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Mike, do you have an ETA for this? Thanks

I was planning on finishing this early today, review gods willing.

Comment 17

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #12)
> > You don't need this. You should use:
> > 
> > this.getPlacementOfWidget(kLoopWidgetThrottled, false, true);
> > 
> > see the definition of getPlacementOfWidget.
> > 
> > This won't need the areas to be registered yet.
> 
> This appears to be untrue. I tried each a every variation of the boolean
> arguments and in each case I get NULL when the `kLoopWidgetThrottled` widget
> was placed in the menu-panel.
> It only returns valid placement info _after_ the areas have registered.
> 
> Do you have any idea? Otherwise I'd propose the current patch to be eligible
> for review again.

Oh ugh. I thought loadSavedState updated gPlacements; it doesn't. You should just loop over gSavedState.placements manually instead (make sure to nullcheck gSavedState and gSavedState.placements). See the implementation of getPlacementOfWidget for how; you can do this considerably simpler than that code because you know what you're looking for and whether you care about unregistered areas/widgets (ie the two bool arguments).

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #17)
> Oh ugh. I thought loadSavedState updated gPlacements; it doesn't. You should
> just loop over gSavedState.placements manually instead (make sure to
> nullcheck gSavedState and gSavedState.placements). See the implementation of
> getPlacementOfWidget for how; you can do this considerably simpler than that
> code because you know what you're looking for and whether you care about
> unregistered areas/widgets (ie the two bool arguments).

So this means that `removeWidgetFromArea` also won't work... So what should I do there? Thanks for thinking out loud here with me ;)

Comment 19

[Mike de Boer [:mikedeboer]]

Another problem is that I need to move the widget to its 'old' position via gFuturePlacements (since gPlacements doesn't exist yet), but then it'll lose the position and will simply be appended to the area.

Comment 20

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> Another problem is that I need to move the widget to its 'old' position via
> gFuturePlacements (since gPlacements doesn't exist yet), but then it'll lose
> the position and will simply be appended to the area.

I'm confused. I thought the only case we cared about here is users who remove the widget?

If the 35->36->36.1 path doesn't work, though, I don't know how useful this is. I would predict that the majority of users upset about this change will already be on 36, and this seems like a lot of hacky work for little gain at that point. We should have just never done all this ID-switching in the first place. :-\

If we're convinced we want to do this, and we care about having it somewhere else than in the nav-bar, then we should probably s/oldID/newID/ in nsBrowserGlue.js.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Gijs, Mike, I would like to build 36.0.1 today. Do you have news/lead on this?
Otherwise, I might have to build without it.

Comment 23

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #20)
> (In reply to Mike de Boer [:mikedeboer] from comment #19)
> I'm confused. I thought the only case we cared about here is users who
> remove the widget?

Well, that's undefined. If we want to fix this full-circle, we also need to take the moved-to-other-customizable-area scenario.

> If the 35->36->36.1 path doesn't work, though, I don't know how useful this
> is. I would predict that the majority of users upset about this change will
> already be on 36, and this seems like a lot of hacky work for little gain at
> that point. We should have just never done all this ID-switching in the
> first place. :-\

That I concur with, but it couldn't be helped due to the hard requirement for a throttling mechanism that was reset for each Fx release.
To be clear, I also concur that fixing this would be a lot of hacky work to get right. I'm tilting toward WONTFIX because of this.

> If we're convinced we want to do this, and we care about having it somewhere
> else than in the nav-bar, then we should probably s/oldID/newID/ in
> nsBrowserGlue.js.

Well, I went about doing this, but there's no fun way due to the state being stored as JSON... a simple string replace is not possible due to this fact. Apart from that, I'd like to leave the door open for a future implementation of a generic CustomizableUI cleanup mechanism that co-exists with the widget introduction mechanism that we have currently.
In other words: have a `_cleanupObsoleteWidgets` next to `_introduceNewBuiltinWidgets`. `_cleanupObsoleteWidgets` would be invoked _before_ `loadSavedState` which will call `saveSt
()` if necessary and registers one-shot cleanup functions that are invoked by `serializerHelper()`. The cleanup function would look like this:

```js
function cleanupOldLoopWidgets(aKey, aValue) {
  // If the key is an Array index...
  if (typeof aKey == "number") {
    // ... and the value is 'loop-button-throttled', replace it with 'loop-button'.
    if (aValue == "loop-button-throttled") {
      return "loop-button";
    } else if (aValue == "loop-call-button") {
      // ... but the 'loop-call-button' is _really_ obsolete. `serializerHelper` will return `undefined`
      // when the cleanup function returns `false`.
      return false;
    }
  }
  // Nothing to do, keep 'em all.
  return true;
}
```

Just a thought.

Comment 24

[Mike de Boer [:mikedeboer]]

Per discussion on IRC with Gijs, Sylvestre and Blair: WONTFIX.

Comment 25

[Anthony Ricaud (:rik)]

I got this issue today on Nightly. I moved the Hello button from the address bar to the hamburger menu yesterday. Only today have I updated to 2015-03-12 and it moved back to the address bar.

I tried to reproduce by moving it again in the hamburger menu and updating to 2015-03-13 but it didn't reproduce.

Comment 26

[Mike de Boer [:mikedeboer]]

Same issue, I'm afraid, that we decided to WONTFIX.

Comment 27

[Antonio Rodríguez]

Sincerely, I'm startled. A well known issue, that is reproducing consistently every time a user updates his/her browser (I have removed half a dozen buttons because of the update to version 36.1!), and you decide not to fix it? How is this different from Google pushing its apps and services all over the user's devices and browsers? What are Mozilla Foundation's values? Have they changed overnight?

Comment 28

[Sylvestre Ledru [:Sylvestre]]

(In reply to Mike de Boer [:mikedeboer] from comment #26)
> Same issue, I'm afraid, that we decided to WONTFIX.
Seriously? To me, the issue was just the 35 => 36 update, not for all update scenari. 

I really think we should do something about that. If not, we should at least block the possibility to move the Hello and share buttons. Otherwise, users will complain at every update!

Comment 29

[Mark Banner (:standard8)]

(In reply to Antonio Rodríguez from comment #27)
> Sincerely, I'm startled. A well known issue, that is reproducing
> consistently every time a user updates his/her browser (I have removed half
> a dozen buttons because of the update to version 36.1!), and you decide not
> to fix it?

This issue should only affects users going from Firefox 35 to newer versions. Updates after 36 should _not_ be affected. For example 36 to 36.0.1 or 36.0.1 to the not-yet-released 37 will _not_ be affected.

If you think you are seeing an issue with the Hello button reappearing for updates after 36, please file a new bug. It is unlikely to be a result of this issue here, but of something else.

> How is this different from Google pushing its apps and services
> all over the user's devices and browsers? What are Mozilla Foundation's
> values? Have they changed overnight?

This bug was wontfixed due to its complexity and risk to a release, not because of any other reason.

Comment 30

[Shane Caraveo (:mixedpuppy)]

I  just tested the share button going from 36->36.0.1, unfortunately forgot to move the hello button.  In any case, the share button did not get placed back into the toolbar.

Comment 31

[Antonio Rodríguez]

Yesterday was the last time I removed a Hello button. This one was in a friend's computer that had just upgraded form 36 to 36.0.1. And yes, I had already removed it in the same computer when it upgraded from 35 to 36. I wish they had paid me each time I removed one of those buttons!

IMHO, it's absurd to pretend it's a different bug just because it happens in a different point release of the browser with the same exact symptoms.

I understand the difficulty on fixing this. I'm a software architect, and I'm responsible of a line of programs that sell about a couple hundred licenses a year, so I have some experience planing and implementing fixes for issues similar to this one. I also understand that, because of the large installed base, an hour of my work can save tens or hundreds of hours to my customers, if it allows each user to save five minutes. Firefox has an installed base of millions of users. Do the math.

Comment 32

[Mike de Boer [:mikedeboer]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #28)
> (In reply to Mike de Boer [:mikedeboer] from comment #26)
> Seriously? To me, the issue was just the 35 => 36 update, not for all update
> scenari. 
> 
> I really think we should do something about that. If not, we should at least
> block the possibility to move the Hello and share buttons. Otherwise, users
> will complain at every update!

Comment 25 is about updating Nightly --> Nightly. I'm very reluctant to start supporting Nightly profiles to this extent.
There is _not_ an issue for other update scenarios than 35 => 36, because there was no Loop button to speak of before then. Please rest assured, there will _not_ be other users who'll complain after each and every update!

(In reply to Antonio Rodríguez from comment #27)
> Sincerely, I'm startled. A well known issue, that is reproducing
> consistently every time a user updates his/her browser (I have removed half
> a dozen buttons because of the update to version 36.1!),

You have removed half a dozen buttons in 36.0.1? I don't see how that's possible, as we've never introduced that many in a single version since Firefox 1.0.

> and you decide not
> to fix it? How is this different from Google pushing its apps and services
> all over the user's devices and browsers?

This issue is not at all about pushing things to users that they can not change! In fact, you are - and always will be - able to customize Firefox to match your style. Without the Hello button, if that's your preference.
This issue exists not due to a marketing or PR related decision, but is merely due - how dull - to technical constraints that were introduced during the early roll-out of Firefox Hello: in order to be certain that our servers could handle the load, we made sure to enable Hello to a percentage of Firefox users that we controlled. Just like a opening a water tap gradually to see if the newly fitted drain doesn't leak.
We were super careful, so at first the Hello button only appeared in the palette, which made it discoverable to users who frequently customize their browser _or_ people who were really looking for Hello. In Firefox 35 we moved forward to move the Hello button to a more prominent position - the navbar - but again in a throttled fashion. We also had to be very careful not to break other peoples' customizations here upon upgrading.
For Firefox 36 we were confident enough that the throttling code was not necessary anymore, thus we removed that mechanism. An unfortunate, entirely technical-related side-effect was that the identifier of the Hello button needed to change, so the button re-appeared in the navbar for users who upgraded from 35 to 36.
I guess you could point a big fat finger at me for being the one to introduce that regression, but I'm quite confident that the explanation I just presented is quite reasonable.

> What are Mozilla Foundation's values? Have they changed overnight?

I hope this question is obsolete now. But, if you're still in doubt: no, they haven't! It is in fact _because_ of these values that we're working on this in the open.

Comment 33

[Antonio Rodríguez]

> You have removed half a dozen buttons in 36.0.1? I don't see how that's
> possible, as we've never introduced that many in a single version since
> Firefox 1.0.

I have removed buttons on half a dozen browsers on different computers that I manage (most of them are owned not by me, but by non-tech coworkers and friends). One button on each computer, but half a dozen different browsers. All of this while trying to convince those people that Firefox is a better browser than Chrome, which is the "cool" one.

I consider myself a Firefox evangelist. It's a difficult task, and problems like this make my task even more difficult. I bet I'm not the only one in this case.

It doesn't matter at all what *I* think. If people start suspecting that this is a payback to Telefónica of Spain because of their support to Firefox OS (pressing the misplaced Hello button shows a big Movistar logo), and that thought extends, it will damage Firefox's image, and will make a lot more difficult for me to replace Chrome with Firefox in the computers I manage. You can explain all of this to me and I can understand it, but try to explain it to all those non-tech people...

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Filing bug 1143081 to follow up on the issue in Nightly, reported in Comment 25. 

I think that this button reappearing in 36 was unintentional.  We should make sure it's not happening in Nightly. Let's confirm that it is or isn't happening, in bug 1143081. Thanks.