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

RESOLVED FIXED in Firefox 17

Status

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

17 Branch
Firefox 18
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [Fx17])

Attachments

(2 attachments)

Posted image 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.
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.
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).
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.
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;
   },
Whiteboard: [Fx17]
(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.
Depends on: 779729
Assigning to Gavin now that we're about a week away from merge of 17 to Beta, it needs to get assigned to someone.
Assignee: nobody → gavin.sharp
Posted patch PatchSplinter Review
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.
Assignee: gavin.sharp → jaws
Status: NEW → ASSIGNED
Attachment #667812 - Flags: review?(felipc)
Attachment #667812 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/6134705b1a11
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
No longer depends on: 779729
Attachment #667812 - Flags: approval-mozilla-aurora+

Updated

4 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.