The default bug view has changed. See this FAQ.

Implement mozSocial.isVisible API

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: mixedpuppy, Assigned: jaws)

Tracking

Trunk
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx17])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
consider if we need a boolean value for whether the sidebar is visible or not.
Created attachment 649879 [details] [diff] [review]
Patch

This patch adds a sidebarVisible function to mozSocial, although I'm having a hard time testing it with the scatchpad:
> document.getElementById("social-sidebar-browser").contentWindow.navigator.mozSocial is undefined
but Ctrl+I inspecting it shows the object exists.

Also, I'm not sure if this is the right place for functionality like this. This doesn't look like it is easily accessible from whitelisted content (which as I understand it is the goal).

On a smaller nit, I think this should probably be a getter instead of a function, but I've left that change out for now.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #649879 - Flags: feedback?(gavin.sharp)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
You probably need:
document.getElementById("social-sidebar-browser").contentWindow.navigator.wrappedJSObject.mozSocial
Comment on attachment 649879 [details] [diff] [review]
Patch

Thanks, wrappedJSObject was what I needed. This works fine, when I switch it to a getter then it seems to stick to returning true even when the sidebar is hidden. I'm not sure why, but this patch works so maybe we'll just leave it as a function (maybe just rename it to isSidebarVisible() to make it clear that there is no side effects).
Attachment #649879 - Flags: feedback?(gavin.sharp) → review?(gavin.sharp)
I got some help from Felipe and we found that the reason the getter isn't working is because of the way that we create the propList in MozSocialAPI.jsm. The getter is run once during propList construction and its return value is stored instead of the getter.

I think we should leave this as a function to keep the code simpler and rename it to isSidebarVisible().
It looks like defineProperties does allow property getters, and I think a 'sidebarVisible' property is much nicer...
(Reporter)

Comment 6

5 years ago
We're going to need a visibility api for chat windows, bug 779686.  I'd like to propose that this api becomes isVisible and reflects the visibility state of each particular widget that this api is injected into.
Created attachment 650333 [details] [diff] [review]
Patch v2

This patch adds a getter named 'isVisible', which returns true if the calling widget is visible. Shane, can you test this out with the chatbrowser work you're doing?
Attachment #649879 - Attachment is obsolete: true
Attachment #649879 - Flags: review?(gavin.sharp)
Attachment #650333 - Flags: feedback?(mixedpuppy)
Attachment #650333 - Flags: feedback?(gavin.sharp)
Comment on attachment 650333 [details] [diff] [review]
Patch v2

>diff --git a/toolkit/components/social/MozSocialAPI.jsm b/toolkit/components/social/MozSocialAPI.jsm

