The default bug view has changed. See this FAQ.

Keep the social sidebar loaded, and dispatch events when it is opened/closed

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
dont unload sidebar, use page visibility api (visibility changed event).  this is a change on the provider side, we just need to verify it works
(Assignee)

Comment 1

5 years ago
Created attachment 645616 [details] [diff] [review]
sidebar visibility patch
Attachment #645616 - Flags: feedback?(jaws)
(Assignee)

Updated

5 years ago
Attachment #645616 - Flags: feedback?(mhammond)
(Assignee)

Comment 2

5 years ago
visibility api reference: https://developer.mozilla.org/en/DOM/Using_the_Page_Visibility_API
It seems that minimizing the browser also triggers the visibility event.  I could also imagine that in the future the event will be sent when the page is not minimized but fully obscured by other windows - it doesn't seem to do that now but the spec seems to call for it.  Last I heard, our partners are looking specifically for an event which tells them when the sidebar has been hidden as they may then re-enable certain elements on their normal page.  I'm not sure this approach will offer that.
FWIW, the patch also causes leaks to be reported.  If the approach in this patch goes ahead, I expect you will still want to fully unload the sidebar when the entire social feature is disabled.
Attachment #645616 - Attachment is patch: true
Comment on attachment 645616 [details] [diff] [review]
sidebar visibility patch

Review of attachment 645616 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +424,4 @@
>      }
> +    else {
> +      sbrowser.docShellIsActive = true;
> +    }

This can be refactored to:
sbrowser.docShellIsActive = !broadcaster.hidden;

::: browser/base/content/test/browser_social_sidebar.js
@@ +16,5 @@
>    let manifest = { // normal provider
>      name: "provider 1",
> +    origin: "http://example.com",
> +    sidebarURL: "http://example.com/browser/browser/base/content/test/social_sidebar.html",
> +    workerURL: "http://example.com/browser/browser/base/content/test/social_worker.js",

Out of curiosity, was there a reason that the test switched from https to http?
Attachment #645616 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 647299 [details] [diff] [review]
sidebar visibility patch

based on comments from markh, I think custom events for showing/hidding the sidebar is probably better.  Here is a patch implementing that.
Attachment #645616 - Attachment is obsolete: true
Attachment #645616 - Flags: feedback?(mhammond)
Attachment #647299 - Flags: feedback?(jaws)
Attachment #647299 - Flags: feedback?(gavin.sharp)
While I think the custom event approach is fine, does the custom event approach remove the ability for script to query the state of the sidebar?
Summary: use page visibility api for sidebar → Keep the social sidebar loaded, and dispatch events when it is opened/closed
(Assignee)

Comment 8

5 years ago
(In reply to Jared Wein [:jaws] from comment #7)
> While I think the custom event approach is fine, does the custom event
> approach remove the ability for script to query the state of the sidebar?

It does.  We could add a mozSocial.sidebarVisible api.
Comment on attachment 647299 [details] [diff] [review]
sidebar visibility patch

Review of attachment 647299 [details] [diff] [review]:
-----------------------------------------------------------------

IIRC, someone mentioned in IRC #socialdev that they could also get this information to content pages via localstorage. It's not an ideal API since it isn't straightforward, but will allow us to move forward with this. I think a better solution though would be to have a property on .mozSocial since it should be more intuitive.

Shane, can you file a separate bug to add the property to mozSocial?
Attachment #647299 - Flags: feedback?(jaws) → feedback+
(Assignee)

Updated

5 years ago
Blocks: 779360
(Assignee)

Comment 10

5 years ago
(In reply to Jared Wein [:jaws] from comment #9)

> Shane, can you file a separate bug to add the property to mozSocial?


Bug is filed, but this functionality is not needed per conversation about this change.
(Assignee)

Comment 11

5 years ago
Comment on attachment 647299 [details] [diff] [review]
sidebar visibility patch

moving this to review
Attachment #647299 - Flags: feedback?(gavin.sharp) → review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 755131
Comment on attachment 647299 [details] [diff] [review]
sidebar visibility patch

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   updateSidebar: function SocialSidebar_updateSidebar() {

>+    // Load the sidebar document
>+    let type = broadcaster.hidden ? "sidebarhide" : "sidebarshow";

nit: check hideSidebar rather than broadcaster.hidden.

Wouldn't it be simpler for providers if we fired a single event ("sidebarstatechange") with a different detail to indicate show/hide?

>+    if (sbrowser.getAttribute("origin") != Social.provider.origin) {

>+        window.setTimeout(function () {
>+          SocialSidebar.dispatchEvent(type);
>+        }, 10);

use 0

>+      }, true);

Don't you need to pass true to removeEventListener if you passed true to addEventListener? Does it need to be a capturing listener? You could just omit it here.

>     }
>+    else {

nit: cuddle else braces: } else {

I think we do actually want to unload the sidebar when hideSidebar is true because !this.enabled (as opposed to !this.canShow) - enabling/disabling social shouldn't leave the sidebar document loaded forever.
Attachment #647299 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 14

5 years ago
Created attachment 648832 [details] [diff] [review]
sidebar visibility patch

updated based on review feedback
Attachment #647299 - Attachment is obsolete: true
Attachment #648832 - Flags: review?(gavin.sharp)
Comment on attachment 648832 [details] [diff] [review]
sidebar visibility patch

Looking good, just a couple of tweaks needed:

- dispatchEvent is synchronous, so you don't need the setTimeout after sidebarhide.
- I think we should unload the sidebar if !this.canShow, rather than if !Social.enabled.
- use 0 for the setTimeout delay, rather than 10
Attachment #648832 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 648863 [details]
sidebar visibility patch

updated with latest comments
Attachment #648832 - Attachment is obsolete: true
Attachment #648863 - Flags: review?(gavin.sharp)
Created attachment 648877 [details] [diff] [review]
patch with minor tweak and test addition

I just made some minor style tweaks and added a test for "sidebarshow".
Attachment #648863 - Attachment is obsolete: true
Attachment #648863 - Flags: review?(gavin.sharp)
Attachment #648877 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c8194da40
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/bc1c8194da40
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.