sidebar open state not remembered

VERIFIED FIXED in Firefox 31

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 33
x86
Mac OS X
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 8445424 [details] [diff] [review]
fix sidebar state when users do not turn on session restore

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)
(Assignee)

Comment 2

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

Comment 3

5 years ago
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+
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

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

Comment 7

5 years ago
Created attachment 8446096 [details]
fix sidebar state when users do not turn on session restore

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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
(Assignee)

Updated

5 years ago
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → fixed
(Assignee)

Comment 9

5 years ago
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+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2be42467a16

A patch with a minor mod for beta is upcoming.
status-firefox30: affected → wontfix
status-firefox32: affected → fixed
(Assignee)

Comment 11

5 years ago
after discussing with felipe, going forward without the minor mod previously mentioned.

https://hg.mozilla.org/releases/mozilla-beta/rev/1bced5624188
status-firefox31: affected → fixed
Keywords: verifyme
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
status-firefox33: fixed → 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.
status-firefox31: fixed → verified
status-firefox32: fixed → verified
Keywords: verifyme
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-
You need to log in before you can comment on or make changes to this bug.