Closed Bug 1136300 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
2

Tracking

(firefox36+ wontfix, relnote-firefox 36+)

RESOLVED WONTFIX
Iteration:
39.1 - 9 Mar
Tracking Status
firefox36 + wontfix
relnote-firefox --- 36+

People

(Reporter: mconley, Assigned: mikedeboer)

References

Details

Attachments

(1 file)

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.
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?
Flags: needinfo?(mixedpuppy)
[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.
Hardware: x86 → All
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.
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mixedpuppy)
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)
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.
Flags: needinfo?(mdeboer)
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.
Duplicate of this bug: 1137271
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
Flags: needinfo?(mdeboer)
Alright, I will supply a patch tomorrow morning (CET).
Flags: needinfo?(mdeboer)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Points: --- → 2
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.
Attachment #8570436 - Flags: review?(mconley)
Attachment #8570436 - Flags: review?(gijskruitbosch+bugs)
OS: Mac OS X → All
Depends on: 1073215
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.
Attachment #8570436 - Flags: review?(gijskruitbosch+bugs) → review-
(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.
Mike, do you have an ETA for this? Thanks
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(mdeboer)
(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).
Flags: needinfo?(gijskruitbosch+bugs)
(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 ;)
Flags: needinfo?(gijskruitbosch+bugs)
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.
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: 1137084
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.
Flags: needinfo?(mdeboer)
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(mdeboer)
Per discussion on IRC with Gijs, Sylvestre and Blair: WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Same issue, I'm afraid, that we decided to WONTFIX.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → WONTFIX
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?
(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!
(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.
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.
(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.
> 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...
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.
You need to log in before you can comment on or make changes to this bug.