1393343 - Move "Restore Previous Session" Menu Item from Library>History to the Menu.

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[bbell]

About:home is undergoing a total renovation, and it has become necessary to finally find a more logical place to hang the "Restore Previous Session" button.

Move the "Restore Previous Session" Menu Item from Library>History to the Menu.  

See Mockup:  https://mozilla.invisionapp.com/share/5ED6FOEYR#/250127153_Explainer_-_Menu_-_Previous_Session

In addition to this move, include some logic that detects if the user has the Auto Restore Previous Session option turned on, and if so, hide the "Restore Previous Session" item from the Application Menu.

Comment 2

[bbell]

Attachment: restoresession-16.svg

Icon Needed

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1393343 - move 'restore previous session' item into main menu and hide when auto-restore is enabled,

Review commit: https://reviewboard.mozilla.org/r/172618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172618/

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8901145 [details]
Bug 1393343 - move 'restore previous session' item into main menu and hide when auto-restore is enabled,

https://reviewboard.mozilla.org/r/172618/#review177986

Looks good to me! Thanks, Gijs.

::: browser/base/content/browser.js:8301
(Diff revision 1)
>          gBrowser.tabContainer.addEventListener("TabOpen", this);
>        }
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
> +    } else {
> +      let ss = Cc["@mozilla.org/browser/sessionstartup;1"].getService(Ci.nsISessionStartup);

Since this is the second use of this service, can you add it to the list of lazyServiceGetters above? I'd call it 'SessionStartup'.

::: browser/base/content/browser.js:8303
(Diff revision 1)
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
> +    } else {
> +      let ss = Cc["@mozilla.org/browser/sessionstartup;1"].getService(Ci.nsISessionStartup);
> +      if (ss.isAutomaticRestoreEnabled()) {
> +        document.getElementById("Browser:RestoreLastSession").setAttribute("hidden", true);

Oooh, so hiding the command also hides all associated XUL elements? TIL.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8901145 [details]
Bug 1393343 - move 'restore previous session' item into main menu and hide when auto-restore is enabled,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172618/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38a6312f4f8c
move 'restore previous session' item into main menu and hide when auto-restore is enabled, r=mikedeboer

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/38a6312f4f8c

Comment 8

[Md. Majedul islam]

I have reproduced this Bug with Nightly 57.0a1 (2017-08-24) on Ubuntu 16.04, 64 Bit!

The bug's fix is now verified on latest Nightly 57.0a1

Build ID     20170905100117
User Agent   Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 9

[Mohammad Maruf Rahman]

I have reproduced this Bug with Nightly 57.0a1 (2017-08-23) on Windows 7, 64 Bit!

The bug's fix is verified on latest Nightly!

Build ID     20170905100117
User Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170906]

Comment 10

[Md. Majedul islam]

As per Comment 8 and Comment 9, I am marking this bug as verified fixed.

Comment 11

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

Looking at the spec, it looks like we're supposed to be offering the session restoration by showing a panel with _only_ the restoration menuitem until the user dismisses it with another action. Did we intentionally drop that part?

Comment 12

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #11)
> Looking at the spec, it looks like we're supposed to be offering the session
> restoration by showing a panel with _only_ the restoration menuitem until
> the user dismisses it with another action. Did we intentionally drop that
> part?

This wasn't in comment #0 and I totally missed that there was a second part to the mockup (if there was at the time - I don't know, invision changes all the time). It was below the fold, and the stuff above the fold seemed to match comment #0 perfectly...

I also don't really understand how it relates to the other parts of this. Like, when do we show this notification (I assume we still show the other stuff though it's omitted from the mockup, but we show the notification with the highlighted background at the top of the menu, instead of in its usual place, or something? Which seems to be different from what you're assuming based on the mockup...) as opposed to just showing it as part of the menu? Or is it a separate panel? Does it pop open automatically on startup? What does "returning to Firefox" mean in this context?

All of this is a question for Bryan.

Comment 13

[bbell]

> This wasn't in comment #0 and I totally missed that there was a second part
> to the mockup (if there was at the time - I don't know, invision changes all
> the time)

Sorry about the Gijs. I should have probably called that out. I was just so happy to get the feature in I too forgot about that detail. 

> I also don't really understand how it relates to the other parts of this.
> Like, when do we show this notification (I assume we still show the other
> stuff though it's omitted from the mockup, but we show the notification with
> the highlighted background at the top of the menu, instead of in its usual
> place, or something? Which seems to be different from what you're assuming
> based on the mockup...) as opposed to just showing it as part of the menu?
> Or is it a separate panel? Does it pop open automatically on startup? What
> does "returning to Firefox" mean in this context?

To complete the flow there are some details that need to be worked out, around sequensing messages. Like does this come before an update reminder.  Truthfully I mentally punted on this restore notification because I was told that The Strategy and Insights team was supposedly doing some Contextual Feature Recommender, it's a tool that prompts the user to try or do a thing at the right time, and I figured they would take care of this final bit around prompting the user to restore a session. 

As a side note I would be in favor of turning Restore Session on by default, and then we don't have to prompt the user on every cold start.

Comment 14

[bbell]

(In reply to :Gijs from comment #12)
> Like, when do we show this notification 

The door hanger that offers a user the option the restore the previous session would fire off on every restart.

Comment 15

[:Gijs (he/him)]

Jeff, do you know who can make a decision about the notification-style "hey, wanna restore your session" and whether that's important to have for 57? Mike is concerned that no longer offering the yuge button on about:home wants more of a replacement than the more-subdued item in the hamburger panel alone (which isn't super discoverable if people don't go there), for those users who don't restore their session automatically (like most of us, I guess).

I assume someone in product or user research has a handle on this stuff, also with background from bug 1219725 etc. - I just don't know who. If we still want to do more for 57 we will need to move fast (and obviously are more constrained about strings etc).

Comment 16

[:Gijs (he/him)]

(In reply to bbell from comment #14)
> (In reply to :Gijs from comment #12)
> > Like, when do we show this notification 
> 
> The door hanger that offers a user the option the restore the previous
> session would fire off on every restart.

That seems pretty aggressive - a button on a page is easier to ignore than a doorhanger...

Comment 17

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Redirecting to Maria, who is PM for AS / newtab.

Comment 18

[Maria Popova]

The Doorhanger, which helped communicate to the user that they have a session that they can restore, is an unrequired enhancement for 57.

Comment 19

[Peter Dolanjski [:pdol]]

Just talked to Bryan.  We also agreed that we don't need to act in 57 and our shield study (bug 1400942) should give us a better understand if something obvious in the browser chrome is used or not.  Let's wait for that analysis to decide how to execute the latter piece of this spec.