779923 - implement activity flyout panel

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: simply flyout patch

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: firstopenbug.png

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: secondopen.png

Comment 4

[Shane Caraveo (:mixedpuppy)]

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)]

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)]

Attachment: patch on top of 779686

prototype for feedback

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: 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)]

This is a simple enough patch to get together, I'm marking this for 17

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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)]

(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)]

Attachment: updated patch

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: flyout patch with tests

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: updated patch

update event names for show/hide

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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)]

(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)]

Whoops, ok please disregard that then.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: first open rtl

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: second open rtl

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: flyout patch with tests

updated from feedback + removed custom panel css for osx.

Comment 20

[Dão Gottwald [:dao]]

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)]

Attachment: flyout patch with tests

removed arrow images

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: flyout patch with tests

sigh...remember jar as well.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

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 for replies!)]

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)]

(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)]

Attachment: updated from comments

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

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)]

Attachment: 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)]

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)]

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)]

(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)]

Yeah, it's in the patch, but the naming of the function makes it seem quite internal.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: flyout-v2

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

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)]

Attachment: flyout-v2.1

updated with last comments

Comment 36

[Shane Caraveo (:mixedpuppy)]

IMO This can land and we can update css as appropriate for the panels on osx separately.

Comment 37

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=77badc2b71dc

Comment 38

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/772c3eed0cfb

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/772c3eed0cfb