779054 - SocialAPI sidebar is added to chromehidden=[menubar,toolbar,directories] window on session restore

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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] [:mhammond]]

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: [e-mail]]]

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] [:mhammond]]

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] [:mhammond]]

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: [e-mail]]]

(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]

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)]

Attachment: 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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6134705b1a11

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6134705b1a11

Comment 10

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcda58d59a8d