Last Comment Bug 779923 - implement activity flyout panel
: implement activity flyout panel
Status: RESOLVED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy)
:
: Shane Caraveo (:mixedpuppy)
Mentors:
: 763785 (view as bug list)
Depends on: 777176 779686
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-02 10:41 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-10-24 18:02 PDT (History)
5 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simply flyout patch (5.87 KB, patch)
2012-08-02 11:32 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
firstopenbug.png (20.03 KB, image/png)
2012-08-02 11:33 PDT, Shane Caraveo (:mixedpuppy)
no flags Details
secondopen.png (21.41 KB, image/png)
2012-08-02 11:34 PDT, Shane Caraveo (:mixedpuppy)
no flags Details
patch on top of 779686 (4.13 KB, patch)
2012-08-02 16:18 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
simple flyout patch (6.90 KB, patch)
2012-08-03 13:12 PDT, Shane Caraveo (:mixedpuppy)
jaws: feedback+
Details | Diff | Splinter Review
updated patch (6.84 KB, patch)
2012-08-14 15:42 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
flyout patch with tests (14.44 KB, patch)
2012-08-16 15:27 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
updated patch (14.48 KB, patch)
2012-08-17 15:35 PDT, Shane Caraveo (:mixedpuppy)
jaws: review-
Details | Diff | Splinter Review
first open rtl (48.19 KB, image/png)
2012-08-20 14:37 PDT, Shane Caraveo (:mixedpuppy)
no flags Details
second open rtl (48.96 KB, image/png)
2012-08-20 14:37 PDT, Shane Caraveo (:mixedpuppy)
no flags Details
flyout patch with tests (14.92 KB, patch)
2012-08-20 14:52 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
flyout patch with tests (15.56 KB, patch)
2012-08-20 14:59 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
flyout patch with tests (17.49 KB, patch)
2012-08-20 15:10 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
updated from comments (17.59 KB, patch)
2012-08-21 15:02 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
flyout-v2 (22.42 KB, patch)
2012-08-22 11:34 PDT, Shane Caraveo (:mixedpuppy)
jaws: review-
Details | Diff | Splinter Review
flyout-v2 (22.74 KB, patch)
2012-08-22 16:48 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review
flyout-v2.1 (21.47 KB, patch)
2012-08-23 17:27 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-08-02 10:41:06 PDT

    
Comment 1 Shane Caraveo (:mixedpuppy) 2012-08-02 11:32:57 PDT
Created attachment 648406 [details] [diff] [review]
simply flyout patch
Comment 2 Shane Caraveo (:mixedpuppy) 2012-08-02 11:33:52 PDT
Created attachment 648408 [details]
firstopenbug.png
Comment 3 Shane Caraveo (:mixedpuppy) 2012-08-02 11:34:42 PDT
Created attachment 648410 [details]
secondopen.png
Comment 4 Shane Caraveo (:mixedpuppy) 2012-08-02 11:37:27 PDT
The attached patch implements a flyout panel for the sidebar using the existing popup panel with an added browser element.

The first of the two images show a bug when the panel is first opened, this happens with panels anchored to on a side, we would need to figure out the fix for this.

The panel is green because the content I load for testing sets the content background to green when it has access to navigator.mozSocial.
Comment 5 Shane Caraveo (:mixedpuppy) 2012-08-02 11:40:15 PDT
another note, trying to shift the panel closer to the anchor causes the loss of the arrow, I'm assuming this is another generic panel bug.
Comment 6 Shane Caraveo (:mixedpuppy) 2012-08-02 16:18:20 PDT
Created attachment 648542 [details] [diff] [review]
patch on top of 779686

prototype for feedback
Comment 7 Shane Caraveo (:mixedpuppy) 2012-08-03 13:12:25 PDT
Created attachment 648822 [details] [diff] [review]
simple flyout patch

figured out how to fix the first open problem, has to do with side and position attributes on the panel, which are defaulted for the typical panel use.
Comment 8 Shane Caraveo (:mixedpuppy) 2012-08-03 15:13:07 PDT
This is a simple enough patch to get together, I'm marking this for 17
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-08-06 13:54:33 PDT
Comment on attachment 648822 [details] [diff] [review]
simple flyout patch

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

::: browser/base/content/browser.xul
@@ +280,5 @@
>        </deck>
>      </panel>
> +    <panel id="social-flyout-panel"
> +           class="social-arrow-panel"
> +           side="right"