>+    isVisible: {

>+        let sidebarBrowser = mainWindow.document.getElementById("social-sidebar-browser");
>+        if (targetWindow == sidebarBrowser.contentWindow) {
>+          let sidebar = mainWindow.document.getElementById("social-sidebar-box");

>+        let chatbrowsers = mainWindow.document.getElementsByTagName("chatbrowser");

Hardcoding the front-end IDs and use of "chatbrowser" in here isn't ideal, since it really ties this code to the current browser-specific implementation details. I think a better solution would be to just have this return:
targetWindow.QueryInterface(Ci.nsIInterfaceRequestor)
            .getInterface(Ci.nsIWebNavigation)
            .QueryInterface(Ci.nsIDocShell).isActive;

and then have the front-end code set isActive appropriately (when the sidebar closes, when the panels close, etc.). That also has the benefit of making the normal !isActive docshell optimizations like timeout throttling apply to social content, when they're hidden.
Attachment #650333 - Flags: feedback?(gavin.sharp) → feedback-
Created attachment 650343 [details] [diff] [review]
Patch v3

Thanks Gavin, I like this approach a lot better.
Attachment #650333 - Attachment is obsolete: true
Attachment #650333 - Flags: feedback?(mixedpuppy)
Attachment #650343 - Flags: feedback?(mixedpuppy)
Attachment #650343 - Flags: feedback?(gavin.sharp)
(Reporter)

Updated

5 years ago
Whiteboard: [Fx17]
Attachment #650343 - Flags: feedback?(gavin.sharp)
Attachment #650343 - Flags: feedback?(felipc)
Attachment #650343 - Flags: feedback?
Attachment #650343 - Flags: review?(felipc)
Attachment #650343 - Flags: feedback?(mixedpuppy)
Attachment #650343 - Flags: feedback?(felipc)
Attachment #650343 - Flags: feedback?
Attachment #650343 - Flags: review?(felipc) → review+
forgot to mention: while you're at it, it'd be nice if you added names to the functions in mozSocialObj as they are all anonymous at the moment
(Reporter)

Updated

5 years ago
Blocks: 779686
(Reporter)

Updated

5 years ago
Blocks: 774520
Update on the status of this bug: I wrote a test for this bug and have hit some problems with the tests and am working on narrowing down the cause. If the new test for this bug runs before the browser_social_mozSocialAPI.js test, then the mozSocialAPI test will time out. If it runs afterwards then all is fine.
Summary: mozSocial.sidebarVisible → Implement mozSocial.isVisible API
Can you attach the latest patch here? I can try to help figure it out.
Created attachment 653145 [details] [diff] [review]
Tests for patch (reduced Makefile)

This is the test that I wrote for the patch. I made the Makefile.in much shorter to speed up the test runs, and it still reproduces the problem.
Attachment #653145 - Flags: feedback?(gavin.sharp)
Comment on attachment 653145 [details] [diff] [review]
Tests for patch (reduced Makefile)

I think I found the issues, will attach some additional changes that fixes the tests for me.
Attachment #653145 - Flags: feedback?(gavin.sharp) → feedback+
Created attachment 653406 [details] [diff] [review]
test fixes

Two issues:
- exceptions under the social:pref-changed observer weren't being reported for some reason. This hid the fact that since bug 777176 landed, SocialToolbar.updateButtonHiddenState doesn't handle Social.provider being null (which happens during test runs after the tests complete). So it was failing to hide the sidebar after browser_social_sidebar.js ran.
- browser_social_isVisible.js didn't reset the sidebar open state pref, so after it ran social.sidebar.open would stay false, which would prevent the sidebar from loading during browser_social_mozSocial_API.js, which caused it to time out.

I noticed a third issue that wasn't causing failures but seems problematic as well:
- Since bug 777176, browser_social_mozSocial_API seems to rely on the sidebar loading after the pre-loaded panel, which isn't guaranteed anymore (the test used to rely on the panel only loading when it was activated, but bug 777176 obviously changed that). This will be easier to fix once we have the shown/hidden notifications for panels, which Shane has a patch for somewhere, so I'll file a followup.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> SocialToolbar.updateButtonHiddenState doesn't handle Social.provider being
> null (which happens during test runs after the tests complete). So it was
> failing to hide the sidebar after browser_social_sidebar.js ran.

Rather: updateSidebar wasn't being called to hide the sidebar, because updateButtonHiddenState was throwing silently.

> I noticed a third issue that wasn't causing failures but seems problematic
> as well:

We'll fix this in bug 783691.
Attachment #653406 - Flags: review+
Created attachment 653620 [details] [diff] [review]
rollup patch with panel changes removed

As discussed on IRC, I think the panel changes in the previous patch were wrong. Sorting out the right behavior for panels might be a bit tricky, so let's do that in a followup to avoid blocking the landing of 779686.
Attachment #650343 - Attachment is obsolete: true
Attachment #653145 - Attachment is obsolete: true
Attachment #653406 - Attachment is obsolete: true
Attachment #653620 - Flags: feedback?(felipc)
Attachment #653620 - Flags: feedback?(felipc) → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/23870683fcc2
Flags: in-testsuite+
Target Milestone: --- → Firefox 17
Filed bug 784238 to cover the panel/chat changes.
https://hg.mozilla.org/mozilla-central/rev/23870683fcc2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 785676
Comment hidden (spam)

Comment 22

a year ago
follow-up-to-spam
Comment on attachment 8682276 [details]
pref_key_personalize_bg[1].jpg

Please don't abuse flags on attachments.
Attachment #8682276 - Flags: ui-review+
Attachment #8682276 - Flags: superreview?
Attachment #8682276 - Flags: sec-approval?
Attachment #8682276 - Flags: approval-mozilla-release?
Attachment #8682276 - Flags: approval-mozilla-esr31?
Attachment #8682276 - Flags: approval-mozilla-beta?
Attachment #8682276 - Flags: approval-mozilla-aurora?
Attachment #8682276 - Flags: a11y-review?

Updated

a year ago
Flags: needinfo?(alecia.dennord80)
Attachment #8682276 - Attachment is obsolete: true
Attachment #8682276 - Flags: review+
Attachment #8682276 - Flags: feedback+
Attachment #8682276 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.