Closed Bug 1221500 Opened 9 years ago Closed 9 years ago

Warn Firefox 44 users panorama is going away

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 45

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(2 files)

Spinning this off from bug 1221050.

This will need late string changes on 44 aurora if we go the notification route. :-(
Bug 1221500 - use a notification to warn people that panorama is going away, r?ttaubert, a?sylvestre
Attachment #8683044 - Flags: review?(ttaubert)
Attachment #8683044 - Flags: review?(sledru)
Comment on attachment 8683044 [details]
Bug 1221500 - use a notification to warn people that panorama is going away, , a?sylvestre

Hi Sylvestre! A little hiccup from mozreview there, but, this is a string patch that I would like to land on aurora.

Rationale:

We are going to remove tab groups / panorama.

We would like to warn users about this well in advance.

I would like to do the removal in 45. 45 is an ESR release and delaying one more release will mean any backports to 45 will be frustrated by the tab groups tests and feature being present. It's a relatively big feature to remove that has its tentacles everywhere in frontend code, and a few places besides that.

Philipp argued in bug 1221050 that we should warn people that this is coming the release before. That means doing something in 44 to warn users. The proposed way to do this would be to show a desktop notification. Which needs strings.

The one other option we have is to not show a desktop notification but to always just open a new tab with the relevant SUMO page. The downside of this is that tab groups users are more likely than ordinary users to have multiple windows, which creates the risk that such an extra tab would be missed. This would, however, avoid the extra strings going into aurora.

Approval Request Comment
[Feature/regressing bug #]: tab groups removal
[User impact if declined]: we'd have to use a suboptimal method of letting people know the feature is going away
[Describe test coverage new/current, TreeHerder]: no
[Risks and why]: pretty low, aurora is only just cut, we can tweak the detection of tab groups usage if necessary (without further l10n impact)
[String/UUID change made/needed]: yes, 2 strings added
Attachment #8683044 - Flags: review?(sledru) → approval-mozilla-aurora?
Flags: needinfo?(sledru)
Comment on attachment 8683044 [details]
Bug 1221500 - use a notification to warn people that panorama is going away, , a?sylvestre

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from bug 1221050 comment #19)
> Created attachment 8683061 [details]
> tab groups warning.png
> 
> (In reply to Dão Gottwald [:dao] from comment #17)
> > (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> > comment #16)
> > > - Show a desktop notification in the version *prior* to the one that removes
> > > tab groups saying something like »Tab groups will be removed from Firefox in
> > > a few weeks. Click to learn more.«
> > 
> > Why a desktop notification? I believe our standard UI for this kind of
> > warning would a more permanent notification bar.
> 
> We should consider desktop notifications for these kinds of prompts in the
> future. They have different properties in terms of information density and
> persistence.
> 
> That said, upon further reflection, they are probably not ready to perform
> that job well in Firefox 44 yet. Given that, I agree with you that a
> notificaiton bar would be the best option. Let's make it permanent (not tab
> specific), placed at the bottom of the window and use the yellow alert color:
> 
> Heads up! Tab groups will be removed from Firefox soon. [Learn more]
> 
> Matej, what do you think of that copy? It would only be shown to users who
> have more than one tab group open.

--> needinfo Matej.

> If possible, we could also show a warning inside the tab groups UI like in
> the attachment.

Do we really have to add even more strings to 44? We're already breaking the rules here.
Flags: needinfo?(sledru) → needinfo?(matej)
Attachment #8683044 - Flags: review?(ttaubert)
Attachment #8683044 - Flags: approval-mozilla-aurora?
Attachment #8683044 - Flags: review?(ttaubert)
Attachment #8683044 - Flags: review?(sledru)
Comment on attachment 8683044 [details]
Bug 1221500 - use a notification to warn people that panorama is going away, , a?sylvestre

Bug 1221500 - use a notification to warn people that panorama is going away, r?ttaubert, a?sylvestre
Attachment #8683044 - Flags: review?(sledru) → approval-mozilla-aurora?
(In reply to :Gijs Kruitbosch from comment #3)
> > If possible, we could also show a warning inside the tab groups UI like in
> > the attachment.
> 
> Do we really have to add even more strings to 44? We're already breaking the
> rules here.

I've added this, using the same strings, and using a button rather than a link, and with the same styling as the existing banner there (that tells you we've enabled session restore for you if you start using tab groups). I don't think it's worth making that banner look pretty, though if you feel very strongly the current styling is horrible then we can deal with that in a followup.
(In reply to :Gijs Kruitbosch from comment #3)
> 
> > Heads up! Tab groups will be removed from Firefox soon. [Learn more]
> > 
> > Matej, what do you think of that copy? It would only be shown to users who
> > have more than one tab group open.
> 
> --> needinfo Matej.

I think this looks OK, though it should probably be "Tab Groups" with a capital G.

P.S. It makes me very sad to work on this string. I use Tab Groups all the time!
Flags: needinfo?(matej)
This will need to go in to the 38.x (38.6.0?) ESR release as well, won't it?
(In reply to Oliver Henshaw from comment #7)
> This will need to go in to the 38.x (38.6.0?) ESR release as well, won't it?

We can't add new strings on ESR release, so it's a no on my side of things.
Attachment #8683044 - Flags: review?(ttaubert) → review+
Comment on attachment 8683044 [details]
Bug 1221500 - use a notification to warn people that panorama is going away, , a?sylvestre

https://reviewboard.mozilla.org/r/24185/#review21833

::: browser/components/nsBrowserGlue.js:1332
(Diff revision 2)
> +        if (win.TabView._tabBrowserHasHiddenTabs()) {

As gBrowser.hideTab() is probably used by other add-ons as well, maybe check TabView.firstUseExperienced too? These two conditions do not reliably tell whether the user just peeked once and then never used it again, or stopped using it, or still uses it, but I think that's the best we can do.

::: browser/components/nsBrowserGlue.js:1381
(Diff revision 2)
> +    let clickCallback = (subject, topic, data) => {
> +      // This callback will be called twice but only once with this topic
> +      if (topic != "alertclickcallback")
> +        return;
> +      let win = RecentWindow.getMostRecentBrowserWindow();
> +      win.openUILinkIn(data, "tab");
> +    };

This seems unused?

::: browser/components/tabview/ui.js:514
(Diff revision 2)
> +    this.notifyPanoramaDeprecation();

Nit: We don't call it Panorama anywhere in here, iirc. Why not just notifyDeprecation() or notifyAboutDeprecation()? Not that it matters a lot anymore...
Sylvestre, can I get approval to preland the strings from this patch on aurora? I don't really want to show the notification until we have a decent SUMO page, and from bug 1221497 it seems that for that it'd be useful to know which add-ons work, for which more work needs to be done, which is going to take time. The issue here is the l10n thing which really needs to go in ASAP if it is going to get there at all, so please can you review that for approval so as to take some of the time pressure off? :-)
Flags: needinfo?(sledru)
Why are we not just landing this on central and the feature removal in six weeks? As much as I support that this is finally happening, do we gain anything by taking a six weeks shortcut?
(In reply to Dão Gottwald [:dao] from comment #11)
> Why are we not just landing this on central and the feature removal in six
> weeks? As much as I support that this is finally happening, do we gain
> anything by taking a six weeks shortcut?

45 is esr, and backporting pretty much anything after that is going to be hell.
You mean conflicts in code outside of browser/components/tabview/ using the TabView API? I think those should be in the same ballpark as with other random code changes happening in any given six week cycle. "Hell" sounds a bit exaggerated.
(In reply to Dão Gottwald [:dao] from comment #13)
> You mean conflicts in code outside of browser/components/tabview/ using the
> TabView API? I think those should be in the same ballpark as with other
> random code changes happening in any given six week cycle. "Hell" sounds a
> bit exaggerated.

That's the smaller part of the problem. My reason for saying hell is the following: if I write a patch for Firefox 46, and then uplift to 45 ESR, and touch anything related to tabs, docshell loading behaviour / beforeunload, or tabmodal dialogs, or stuff like that, I'm liable to break some of the tabview tests because they touch "interesting" parts of how we deal with tabs and tabmodal dialogs. The tests wouldn't be there for 46, but would be for 45, and so patches would bounce off ESR (and if we ignore the tests, they would actually break something that we would then still be shipping).

Bug 967873, for instance, has already had to jump through hoops because of tabview. It's too late for that bug now, but considering the focus on e10s and its performance, and some of the issues we've had with tabmodal onbeforeunload, I'm sure that there will be other cases once tabview is actually gone.

Really, all I'm asking for is to uplift 3 strings in the first week of aurora. Doesn't seem like the end of the world to me, considering the trade-offs.
Redirecting to Ritu as she owns 44.
Flags: needinfo?(sledru) → needinfo?(rkothari)
https://hg.mozilla.org/mozilla-central/rev/96f1230a34d6
https://hg.mozilla.org/mozilla-central/rev/bfb7ef931971
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Blocks: 1222786
Pike, heads up this patch here has some string changes that need to be included in Aurora44. I am leaning towards taking it. I know it is not ideal that this came after the merge but it seems like a good idea to include in Aurora44. Please let me know if you have any concerns.
Flags: needinfo?(rkothari) → needinfo?(l10n)
Comment on attachment 8683044 [details]
Bug 1221500 - use a notification to warn people that panorama is going away, , a?sylvestre

This has been in Nightly for a few days and it seems like a good idea despite the string changes to warn our users of upcoming changes. This kind of stuff helps our end-users be more aware of changes that might be coming along the way. Let's uplift to Aurora44.
Attachment #8683044 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(l10n)
Depends on: 1223947
Depends on: 1224635
FF 44.0 in Windows; the colored banner message obscures the "Tabs from other windows" display, and removes my ability to click those search result tabs.  And I was looking for either a way to dismiss the overlayed message, i.e. an X button to make it go away one time; or else a preference setting to disable it.  The main difficulty for me is that I can no longer click on those "Tabs from other windows" results.  Attaching a representative .jpg.  Thanks; Larry
Partial window grab, showing the Heads up message obscuring and preventing access to Tabs from other windows search result.
I neglected to mention that at first, I wondered if clicking the Learn More button would dismiss the banner in addition to opening the link in a new tab.  Unfortunately it does not; but that would be one fairly smooth option for dismissing the banner on the current Tab Groups window.  Thanks
(In reply to Larry from comment #27)
> I neglected to mention that at first, I wondered if clicking the Learn More
> button would dismiss the banner in addition to opening the link in a new
> tab.  Unfortunately it does not; but that would be one fairly smooth option
> for dismissing the banner on the current Tab Groups window.  Thanks

Hey Larry,

The issue here is related to how the tab groups code absolutely positions *everything*. I tried to make sure that this overlap wouldn't occur by making the total height of the search container (which is in fact overlapping the entire tab groups window) smaller. For some reason, that change doesn't seem to be working on your machine. I just re-checked on mine, and it works here. I don't know why.

Normally, we would investigate something like this (clearly there's something I'm missing!) and then fix it. But because tab groups are going away, that's no use - all future versions of Firefox no longer have any of this code. We're not going to release a 44.0.1 just for this issue. I'm really sorry, but this isn't going to get fixed. 

The best way to deal with this for you (it's clearly an issue!) would be to install https://addons.mozilla.org/en-US/firefox/addon/tab-groups-panorama/ .

This will get rid of the warning message, and will also ensure you can keep using tab groups in 45 and later, when it will have been removed from Firefox proper. I hope that helps.
I will do that (panorama addon).  I understand what you're saying about positioning and overlap.  (As a background wish, I would like some way in future, other situations to be have a way to dismiss one of these, in the event that it is interfering with something.)  No problem, I am ok.
Thanks Gijs, for your helpful and quick response; Larry
Product: Firefox → Firefox Graveyard
Attachment #8683044 - Attachment description: MozReview Request: Bug 1221500 - use a notification to warn people that panorama is going away, r?ttaubert, a?sylvestre → Bug 1221500 - use a notification to warn people that panorama is going away, , a?sylvestre
You need to log in before you can comment on or make changes to this bug.