The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

17 Branch
Firefox 18
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [Fx17])

Attachments

(2 attachments)

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.
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.

Updated

5 years ago
Depends on: 779729
tracking-firefox17: --- → ?
tracking-firefox17: ? → +
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
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.
Assignee: gavin.sharp → jaws
Status: NEW → ASSIGNED
Attachment #667812 - Flags: review?(felipc)
Attachment #667812 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6134705b1a11
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/6134705b1a11
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
No longer depends on: 779729
Attachment #667812 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/dcda58d59a8d
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.