Last Comment Bug 779054 - SocialAPI sidebar is added to chromehidden=[menubar,toolbar,directories] window on session restore
: SocialAPI sidebar is added to chromehidden=[menubar,toolbar,directories] wind...
Status: RESOLVED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 23:13 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2013-12-27 14:25 PST (History)
7 users (show)
jaws: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Screenshot of bug (333.12 KB, image/png)
2012-07-30 23:13 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details
Patch (1.09 KB, patch)
2012-10-03 22:37 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-07-30 23:13:23 PDT
Created attachment 647451 [details]
Screenshot of bug

With the Social API enabled, I opened an image on an article in the nytimes.com website. The image opened in a popup window. I left the window open and continued to use the browser until it later crashed (seems unrelated).

Upon restarting the browser, it went into session restore and the popup now contained the social api sidebar.
Comment 1 Mark Hammond [:markh] 2012-08-01 00:21:11 PDT
I can reproduce this by showing some page with a tag such as:

 <a href="#" onclick="window.open('http://www.google.com', 'foo', 'dialog');">click for google</a>

Clicking on the tag opens the new window without chrome and life is good.  Using the DOM inspector, the window object has the following attribute:

  chromehidden="menubar toolbar directories extrachrome "

Now, keep the window open, restart firefox and restore the session.  The window reopens and the sidebar is shown in it.  However, use the DOM inspector now and the attribute is:

  chromehidden="menubar toolbar directories "

Note the missing 'extrachrome' - and this is the specific string value that browser-social.js is looking for - the sidebar is shown because it is missing.  I can't see an existing bug for this attribute missing on session restore.  A work-around might be to skip the sidebar when there is a non-empty chromehidden attribute, but I'm not familiar enough with things to know if that is feasible.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-01 06:47:10 PDT
See bug 454047 comment 2 - we'll probably need a fix similar to bug 495495's, making SessionStore.jsm's restoreWindowFeatures aware of the social sidebar (perhaps by just having it call SocialSidebar.updateSidebar(), or otherwise cause it to be called).
Comment 3 Mark Hammond [:markh] 2012-08-01 17:20:05 PDT
Having session restore be aware of the sidebar would be a workaround for the problem that the chromehidden attribute is wrong after the restore.  It seems just having the chromehidden attribute be correct after a restore would be even better :)  Note that having session restore simply call SocialSidebar.updateSidebar() would not work - it *is* being called now, it is just that the incorrect chromehidden attribute is causing it to be shown.  Note that the incorrect chromehidden attribute isn't just a timing issue - the value *never* gets restored correctly.
Comment 4 Mark Hammond [:markh] 2012-08-01 22:39:01 PDT
Just to clarify, if the patch below is applied things work as expected.  Hopefully this demonstrates that the problem isn't that "chromehidden" hasn't been applied to the window yet, just that the "extrachrome" value doesn't exist on restored windows.

--- a/browser/base/content/browser-social.js
+++ b/browser/base/content/browser-social.js
@@ -393,17 +393,17 @@ var SocialSidebar = {
     return Social.uiVisible && Social.provider.sidebarURL && !this.chromeless;
   },

   // Whether this is a "chromeless window" (e.g. popup window). We don't show
   // the sidebar in these windows.
   get chromeless() {
     let docElem = document.documentElement;
     return docElem.getAttribute('disablechrome') ||
-           docElem.getAttribute('chromehidden').indexOf("extrachrome") >= 0;
+           docElem.getAttribute('chromehidden').length != 0;
   },
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 01:01:20 PDT
(In reply to Mark Hammond (:markh) from comment #3)
> Note that the incorrect chromehidden attribute isn't just a timing issue -
> the value *never* gets restored correctly.

Sounds like a sessionstore bug, then.
Comment 6 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-28 16:12:38 PDT
Assigning to Gavin now that we're about a week away from merge of 17 to Beta, it needs to get assigned to someone.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-10-03 22:37:32 PDT
Created attachment 667812 [details] [diff] [review]
Patch

Bug 779729 doesn't look like it will get a proper fix in time for the 17 uplift to beta. This patch instead checks for chromehidden='toolbar' which can also be used to check for popups.

In fact, it is very useful, since we don't want the sidebar to show in a place where the social provider toolbar buttons are visible.
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-10-04 00:01:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6134705b1a11
Comment 9 Ed Morley [:emorley] 2012-10-04 08:54:07 PDT
https://hg.mozilla.org/mozilla-central/rev/6134705b1a11
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-05 07:38:44 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/dcda58d59a8d

Note You need to log in before you can comment on or make changes to this bug.