Closed
Bug 784238
Opened 12 years ago
Closed 12 years ago
make isVisible API work for notification panels / chat windows
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 17
People
(Reporter: Gavin, Assigned: mixedpuppy)
References
Details
(Whiteboard: [Fx17])
Attachments
(1 file, 7 obsolete files)
6.45 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
Bug 779360 implemented the mozSocial.isVisible API, but only made it work for the sidebar. We should also make it work for chat windows and the notification panel windows. For notification panels, we need to reflect whether the notification is currently being displayed. For chat windows, we should probably reflect whether the window is maximized? Not sure about this.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Fx17]
Assignee | ||
Comment 1•12 years ago
|
||
since the larger part of this is the chatbox, I'll take it.
Assignee: nobody → mixedpuppy
Assignee | ||
Comment 2•12 years ago
|
||
a. always set before socialFrameShow/Hide events. b. set on load
Attachment #653876 -
Flags: review?(jaws)
Attachment #653876 -
Flags: review?(gavin.sharp)
Comment 3•12 years ago
|
||
Comment on attachment 653876 [details] [diff] [review] panel/chat isVisible support Review of attachment 653876 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following issues addressed. ::: browser/base/content/socialchat.xml @@ +23,5 @@ > <implementation implements="nsIDOMEventListener"> > <field name="iframe" readonly="true"> > document.getAnonymousElementByAttribute(this, "anonid", "iframe"); > </field> > + nit: trailing whitespace @@ +35,5 @@ > + this.isActive = false; > + } else { > + this.removeAttribute("minimized"); > + this.isActive = true; > + } this.isActive = !val; if (val) this.setAttribute(...); else this.removeAttribute(...); @@ +44,5 @@ > + <getter> > + return this.iframe.docShell.isActive; > + </getter> > + <setter> > + this.iframe.docShell.isActive = val; this.iframe.docShell.isActive = !!val;
Attachment #653876 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #653876 -
Attachment is obsolete: true
Attachment #653876 -
Flags: review?(gavin.sharp)
Attachment #653941 -
Flags: review?(jaws)
Attachment #653941 -
Flags: review?(gavin.sharp)
Comment 5•12 years ago
|
||
Comment on attachment 653941 [details] [diff] [review] updated w/comments Review of attachment 653941 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +480,5 @@ > panel.addEventListener("popupshown", function onpopupshown() { > panel.removeEventListener("popupshown", onpopupshown); > SocialToolbar.button.setAttribute("open", "true"); > if (notifBrowser.contentDocument.readyState == "complete") { > + notifBrowser.docShell.isActive = true; Can you move |notifBrowser.docShell.isActive = true;| to before the if-statement above? I think it's OK if we mark the document as active before it has finished loading, and it will be similar to the patch for bug 784198.
Attachment #653941 -
Flags: review?(jaws)
Attachment #653941 -
Flags: review?(gavin.sharp)
Attachment #653941 -
Flags: review+
Assignee | ||
Comment 6•12 years ago
|
||
updated with comments
Attachment #653941 -
Attachment is obsolete: true
Attachment #654415 -
Flags: review?(jaws)
Assignee | ||
Comment 7•12 years ago
|
||
updated from feedback comments
Attachment #654415 -
Attachment is obsolete: true
Attachment #654415 -
Flags: review?(jaws)
Attachment #654416 -
Flags: review?(jaws)
Assignee | ||
Comment 8•12 years ago
|
||
sigh, get the right patch here
Attachment #654416 -
Attachment is obsolete: true
Attachment #654416 -
Flags: review?(jaws)
Attachment #654426 -
Flags: review?(jaws)
Assignee | ||
Comment 9•12 years ago
|
||
rebased
Attachment #654426 -
Attachment is obsolete: true
Attachment #654426 -
Flags: review?(jaws)
Attachment #654705 -
Flags: review?(jaws)
Attachment #654705 -
Flags: review?(gavin.sharp)
Comment 10•12 years ago
|
||
Comment on attachment 654705 [details] [diff] [review] panel/chat isVisible support Review of attachment 654705 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +441,5 @@ > > panel.addEventListener("popupshown", function onpopupshown() { > panel.removeEventListener("popupshown", onpopupshown); > SocialToolbar.button.setAttribute("open", "true"); > notificationFrame.docShell.isAppTab = true; I thought you were going to add |notificationFrame.docShell.isActive = true;| here?
Attachment #654705 -
Flags: review?(jaws)
Attachment #654705 -
Flags: review?(gavin.sharp)
Attachment #654705 -
Flags: review-
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #10) > Comment on attachment 654705 [details] [diff] [review] > panel/chat isVisible support > > Review of attachment 654705 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/browser-social.js > @@ +441,5 @@ > > > > panel.addEventListener("popupshown", function onpopupshown() { > > panel.removeEventListener("popupshown", onpopupshown); > > SocialToolbar.button.setAttribute("open", "true"); > > notificationFrame.docShell.isAppTab = true; > > I thought you were going to add |notificationFrame.docShell.isActive = > true;| here? that's odd, I thought I had.
Assignee | ||
Comment 12•12 years ago
|
||
add missing isActive=true
Attachment #654705 -
Attachment is obsolete: true
Attachment #654849 -
Flags: review?(jaws)
Comment 13•12 years ago
|
||
Comment on attachment 654849 [details] [diff] [review] panel/chat isVisible support Review of attachment 654849 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +432,5 @@ > notificationFrame.contentDocument.documentElement.dispatchEvent(evt); > } > > panel.addEventListener("popuphiding", function onpopuphiding() { > panel.removeEventListener("popuphiding", onpopuphiding); Can you change this to use "popuphidden"?
Attachment #654849 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 14•12 years ago
|
||
using popuphidden
Attachment #654849 -
Attachment is obsolete: true
Attachment #654865 -
Flags: review?(jaws)
Updated•12 years ago
|
Attachment #654865 -
Flags: review?(jaws) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 15•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=77badc2b71dc
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5734825a4f1f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•