This doesn't look like it will work for RTL.

::: browser/themes/pinstripe/browser.css
@@ +3511,5 @@
>    margin-right: -1px;
>    -moz-transform: scaleX(-1);
>  }
>  
> +.social-arrow-panel .panel-arrow[side="right"] {

The side="left" and side="right" here can be simplified if there was a side="horiz-end" type of value.

Then you could do:
.social-panel-arrow[side="horiz-end"] {
  list-style-image: url("...");
  -moz-margin-end: -1px;
}

.social-panel-arrow[side="horiz-end"]:-moz-locale-dir(rtl) {
  transform: scaleX(-1);
}

::: toolkit/components/social/MozSocialAPI.jsm
@@ +181,5 @@
> +    let doc = notifBrowser.contentDocument;
> +    // "notif" is an implementation detail that we should get rid of
> +    // eventually
> +    let body = doc.getElementById("notif") || (doc.body && doc.body.firstChild);
> +    if (!body) {

We should be using the measurements from doc.getElementById("notif").firstChild. So this code should be:
let body = doc.getElementById("notif") || doc.body;
if (!body || !body.firstChild) {
  return;
}

See the patch for bug 777176 for some of the tweaks that I made to this function to get it to work correctly with the panel sizing.
Comment 10 Shane Caraveo (:mixedpuppy) 2012-08-06 14:29:18 PDT
(In reply to Jared Wein [:jaws] from comment #9)

> >      </panel>
> > +    <panel id="social-flyout-panel"
> > +           class="social-arrow-panel"
> > +           side="right"
> 
> This doesn't look like it will work for RTL.

> > +.social-arrow-panel .panel-arrow[side="right"] {
> 
> The side="left" and side="right" here can be simplified if there was a
> side="horiz-end" type of value.

The "side" attribute is a part of the panel xbl and must be set for right/left to appear correctly on the first popup.

> We should be using the measurements from
> doc.getElementById("notif").firstChild. So this code should be:
> let body = doc.getElementById("notif") || doc.body;
> if (!body || !body.firstChild) {
>   return;
> }
> 
> See the patch for bug 777176 for some of the tweaks that I made to this
> function to get it to work correctly with the panel sizing.

yeah, iirc I saw that after I did the patch, we need to perhaps move that to a utility function somewhere.
Comment 11 Shane Caraveo (:mixedpuppy) 2012-08-14 15:42:16 PDT
Created attachment 651919 [details] [diff] [review]
updated patch
Comment 12 Shane Caraveo (:mixedpuppy) 2012-08-16 15:27:13 PDT
Created attachment 652591 [details] [diff] [review]
flyout patch with tests
Comment 13 Shane Caraveo (:mixedpuppy) 2012-08-17 15:35:58 PDT
Created attachment 652957 [details] [diff] [review]
updated patch

update event names for show/hide
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-08-20 12:58:02 PDT
Comment on attachment 652957 [details] [diff] [review]
updated patch

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

I see changes for pinstripe/browser.css and winstripe/browser.css, should there be changes to gnomestripe?

::: browser/base/content/browser.xul
@@ +272,5 @@
>        <box id="social-notification-box" flex="1"></box>
>      </panel>
> +    <panel id="social-flyout-panel"
> +           class="social-arrow-panel"
> +           side="right"

Where does side get changed to "right"? Could it ever be "top" or "bottom"?

::: browser/base/content/test/browser_social_flyout.js
@@ +28,5 @@
> +        case "got-sidebar-message":
> +          port.postMessage({topic: "test-flyout-open"});
> +          break;
> +        case "got-flyout-visibility":
> +          ok(e.data.result == "hidden", "flyout visibility "+e.data.result);

is(e.data.result, "hidden", "flyout visibility should be hidden");

::: browser/base/content/test/social_flyout.html
@@ +12,5 @@
> +      }, false);
> +      window.addEventListener("socialFrameHide", function(e) {
> +        var port = navigator.mozSocial.getWorker().port;
> +        port.postMessage({topic: "flyout-visibility", result: "hidden"});
> +        

nit: trailing whitespace

::: browser/themes/pinstripe/browser.css
@@ +3512,2 @@
>    list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png");
>    margin-right: -1px;

I'm not sure why separate arrow panel images are needed instead of using the already existing arrow images. Either way, we don't need to duplicate some of these rules.

This can be:
> .social-arrow-panel .panel-arrow[side="left"],
> .social-arrow-panel .panel-arrow[side="right"] {
>   list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png");
>   -moz-margin-end: -1px;
> }
> 
> .social-arrow-panel .panel-arrow[side="left"] {
>   transform: scaleX(-1);
> }

@@ +3513,5 @@
>    margin-right: -1px;
>    -moz-transform: scaleX(-1);
>  }
>  
> +.social-arrow-panel .panel-arrow[side="right"] {

Since you're changing these, please convert them to use the child selector (>).

::: toolkit/components/social/MozSocialAPI.jsm
@@ +205,5 @@
> +    // create and initialize the panel for this window
> +    let iframe = chromeWindow.document.createElement("iframe");
> +    iframe.setAttribute("type", "content");
> +    iframe.setAttribute("flex", "1");
> +    iframe.setAttribute("mozframetype", "content");

If you are using a XUL:iframe, then you won't need to set the mozframetype attribute. That is only needed for HTML:iframes.

@@ +210,5 @@
> +    panel.appendChild(iframe);
> +
> +    panel.addEventListener("popupshowing", function onpopupshowing() {
> +      let docShell = iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell);
> +      docShell.isActive = true;

This QI isn't needed if you use a XUL:iframe (https://developer.mozilla.org/en-US/docs/XUL/iframe#p-docShell).

@@ +248,5 @@
> +    if (!body || !body.firstChild) {
> +      return;
> +    }
> +
> +    let [height, width] = [body.firstChild.offsetHeight || 300, 330];

For chat, I think we'll want to limit the height of the chat window to 300, so please add a check like so after this:
> height = height > 300 ? 300 : height;
Comment 15 Shane Caraveo (:mixedpuppy) 2012-08-20 13:24:47 PDT
(In reply to Jared Wein [:jaws] from comment #14)
> Comment on attachment 652957 [details] [diff] [review]

> @@ +248,5 @@
> > +    if (!body || !body.firstChild) {
> > +      return;
> > +    }
> > +
> > +    let [height, width] = [body.firstChild.offsetHeight || 300, 330];
> 
> For chat, I think we'll want to limit the height of the chat window to 300,
> so please add a check like so after this:
> > height = height > 300 ? 300 : height;

The flyout panel is not chat.
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2012-08-20 13:36:08 PDT
Whoops, ok please disregard that then.
Comment 17 Shane Caraveo (:mixedpuppy) 2012-08-20 14:37:10 PDT
Created attachment 653524 [details]
first open rtl
Comment 18 Shane Caraveo (:mixedpuppy) 2012-08-20 14:37:34 PDT
Created attachment 653526 [details]
second open rtl
Comment 19 Shane Caraveo (:mixedpuppy) 2012-08-20 14:52:32 PDT
Created attachment 653534 [details] [diff] [review]
flyout patch with tests

updated from feedback + removed custom panel css for osx.
Comment 20 Dão Gottwald [:dao] 2012-08-20 14:55:20 PDT
Comment on attachment 653534 [details] [diff] [review]
flyout patch with tests

>-  list-style-image: url("chrome://browser/skin/social/panelarrow-up.png");
>-  list-style-image: url("chrome://browser/skin/social/panelarrow-down.png");
>-  list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png");
>-  list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png");

You need to completely remove these files from the theme.
Comment 21 Shane Caraveo (:mixedpuppy) 2012-08-20 14:59:36 PDT
Created attachment 653537 [details] [diff] [review]
flyout patch with tests

removed arrow images
Comment 22 Shane Caraveo (:mixedpuppy) 2012-08-20 15:10:38 PDT
Created attachment 653543 [details] [diff] [review]
flyout patch with tests

sigh...remember jar as well.
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2012-08-21 12:30:51 PDT
Comment on attachment 653543 [details] [diff] [review]
flyout patch with tests

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

::: browser/base/content/test/browser_social_flyout.js
@@ +28,5 @@
> +        case "got-sidebar-message":
> +          port.postMessage({topic: "test-flyout-open"});
> +          break;
> +        case "got-flyout-visibility":
> +          ok(e.data.result == "hidden", "flyout visibility is hidden");

is(e.data.result, "hidden", "flyout visibility should be 'hidden'");

@@ +31,5 @@
> +        case "got-flyout-visibility":
> +          ok(e.data.result == "hidden", "flyout visibility is hidden");
> +          next();
> +        case "got-flyout-message":
> +          ok(true, "got flyout message");

This should check e.data.result, which would show if the |window| was valid or not.

::: toolkit/components/social/MozSocialAPI.jsm
@@ +264,5 @@
> +    notifBrowser.style.height = height + "px";
> +  }
> +
> +  let lastOrigin = notifBrowser.getAttribute("origin");
> +  if (lastOrigin != provider.origin) {

How do you foresee the origin attribute getting out of date?
Comment 24 :Felipe Gomes (needinfo me!) 2012-08-21 12:44:44 PDT
Comment on attachment 653543 [details] [diff] [review]
flyout patch with tests

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

::: toolkit/components/social/MozSocialAPI.jsm
@@ +199,5 @@
> +  if (!fullURL)
> +    return;
> +
> +  // assuming that we are using a deck, and the last browser element is reserved
> +  // for flyouts

this comment seems outdated, doesn't reflect the code

could you split the first-time panel creation in a separate function, and leave this one only about setting the URL and displaying the panel? and then in this function you could do:

let notifBrowser = panel.firstChild ? panel.firstChild : createFlyoutFrame(..)
Comment 25 Shane Caraveo (:mixedpuppy) 2012-08-21 14:44:22 PDT
(In reply to Jared Wein [:jaws] from comment #23)
> Comment on attachment 653543 [details] [diff] [review]

> ::: toolkit/components/social/MozSocialAPI.jsm
> @@ +264,5 @@
> > +    notifBrowser.style.height = height + "px";
> > +  }
> > +
> > +  let lastOrigin = notifBrowser.getAttribute("origin");
> > +  if (lastOrigin != provider.origin) {
> 
> How do you foresee the origin attribute getting out of date?

changing the provider will at a minimum unset origin.
Comment 26 Shane Caraveo (:mixedpuppy) 2012-08-21 15:02:36 PDT
Created attachment 653973 [details] [diff] [review]
updated from comments
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2012-08-21 23:22:45 PDT
If the provider changes, we should just remove all of the iframes that were constructed as part of the previous provider.

Either way, I think we should remove multiprovider focused parts of this patch until multiprovider support has landed.
Comment 28 Shane Caraveo (:mixedpuppy) 2012-08-22 11:34:06 PDT
Created attachment 654291 [details] [diff] [review]
flyout-v2

this patch changes direction a bit, moving the bulk of the panel code into browser-social.js
Comment 29 Jared Wein [:jaws] (please needinfo? me) 2012-08-22 14:47:58 PDT
Comment on attachment 654291 [details] [diff] [review]
flyout-v2

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

::: browser/base/content/browser-social.js
@@ +232,5 @@
> +    if (!panel.firstChild)
> +      return
> +    panel.removeEventListener("popupshown", this.onShown.bind(this));
> +    panel.removeEventListener("popuphiding", this.onHidden.bind(this));
> +    window.removeEventListener("unload", this.destroyFrame.bind(this));

.bind() will return a new instance of the function, so this removeEventListener won't actually remove the listener. One way to fix this is to store a reference to the bound function and use that reference for removing the event listener.

@@ +233,5 @@
> +      return
> +    panel.removeEventListener("popupshown", this.onShown.bind(this));
> +    panel.removeEventListener("popuphiding", this.onHidden.bind(this));
> +    window.removeEventListener("unload", this.destroyFrame.bind(this));
> +    panel.removeChild(panel.firstChild);

We only remove the firstChild, but if createFrame is called multiple times then many children can exist. If we want to limit this to only one child of the panel, then we should enforce that limit within createFrame as well as destroyFrame.

Also, I think createFrame and destroyFrame should have underscore prefixes since they shouldn't be called externally.

@@ +268,5 @@
> +    panel.hidden = false;
> +    let iframe = panel.firstChild;
> +
> +  
> +    let src = iframe.getAttribute("src");

nit: only one blank line above and also remove trailing whitespace in this patch.

::: browser/base/content/test/social_sidebar.html
@@ +9,5 @@
>            var topic = e.data.topic;
>            switch (topic) {
> +            case "test-flyout-open":
> +              navigator.mozSocial.openPanel("social_flyout.html", function(flyoutWin) {
> +                port.postMessage({topic: "flyout-opened", result: flyoutWin ? "ok" : "failed"});

Where is "flyout-opened" used? I don't see it anywhere else in this patch.

::: browser/themes/winstripe/browser.css
@@ +3443,5 @@
>  #social-statusarea-username:hover {
>    text-decoration: underline;
>  }
>  
> +.social-arrow-panel {

Lets get rid of the social-arrow-panel class. The height and width are explicitly set in sizeSocialPanelToContent and the other styles that used .social-arrow-panel have been removed in this patch.
Comment 30 Jared Wein [:jaws] (please needinfo? me) 2012-08-22 14:48:32 PDT
Comment on attachment 653973 [details] [diff] [review]
updated from comments

Clearing review on the older patch as Shane has requested to use the newer version going forward.
Comment 31 Shane Caraveo (:mixedpuppy) 2012-08-22 15:25:45 PDT
(In reply to Jared Wein [:jaws] from comment #29)
> Comment on attachment 654291 [details] [diff] [review]
> flyout-v2

> @@ +233,5 @@
> > +      return
> > +    panel.removeEventListener("popupshown", this.onShown.bind(this));
> > +    panel.removeEventListener("popuphiding", this.onHidden.bind(this));
> > +    window.removeEventListener("unload", this.destroyFrame.bind(this));
> > +    panel.removeChild(panel.firstChild);
> 
> We only remove the firstChild, but if createFrame is called multiple times
> then many children can exist. If we want to limit this to only one child of
> the panel, then we should enforce that limit within createFrame as well as
> destroyFrame.

I'll limit it to one in createFrame.

> Also, I think createFrame and destroyFrame should have underscore prefixes
> since they shouldn't be called externally.

destroyFrame would be called in the case you disable the feature (I thought I had put that in the patch)
Comment 32 Jared Wein [:jaws] (please needinfo? me) 2012-08-22 15:28:49 PDT
Yeah, it's in the patch, but the naming of the function makes it seem quite internal.
Comment 33 Shane Caraveo (:mixedpuppy) 2012-08-22 16:48:00 PDT
Created attachment 654427 [details] [diff] [review]
flyout-v2
Comment 34 Jared Wein [:jaws] (please needinfo? me) 2012-08-23 15:57:36 PDT
Comment on attachment 654427 [details] [diff] [review]
flyout-v2

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

r+ from me with the following issues addressed.

::: browser/base/content/browser-social.js
@@ +17,5 @@
>    },
>  
>    // Called on window unload
>    uninit: function SocialUI_uninit() {
> +    SocialFlyout.unload();

Do we need to be removing DOM elements on window unload? Seems like that is taken care of already when the window is being unloaded itself.

@@ +262,5 @@
> +      this._createFrame();
> +    panel.hidden = false;
> +    let iframe = panel.firstChild;
> +
> +  

carry over from previous review, please remove this extra blank line and trailing whitespace in this patch.

@@ +272,5 @@
> +        if (aCallback)
> +          aCallback(iframe.contentWindow);
> +      }, true);
> +      iframe.setAttribute("src", aURL);
> +    }

I think the callback should still be called if the src == aURL.

::: browser/base/content/browser.xul
@@ +267,5 @@
>  #endif
>        </hbox>
>      </panel>
>  
> +    <panel id="social-notification-panel" class="social-arrow-panel" type="arrow" hidden="true" noautofocus="true">

We no longer need the class="social-arrow-panel" on these two panels.

::: toolkit/components/social/MozSocialAPI.jsm
@@ +193,5 @@
>    }
>    return fullURL;
>  }
>  
> +function openPanel(chromeWindow, provider, url, offset, callback) {

Does this need to be a separate function? I think the body of this function can be moved inside of MozSocialAPI.openPanel.
Comment 35 Shane Caraveo (:mixedpuppy) 2012-08-23 17:27:48 PDT
Created attachment 654866 [details] [diff] [review]
flyout-v2.1

updated with last comments
Comment 36 Shane Caraveo (:mixedpuppy) 2012-08-24 15:20:41 PDT
IMO This can land and we can update css as appropriate for the panels on osx separately.
Comment 37 Shane Caraveo (:mixedpuppy) 2012-08-24 17:27:23 PDT
https://tbpl.mozilla.org/?tree=Try&rev=77badc2b71dc
Comment 39 Ryan VanderMeulen [:RyanVM] 2012-08-25 08:45:36 PDT
https://hg.mozilla.org/mozilla-central/rev/772c3eed0cfb
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-24 18:02:44 PDT
*** Bug 763785 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.