The default bug view has changed. See this FAQ.

make isVisible API work for notification panels / chat windows

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx17])

Attachments

(1 attachment, 7 obsolete attachments)

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

5 years ago
Whiteboard: [Fx17]
(Assignee)

Comment 1

5 years ago
since the larger part of this is the chatbox, I'll take it.
Assignee: nobody → mixedpuppy
(Assignee)

Comment 2

5 years ago
Created attachment 653876 [details] [diff] [review]
panel/chat isVisible support

a. always set before socialFrameShow/Hide events. 
b. set on load
Attachment #653876 - Flags: review?(jaws)
Attachment #653876 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Depends on: 783691
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

5 years ago
Created attachment 653941 [details] [diff] [review]
updated w/comments
Attachment #653876 - Attachment is obsolete: true
Attachment #653876 - Flags: review?(gavin.sharp)
Attachment #653941 - Flags: review?(jaws)
Attachment #653941 - Flags: review?(gavin.sharp)
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

5 years ago
Created attachment 654415 [details] [diff] [review]
events patch

updated with comments
Attachment #653941 - Attachment is obsolete: true
Attachment #654415 - Flags: review?(jaws)
(Assignee)

Comment 7

5 years ago
Created attachment 654416 [details] [diff] [review]
flyout-v2

updated from feedback comments
Attachment #654415 - Attachment is obsolete: true
Attachment #654415 - Flags: review?(jaws)
Attachment #654416 - Flags: review?(jaws)
(Assignee)

Comment 8

5 years ago
Created attachment 654426 [details] [diff] [review]
events patch

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

5 years ago
Created attachment 654705 [details] [diff] [review]
panel/chat isVisible support

rebased
Attachment #654426 - Attachment is obsolete: true
Attachment #654426 - Flags: review?(jaws)
Attachment #654705 - Flags: review?(jaws)
Attachment #654705 - Flags: review?(gavin.sharp)
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

5 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

5 years ago
Created attachment 654849 [details] [diff] [review]
panel/chat isVisible support

add missing isActive=true
Attachment #654705 - Attachment is obsolete: true
Attachment #654849 - Flags: review?(jaws)
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

5 years ago
Created attachment 654865 [details] [diff] [review]
panel/chat isVisible support

using popuphidden
Attachment #654849 - Attachment is obsolete: true
Attachment #654865 - Flags: review?(jaws)
Attachment #654865 - Flags: review?(jaws) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 15

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=77badc2b71dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5734825a4f1f
Status: NEW → ASSIGNED
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5734825a4f1f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.