sidebar open state not remembered

VERIFIED FIXED in Firefox 31

Status

defect
VERIFIED FIXED
5 years ago
5 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 33
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(firefox30 wontfix, firefox31 verified, firefox32 verified, firefox33 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

refactoring in Bug 894806 changed to using window state for the sidebar state.  however, this is not remembered unless you change the default startup pref (on the general prefs tab) to "remember windows and tabs from last time".  Many uses do not change this pref, so the sidebar disappears on restart.
I want to push this up the train as far as it will go as it is a painful problem for users.

https://tbpl.mozilla.org/?tree=Try&rev=ce46e2b930e0
Assignee: nobody → mixedpuppy
Attachment #8445424 - Flags: review?(mhammond)
STR

- new profile
- activate a sidebar provider
- with sidebar visible, restart firefox
[alternative on osx, close last window without quiting firefox, no windows open, open new window]

EXPECT

- sidebar visible after restart

ERROR

- sidebar is not visible after restart
Comment on attachment 8445424 [details] [diff] [review]
fix sidebar state when users do not turn on session restore

just noticed markh is away...see if Felipe can take this review.
Attachment #8445424 - Flags: review?(mhammond) → review?(felipc)
Attachment #8445424 - Flags: review?(felipc) → review+
Comment on attachment 8445424 [details] [diff] [review]
fix sidebar state when users do not turn on session restore

@ttaubert, felipe suggested getting your input on the issue with the untracked hidden window comment in this patch (see the third chunk).
Attachment #8445424 - Flags: feedback?(ttaubert)
Comment on attachment 8445424 [details] [diff] [review]
fix sidebar state when users do not turn on session restore

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

::: browser/base/content/browser-social.js
@@ +89,5 @@
>    uninit: function SocialUI_uninit() {
>      if (!this._initialized) {
>        return;
>      }
> +    SocialSidebar.saveWindowState();

So why exactly do we save the state on uninit()? Can't we save the state when the sidebar is expanded/collapsed?

@@ +757,5 @@
>        if (!data.hidden)
>          this.show(data.origin);
> +    } else if (Services.prefs.prefHasUserValue("social.sidebar.provider")) {
> +      // no window state, use the global state if it is available
> +      this.show(Services.prefs.getCharPref("social.sidebar.provider"));

.getCharPref() will throw if the pref doesn't exist, no?

::: browser/base/content/test/social/browser_social_window.js
@@ +30,5 @@
>    }, topic, false);
>  }
>  
> +function closeWindow(w, cb) {
> +  waitForCondition(function() w.closed,

I think waiting for the "domwindowclosed" notification would be better.

@@ +171,5 @@
> +                });
> +              });
> +            });
> +          });
> +        });        

Nit: trailing white space
Attachment #8445424 - Flags: feedback?(ttaubert) → feedback+
(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #5)

> ::: browser/base/content/browser-social.js
> @@ +89,5 @@
> >    uninit: function SocialUI_uninit() {
> >      if (!this._initialized) {
> >        return;
> >      }
> > +    SocialSidebar.saveWindowState();
> 
> So why exactly do we save the state on uninit()? Can't we save the state
> when the sidebar is expanded/collapsed?

It is saved when opened or the provider is changed.  We also need to save on close so the "last window wins" if you have different sidebars in different windows.  That is only an issue if the user doesn't restart state on startup, otherwise each window would resume with the sidebar it contained.


> @@ +757,5 @@
> >        if (!data.hidden)
> >          this.show(data.origin);
> > +    } else if (Services.prefs.prefHasUserValue("social.sidebar.provider")) {
> > +      // no window state, use the global state if it is available
> > +      this.show(Services.prefs.getCharPref("social.sidebar.provider"));
> 
> .getCharPref() will throw if the pref doesn't exist, no?

on, it's wrapped in a prefHasUserValue check.
updated test with additional feedback.

pushed https://hg.mozilla.org/integration/fx-team/rev/d41a6d09ccd6
Attachment #8445424 - Attachment is obsolete: true
Attachment #8446096 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d41a6d09ccd6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment on attachment 8446096 [details]
fix sidebar state when users do not turn on session restore

[Approval Request Comment]
Regression caused by (bug #):  	894806
User impact if declined: the sidebar closes for users with default prefs when firefox is restarted even though they left the sidebar open.  UX to reopen is non-obvious
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8446096 - Flags: approval-mozilla-release?
Attachment #8446096 - Flags: approval-mozilla-beta?
Attachment #8446096 - Flags: approval-mozilla-aurora?
Attachment #8446096 - Flags: approval-mozilla-beta?
Attachment #8446096 - Flags: approval-mozilla-beta+
Attachment #8446096 - Flags: approval-mozilla-aurora?
Attachment #8446096 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2be42467a16

A patch with a minor mod for beta is upcoming.
after discussing with felipe, going forward without the minor mod previously mentioned.

https://hg.mozilla.org/releases/mozilla-beta/rev/1bced5624188
Verified on Nightly 33.0a1 2014-06-27

Sidebar status is preserved on restart without the user needing to toggle any preferences.

Tests Preformed: (Mac and Windows)
* Installed GOAL sidebar
* Quit Firefox
* Launch Firefox : Goal Sidebar is displayed
* Close all windows leaving firefox running (OSX)
* Open new windows : Sidebar is displayed 
* Uncheck display goal sidebar leaving the service installed
* Quit and Relaunch Firefox : Sidebar is not displayed but still installed.
* View -> Sidebar -> display Goal Sidebar
* Quit and Relaunch Firefox : is  displayed
* About:Addons -> disable sidebar
* Quit and Relaunch Firefox : Sidebar is not displayed 
* About:Addons->Remove : Sidebar is removed 
* About:Addons - enable sidebar - sidebar is NOT displayed
* Select View->Sidebar->GOAL  : Sidebar is displayed
Status: RESOLVED → VERIFIED
QA Contact: mschifer
I've verified this on Windows 7 x64, Mac OS X 10.8.5, and Ubuntu 13.04 x64, using Firefox 31 Beta 8 (BuildID: 20140707160635) and the latest Firefox 32 Aurora (BuildID: 20140709004001). I covered the same scenarios as Marc in comment 12 (including display of the sidebar after update) and I could not find any issues.
Comment on attachment 8446096 [details]
fix sidebar state when users do not turn on session restore

31 is going to ship next week. So, not taking the patch in release.
Attachment #8446096 - Flags: approval-mozilla-release? → approval-mozilla-release-
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.