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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bbell, Assigned: Gijs)

Tracking

57 Branch
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Whiteboard: [photon-structure]
Whiteboard: [photon-structure] → [photon-structure] [triage]
Assignee: gijskruitbosch+bugs → nobody
Flags: qe-verify+
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure] [triage] → [reserve-photon-structure]
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Iteration: --- → 57.2 - Aug 29
Priority: P3 → P1
(Reporter)

Comment 2

2 years ago
Icon Needed

Comment 4

2 years ago
mozreview-review
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.
Attachment #8901145 - Flags: review?(mdeboer) → review+
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Comment hidden (mozreview-request)

Comment 6

2 years ago
Pushed by gijskruitbosch@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/38a6312f4f8c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 8

2 years ago
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
QA Whiteboard: [bugday-20170906]
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]
As per Comment 8 and Comment 9, I am marking this bug as verified fixed.

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+

Updated

2 years ago
Blocks: 1400052
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?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 12

2 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bbell)
(Reporter)

Comment 13

2 years ago
> 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.
Flags: needinfo?(bbell)
(Reporter)

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
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).
Flags: needinfo?(jgriffiths)
(Assignee)

Comment 16

2 years ago
(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...
Redirecting to Maria, who is PM for AS / newtab.
Flags: needinfo?(jgriffiths) → needinfo?(mpopova)

Comment 18

2 years ago
The Doorhanger, which helped communicate to the user that they have a session that they can restore, is an unrequired enhancement for 57.
Flags: needinfo?(mpopova)
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.
You need to log in before you can comment on or make changes to this bug.