Note: There are a few cases of duplicates in user autocompletion which are being worked on.

implement activity flyout panel

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:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx17])

Attachments

(4 attachments, 13 obsolete attachments)

21.41 KB, image/png
Details
48.19 KB, image/png
Details
48.96 KB, image/png
Details
21.47 KB, patch
jaws
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 648406 [details] [diff] [review]
simply flyout patch
(Assignee)

Comment 2

5 years ago
Created attachment 648408 [details]
firstopenbug.png
(Assignee)

Comment 3

5 years ago
Created attachment 648410 [details]
secondopen.png
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #648406 - Flags: feedback?(jaws)
Attachment #648406 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 777176, 774506
(Assignee)

Updated

5 years ago
Depends on: 779686
(Assignee)

Comment 6

5 years ago
Created attachment 648542 [details] [diff] [review]
patch on top of 779686

prototype for feedback
Attachment #648406 - Attachment is obsolete: true
Attachment #648406 - Flags: feedback?(jaws)
Attachment #648406 - Flags: feedback?(gavin.sharp)
Attachment #648542 - Flags: feedback?(mhammond)
Attachment #648542 - Flags: feedback?(jaws)
Attachment #648542 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 7

5 years ago
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.
Attachment #648408 - Attachment is obsolete: true
Attachment #648542 - Attachment is obsolete: true
Attachment #648542 - Flags: feedback?(mhammond)
Attachment #648542 - Flags: feedback?(jaws)
Attachment #648542 - Flags: feedback?(gavin.sharp)
Attachment #648822 - Flags: feedback?(jaws)
Attachment #648822 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 8

5 years ago
This is a simple enough patch to get together, I'm marking this for 17
Whiteboard: [Fx17]
(Assignee)

Updated

5 years ago
Assignee: nobody → mixedpuppy
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.
Attachment #648822 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Updated

5 years ago
No longer depends on: 774506
(Assignee)

Comment 11

5 years ago
Created attachment 651919 [details] [diff] [review]
updated patch
Attachment #648822 - Attachment is obsolete: true
Attachment #648822 - Flags: feedback?(gavin.sharp)
Attachment #651919 - Flags: feedback?(felipc)
(Assignee)

Comment 12

5 years ago
Created attachment 652591 [details] [diff] [review]
flyout patch with tests
Attachment #651919 - Attachment is obsolete: true
Attachment #651919 - Flags: feedback?(felipc)
Attachment #652591 - Flags: review?(jaws)
Attachment #652591 - Flags: review?(felipc)
(Assignee)

Comment 13

5 years ago
Created attachment 652957 [details] [diff] [review]
updated patch

update event names for show/hide
Attachment #652591 - Attachment is obsolete: true
Attachment #652591 - Flags: review?(jaws)
Attachment #652591 - Flags: review?(felipc)
Attachment #652957 - Flags: review?(jaws)
Attachment #652957 - Flags: review?(felipc)
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;
Attachment #652957 - Flags: review?(jaws) → review-
(Assignee)

Comment 15

5 years ago
(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.
Whoops, ok please disregard that then.
(Assignee)

Comment 17

5 years ago
Created attachment 653524 [details]
first open rtl
(Assignee)

Comment 18

5 years ago
Created attachment 653526 [details]
second open rtl
(Assignee)

Comment 19

5 years ago
Created attachment 653534 [details] [diff] [review]
flyout patch with tests

updated from feedback + removed custom panel css for osx.
Attachment #652957 - Attachment is obsolete: true
Attachment #652957 - Flags: review?(felipc)
Attachment #653534 - Flags: review?(jaws)
Attachment #653534 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 21

5 years ago
Created attachment 653537 [details] [diff] [review]
flyout patch with tests

removed arrow images
Attachment #653534 - Attachment is obsolete: true
Attachment #653534 - Flags: review?(jaws)
Attachment #653534 - Flags: review?(gavin.sharp)
Attachment #653537 - Flags: review?(jaws)
Attachment #653537 - Flags: review?(gavin.sharp)
(Assignee)

Comment 22

5 years ago
Created attachment 653543 [details] [diff] [review]
flyout patch with tests

sigh...remember jar as well.
Attachment #653537 - Attachment is obsolete: true
Attachment #653537 - Flags: review?(jaws)
Attachment #653537 - Flags: review?(gavin.sharp)
Attachment #653543 - Flags: review?(jaws)
Attachment #653543 - Flags: review?(gavin.sharp)
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 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(..)
(Assignee)

Comment 25

5 years ago
(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.
(Assignee)

Comment 26

5 years ago
Created attachment 653973 [details] [diff] [review]
updated from comments
Attachment #653543 - Attachment is obsolete: true
Attachment #653543 - Flags: review?(jaws)
Attachment #653543 - Flags: review?(gavin.sharp)
Attachment #653973 - Flags: review?(jaws)
Attachment #653973 - Flags: review?(felipc)
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.
(Assignee)

Comment 28

5 years ago
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
Attachment #654291 - Flags: review?(jaws)
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.
Attachment #654291 - Flags: review?(jaws) → review-
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.
Attachment #653973 - Attachment is obsolete: true
Attachment #653973 - Flags: review?(jaws)
Attachment #653973 - Flags: review?(felipc)
(Assignee)

Comment 31

5 years ago
(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)
Yeah, it's in the patch, but the naming of the function makes it seem quite internal.
(Assignee)

Comment 33

5 years ago
Created attachment 654427 [details] [diff] [review]
flyout-v2
Attachment #654291 - Attachment is obsolete: true
Attachment #654427 - Flags: review?(jaws)
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.
Attachment #654427 - Flags: review?(jaws) → review+
(Assignee)

Comment 35

5 years ago
Created attachment 654866 [details] [diff] [review]
flyout-v2.1

updated with last comments
Attachment #654427 - Attachment is obsolete: true
Attachment #654866 - Flags: review?(jaws)
Attachment #654866 - Attachment description: flyout-v2 → flyout-v2.1
Attachment #654866 - Flags: review?(jaws) → review+
(Assignee)

Comment 36

5 years ago
IMO This can land and we can update css as appropriate for the panels on osx separately.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 37

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