Closed
Bug 779054
Opened 11 years ago
Closed 11 years ago
SocialAPI sidebar is added to chromehidden=[menubar,toolbar,directories] window on session restore
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(firefox17+ fixed)
RESOLVED
FIXED
Firefox 18
People
(Reporter: jaws, Assigned: jaws)
Details
(Whiteboard: [Fx17])
Attachments
(2 files)
333.12 KB,
image/png
|
Details | |
1.09 KB,
patch
|
Felipe
:
review+
Gavin
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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; },
Updated•11 years ago
|
Whiteboard: [Fx17]
Comment 5•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
tracking-firefox17:
--- → ?
Updated•11 years ago
|
Comment 6•11 years ago
|
||
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
Assignee | ||
Comment 7•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #667812 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6134705b1a11
Flags: in-testsuite-
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6134705b1a11
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Updated•11 years ago
|
Attachment #667812 -
Flags: approval-mozilla-aurora+
Comment 10•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/dcda58d59a8d
status-firefox17:
--- → fixed
Updated•4 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•