Closed Bug 779686 Opened 12 years ago Closed 12 years ago

implement docked chat content areas

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 17

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx17])

Attachments

(3 files, 15 obsolete files)

157.12 KB, image/png
Details
119.62 KB, image/png
Details
42.92 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
First step to create a preliminary patch based on the work done at https://github.com/mixedpuppy/browser-pinned-chats/ so that the basic functionality can be easily reviewed.
FWIW, I had a go at making these chat windows movable - https://github.com/mhammond/browser-pinned-chats.  From a mail to mike on 18-June (so things might have changed since then):

I had a bit of a play with the pinned tabs.  I managed to get resizing mainly working and moving of the windows *nearly* working.  The "moving" is having trouble with redraw - the item moves but doesn't paint - you need to force a repaint of the main window (by either resizing, or obscuring then reshowing the main window).  There also seems to be some issues with mouse capture while moving (eg, sometimes I don't get the mouseup event).

The "moving" is quite 1/2-baked.  As soon as you start dragging the (new) caption area it becomes "unpinned" and there is no way to re-pin it - it is really just a proof-of-concept rather than the "right thing"

If we really do need to resort to manually doing all the moves and resizes, I wonder if using a XUL panel with noautohide=true would end up working better?  I didn't investigate this at all...

It is really exceptionally hacky...
Also, bug 554926 might be relevant here - it is about adding the same basic functionality to the platform's panel impl.
I think for v1 of the chats we're punting on:

- detach to separate window, or dock back into a window
- selectable location
- any management of multiple chat windows, we'll limit to the width of the browser area, though we'd still need to think about closing some if the window is resized to a narrower width
Attached image chatwindows.png (obsolete) —
Attached image chat overflow issue.png (obsolete) —
Attached patch chat prototype patch (obsolete) — Splinter Review
initial prototype based on earlier prototype addon.  this is for feedback and initiation of more discussion on this feature.
Attachment #648530 - Flags: ui-review?
Attachment #648530 - Flags: feedback?(mhammond)
Attachment #648530 - Flags: feedback?(jboriss)
Attachment #648530 - Flags: feedback?(jaws)
Attachment #648530 - Flags: feedback?(gavin.sharp)
Blocks: 779923
Attachment #648530 - Attachment is patch: true
Comment on attachment 648530 [details] [diff] [review]
chat prototype patch

Looks promising! Haven't tested it out yet though :)

>diff --git a/browser/base/content/chatbrowser.xml b/browser/base/content/chatbrowser.xml

>+      <handler event="DOMContentLoaded">

>+        Object.defineProperty(this.browser.contentWindow, "statusIcon", {

Let's get rid of this for now.

>diff --git a/browser/themes/pinstripe/browser.css b/browser/themes/pinstripe/browser.css

>+chatbrowser > hbox > toolbarbutton {

Set a chatbrowser-button class on the buttons in chatbrowser.xml, and use that here (and in other selectors like this).

>+  cursor: default;

No need for this.

>+chatbrowser[open="true"] > hbox > .chat-toggle-button {
>+  -moz-image-region: rect(0, 32px, 16px, 16px);
>+}

You could have the chat-toggle-button inherit "open" from the chatbrowser, so that you can simplify this to:

.chat-toggle-button[open="true"]

>+chatbrowser > hbox {

Can add a class for this.

All of the styles that have "chatbrowser >" could be moved to a chatbrowser.css that is included in chatbrowser.xml (as in e.g. tabbrowser.css), and then you could just remove the "chatbrowser >" prefixes.

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

>+function openChatWindow(chromeWindow, provider, url, callback) {

>+  cb.addEventListener("DOMContentLoaded", function() {

Don't you need to remove this listener?

>+function openServiceWindow(provider, url, name, options, callback) {
>+  // resolve partial URLs and check prePath matches
>+  let fullURL = ensureProviderOrigin(provider, url);

You need to bail if !fullURL, right? Same with openChatWindow.
Attachment #648530 - Flags: feedback?(gavin.sharp) → feedback+
Comment on attachment 648530 [details] [diff] [review]
chat prototype patch

Looks good - is there a way we can see what it actually looks like and interact with it?
Attachment #648530 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 648530 [details] [diff] [review]
chat prototype patch

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

::: browser/themes/pinstripe/browser.css
@@ +3529,5 @@
> +}
> +
> +chatbrowser > hbox > toolbarbutton {
> +  -moz-appearance: none;
> +  border: none !important;

is it possible to remove the |!important| here?

@@ +3577,5 @@
> +   color: -moz-dialogtext;
> +}
> +
> +chatbrowser > hbox {
> +  background: -moz-linear-gradient(white, #ddd);

background-image: linear-gradient(white, #ddd);. if you need background-color: transparent, please make that explicit.

@@ +3600,5 @@
> +
> +
> +#pinnedchat-toolbar {
> +  -moz-appearance: none;
> +  transparent: true;

I don't think the |transparent| property-name exists.

@@ +3601,5 @@
> +
> +#pinnedchat-toolbar {
> +  -moz-appearance: none;
> +  transparent: true;
> +  opacity: 1;

opacity:1; should be the default property value, so no need to set it here.

@@ +3612,5 @@
> +  transparent: true;
> +  height: 0;
> +  width: 100%;
> +  opacity: 1.0;
> +  -moz-box-pack: end;

|-moz-box-pack:end;| here should be unnecessary since browser.xul already has pack="end" in the markup.

@@ +3621,5 @@
> +  height: 24px;
> +  background-color: white;
> +  border: 1px solid #404040; /* osx splitter color */
> +  border-bottom: none;
> +  margin: -5px -1px 0 0;

These margin values don't look like they'll support RTL correctly.

@@ +3627,5 @@
> +
> +#pinnedchats > chatbrowser[open="true"] {
> +  width:200px;
> +  height:200px;
> +  margin-top:-200px;

nit: space between colon and values.
Attachment #648530 - Flags: feedback?(jaws) → feedback+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> Comment on attachment 648530 [details] [diff] [review]
> chat prototype patch
> 
> Looks promising! Haven't tested it out yet though :)
> 
> >diff --git a/browser/base/content/chatbrowser.xml b/browser/base/content/chatbrowser.xml
> 
> >+      <handler event="DOMContentLoaded">
> 
> >+        Object.defineProperty(this.browser.contentWindow, "statusIcon", {
> 
> Let's get rid of this for now.

The statusIcon is used to convey presence information so there needs to be a way for content to set this.  We could handle it like favicons, or do some kind of linkadded listener instead.
Attached patch chat prototype patch (obsolete) — Splinter Review
With the exception of removing statusIcon, all feedback should be applied in this version.  As well, css should be "working" on all platforms.  I'm not at the office as I write this, so I have not tested aero or linux to see what it actually looks like.
Attachment #648530 - Attachment is obsolete: true
Attachment #648530 - Flags: ui-review?
Attachment #648530 - Flags: feedback?(jboriss)
Attachment #649706 - Flags: feedback?(jaws)
Attachment #649706 - Flags: feedback?(gavin.sharp)
Comment on attachment 649706 [details] [diff] [review]
chat prototype patch

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

::: browser/base/content/browser.css
@@ +628,5 @@
> +  width: 100px;
> +  height: 24px;
> +  background-color: white;
> +  /* some themes modify border color */
> +  border: 1px solid gray;

Only structural (display,visibility,width,height,etc) properties should be in /browser/base/content/browser.css. Any visual styles (background-color,border) should be in /browser/themes/*stripe/browser.css

@@ +630,5 @@
> +  background-color: white;
> +  /* some themes modify border color */
> +  border: 1px solid gray;
> +  border-bottom: none;
> +  margin: -5px -1px 0 0;

Can you install the Force RTL addon to get an approximation of this in RTL mode?
Attachment #649706 - Flags: feedback?(jaws) → feedback+
Attached patch chat prototype patch (obsolete) — Splinter Review
tested rtl with the addon, appearance is now correct in either.
Attachment #649706 - Attachment is obsolete: true
Attachment #649706 - Flags: feedback?(gavin.sharp)
Attachment #649830 - Flags: feedback?(jaws)
Attachment #649830 - Flags: feedback?(gavin.sharp)
Comment on attachment 649830 [details] [diff] [review]
chat prototype patch

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

::: browser/base/content/browser.css
@@ +638,5 @@
> +}
> +
> +/* keep the appearance of a 1px border between chats */
> +#pinnedchats:-moz-locale-dir(ltr) > chatbrowser {
> +  margin-right: -1px;  

both of these can be replaced with a single rule of: -moz-margin-end:-1px; in #pinnedchats.

::: browser/base/content/chatbrowser.xml
@@ +41,5 @@
> +        ]]></body>
> +      </method>
> +    </implementation>
> +    
> +    <handlers>

minor nit: if you look at the patch through the Splinter Review tool, you'll see trailing whitespace is highlighted as red. it'd be nice if you could remove the trailing whitespace where it is being added (don't worry about the preexisting places since I don't want this patch to touch lines that aren't relevant).

@@ +50,5 @@
> +        let box = this;
> +        Object.defineProperty(this.browser.contentWindow, "statusIcon", {
> +          set: function(val) {
> +            // TODO: sanitize the url
> +            box.image = val;

Please follow the same sanitization of the URL that was done in bug 776606.

::: browser/base/jar.mn
@@ +42,5 @@
>  *       content/browser/browser.xul                   (content/browser.xul)
>  *       content/browser/browser-tabPreviews.xml       (content/browser-tabPreviews.xml)
>  *       content/browser/content.js                    (content/content.js)
> +*       content/browser/chatbrowser.xml               (content/chatbrowser.xml)
> +*       content/browser/chatbrowser.css               (content/chatbrowser.css)

Do these files need to be run through the preprocessor? It doesn't look like it. See https://developer.mozilla.org/en-US/docs/JAR_Manifests for documentation on what the asterisk and other leading characters mean in jar files.

::: browser/themes/gnomestripe/browser.css
@@ +2750,5 @@
>  }
>  
> +#pinnedchats > chatbrowser {
> +  background-color: white;
> +  border: 1px solid gray; /* color ? */

I'd remove these comments for now, we can revisit the colors later if we think they don't match (the same goes for other properties as well).
Attachment #649830 - Flags: feedback?(jaws) → feedback+
Attached patch chat prototype patch (obsolete) — Splinter Review
with last comments fixed
Attachment #649830 - Attachment is obsolete: true
Attachment #649830 - Flags: feedback?(gavin.sharp)
Attachment #649847 - Flags: feedback?(jaws)
Attachment #649847 - Flags: feedback?(gavin.sharp)
(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Created attachment 648529 [details]
> chat overflow issue.png

To deal with overflow, we can hide chats which don't have enough room to display at the full width.  The chat that gets hidden first should be the one that has been acted upon or received a message longest ago, whether or not it's further to the left (assuming LTR).  So, if a user simply opens four chats, the newest chat would be on the right, and resizing the window would first hide the one on the left.  As the user decreases the width of the window, chats should disappear in decreasing order of last action, meaning the chat acted on last should be the last one to display on a decreasing window.

Hopefully the attached diagram makes this clear.  Are there any problems that would arise doing this if the chats were implemented as a single content area, as we discussed yesterday?
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #16)
> Created attachment 649904 [details]
> Diagram: Collapsing browser window width progressively hiding chats
> 
> (In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> > Created attachment 648529 [details]
> > chat overflow issue.png
> 
> The chat that gets hidden first should be the
> one that has been acted upon or received a message longest ago, whether or
> not it's further to the left (assuming LTR).  

I'll have to think about how we can manage a "least active" capability, it will most likely have to be provided by the social provider.

> Hopefully the attached diagram makes this clear.  Are there any problems
> that would arise doing this if the chats were implemented as a single
> content area, as we discussed yesterday?

We're doing a content area for each chat.  A single content would complicate click through to content underneath transparent area's.
Comment on attachment 649847 [details] [diff] [review]
chat prototype patch

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+#pinnedchats {
>+  -moz-appearance: none;

hboxes don't have a moz-appearance by default, so there shouldn't be any need to override it here. We need to be careful with unnecessary styles, they've been showing up relatively frequently in social patches, and that leads to confusing and overly complicated CSS.

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>       <statuspanel id="statusbar-display" inactive="true"/>
>+      <toolbar id="pinnedchat-toolbar" layer="true" mousethrough="always">

Does this need to be a toolbar, or could you just put the hbox here directly?

I'm not sure whether we really want this to be layer="true". We should check what effect it has on the layer tree, and/or get someone who understands layers better to advise.

>diff --git a/browser/base/content/chatbrowser.css b/browser/base/content/chatbrowser.css

These styles are basically all appearance-related, so this file should probably live in the theme rather than in /content (that means you'll need one copy per theme :/).

>diff --git a/browser/base/content/chatbrowser.xml b/browser/base/content/chatbrowser.xml

Add a license header here, and in all other newly added files.

>+<bindings id="chatBrowserBindings"

>+    xmlns:html="http://www.w3.org/1999/xhtml"

Doesn't look to be used?

>+        <xul:label class="chat-title" flex="1" xbl:inherits="value=label,accesskey,crop"/>

Not sure it makes sense to inherit the accesskey here?

>+        <xul:toolbarbutton class="chat-toggle-button chat-toolbarbutton"
>+                           xbl:inherits="open"
>+                           oncommand="this.parentNode.parentNode.toggle();"/>

Should be able to use: document.getBindingParent(this).toggle(); (same for chat-close-button)

>+      <xul:browser anonid="browser" class="chat-browser" flex="1" xbl:inherits="src,origin" type="content"/>

Can this be an iframe? Looks like the only properties you need off the browser are .contentWindow and contentTitle, which on an iframe are just .contentWindow and .contentDocument.title.

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

>+function openChatWindow(chromeWindow, provider, url, callback) {

>+  var container = chromeWindow.document.getElementById("pinnedchats");

Not sure I love this interface between the backend/frontend code. At the very least you should handle pinnedchats not existing and fail gracefully. It might be better to create a createSocialChatWindow(url, loadCallback) function in the global window scope, continaing the code below that actually creates and inserts the chatbrowser, and have this code call that if it exists. Keeps the separation between browser/ and toolkit/ better defined.
Attachment #649847 - Flags: feedback?(gavin.sharp) → feedback+
Attached image chatwindows.png
image for most recent patch.  this shows minimized, maximized and the button for windows hidden due to overflow.   We do need some images for various parts of this.
Attachment #648528 - Attachment is obsolete: true
Attachment #648529 - Attachment is obsolete: true
Attachment #650968 - Flags: ui-review?(jboriss)
Attached patch chat window patch (obsolete) — Splinter Review
This patch implements support for overflow moving windows into a button as well as some basic logic discussed with Boriss over email.  Most if not all previous comments have been integrated into this patch.  

left todo:
- get new images created for window overflow button, minimize/maximize button
- verify/cleanup css on win/lin
- tests
Attachment #649847 - Attachment is obsolete: true
Attachment #649847 - Flags: feedback?(jaws)
Attachment #650969 - Flags: feedback?(jaws)
Attachment #650969 - Flags: feedback?(gavin.sharp)
Attached patch chat window patch (obsolete) — Splinter Review
updated with some windows tweaks to make it look ok for now
Attachment #650969 - Attachment is obsolete: true
Attachment #650969 - Flags: feedback?(jaws)
Attachment #650969 - Flags: feedback?(gavin.sharp)
Attachment #651049 - Flags: feedback?(jaws)
Attachment #651049 - Flags: feedback?(gavin.sharp)
Comment on attachment 650968 [details]
chatwindows.png

If there is a chat window that is right-most, as in this screenshot, does it block access to the bottom part of the scrollbars?

We should move the chat window over just enough to not block any part of the scrollbars (even on pages that lack scrollbars, so as not to jump when switching tabs or when dynamic changes to content are made).
Depends on: 774506
Blocks: 774506
No longer depends on: 774506
Comment on attachment 651049 [details] [diff] [review]
chat window patch

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

Patch is looking good. Some drive-by feedback:

(looks like the patch for bug 760365 is mixed in this one)

::: browser/base/content/browser-social.js
@@ +165,5 @@
> +    return document.getElementById("pinnedchats");
> +  },
> +  // Whether the chats can be shown for this window.
> +  get canShow() {
> +    return Social.uiVisible && !this.chromeless;

this.chromeless doesn't exist in this object

::: browser/base/content/browser.css
@@ +615,5 @@
> +  -moz-binding: url("chrome://browser/content/chatbrowser.xml#chatbar");
> +  -moz-appearance: none;
> +  height: 0;
> +  margin-top: -20px;
> +}

any reason on the difference between using <chatbrowser> directly and making pinnedchats an <hbox>?

::: browser/base/content/chatbrowser.xml
@@ +26,5 @@
> +        let docShell = this.browser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell);
> +        docShell.setIsBrowserElement();
> +      </constructor>
> +
> +      <field name="browser">

readonly="true"

the name browser is a bit misleading because people it appears this is a <browser> element instead of an iframe  (e.g. I was going to suggest you to use this.browser.docShell above but that shortcut doesn't exist for iframes)

@@ +65,5 @@
> +      <handler event="DOMContentLoaded">
> +      <![CDATA[
> +        this.setAttribute("label", this.browser.contentDocument.title);
> +        let box = this;
> +        Object.defineProperty(this.browser.contentWindow, "statusIcon", {

Hmm this looks bad, you shouldn't define a property directly in the content window global. It should probably be part of the mozSocial API obj
Depends on: 779360
(In reply to Jared Wein [:jaws] from comment #22)
> Comment on attachment 650968 [details]
> chatwindows.png
> 
> If there is a chat window that is right-most, as in this screenshot, does it
> block access to the bottom part of the scrollbars?
> 
> We should move the chat window over just enough to not block any part of the
> scrollbars (even on pages that lack scrollbars, so as not to jump when
> switching tabs or when dynamic changes to content are made).

Maybe we can get Boriss to comment on that.
Attached patch updated from comments (obsolete) — Splinter Review
not quite ready with tests yet, but updated based on feedback.
Attachment #651049 - Attachment is obsolete: true
Attachment #651049 - Flags: feedback?(jaws)
Attachment #651049 - Flags: feedback?(gavin.sharp)
Attachment #651914 - Flags: feedback?(felipc)
Comment on attachment 651914 [details] [diff] [review]
updated from comments

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

This is looking good.  The firstRemovableChild seems a lot of effort, and I'm wondering if you'll need a more complete focus handling soon (for example to style focused/unfocused chatbrowsers in a different way), so listening for focus/blur might make things easier and simpler. But whatever is not strictly necessary for this bug is better left for a follow-up.

Also, this is a bit nitpicking, but: I don't know about the name "chatbrowser". It seems to be exposing an implementation detail (which is not even true anymore!, since it uses an iframe instead of browser). Maybe chatbox would be better? (so a chatbar has chatboxes.. makes more sense). Don't know what others feel about it

::: browser/base/content/browser-social.js
@@ +168,5 @@
> +  // Whether the chats can be shown for this window.
> +  get chromeless() {
> +    let docElem = document.documentElement;
> +    return docElem.getAttribute('disablechrome') ||
> +           docElem.getAttribute('chromehidden').indexOf("extrachrome") >= 0;

nit, if there is a good way to move it: it would be good to avoid repeating this function as it's already defined in the other object.

Or rather, .chromeless seems only an internal thing that doesn't need to be a "public" getter. So I'd just add this chunk of code inside .canShow

::: browser/base/content/chatbrowser.xml
@@ +72,5 @@
> +          return;
> +
> +        let relStrings = rel.split(/\s+/);
> +        let rels = {};
> +        for (let i = 0; i < relStrings.length; i++) {

you don't need this for loop, you can just check if link.rel.indexOf("icon") != -1

@@ +80,5 @@
> +          let uri = makeURI(link.href, targetDoc.characterSet);
> +
> +          // Sanitize the url from any potential script-injection.
> +          try {
> +            let scheme = uri ? uri.scheme : "";

please also clear the userPass property in the uri

@@ +89,5 @@
> +            continue;
> +          }
> +
> +          // we made it this far, use it
> +          this.setAttribute('image', link.href);

use uri.spec instead of link.href as we might have cleared the userPass

keep in mind that this will only support absoulute paths. if you want to also support relative ones you'll need to resolve the link.href against the ownerDocument URI

I wonder if we need to do this as well: http://hg.mozilla.org/mozilla-central/annotate/86ee4deea55b/browser/base/content/browser.js#l3225

@@ +201,5 @@
> +        ]]></body>
> +      </method>
> +
> +      <method name="hideChat">
> +        <parameter name="chatbrowser"/>

keep parameters name styling consistent (aFoo)
Attachment #651914 - Flags: feedback?(felipc) → feedback+
(In reply to :Felipe Gomes from comment #26)
> Maybe
> chatbox would be better? (so a chatbar has chatboxes.. makes more sense).
> Don't know what others feel about it

Might as well be "socialchat*" while we are at it...
(In reply to :Felipe Gomes from comment #26)
> @@ +80,5 @@
> > +          let uri = makeURI(link.href, targetDoc.characterSet);
> > +
> > +          // Sanitize the url from any potential script-injection.
> > +          try {
> > +            let scheme = uri ? uri.scheme : "";
> 
> please also clear the userPass property in the uri

I'm fine with doing that, but am wondering if it is necessary as DOMLinkHandler in browser.js does not do that.

> @@ +89,5 @@
> > +            continue;
> > +          }
> > +
> > +          // we made it this far, use it
> > +          this.setAttribute('image', link.href);
> 
> use uri.spec instead of link.href as we might have cleared the userPass

see comment at end.

> keep in mind that this will only support absoulute paths. if you want to
> also support relative ones you'll need to resolve the link.href against the
> ownerDocument URI

link.href is already resolved

> I wonder if we need to do this as well:
> http://hg.mozilla.org/mozilla-central/annotate/86ee4deea55b/browser/base/
> content/browser.js#l3225

I can add the security checks.

Just seeking further understanding...

I had borrowed much of this logic from that code you point to.  The regular browser handler does not clear passwords, and does use link.href in the end.  A quick test shows that username/password will go through, should this be an issue in the regular browser?  Or might it be necessary to pass that information through if the website included it?
(In reply to Shane Caraveo (:mixedpuppy) from comment #28)
> I'm fine with doing that, but am wondering if it is necessary as
> DOMLinkHandler in browser.js does not do that.

DOMLinkHandler not doing it is bug 580794.
Attached patch patch with tests (obsolete) — Splinter Review
Attachment #651914 - Attachment is obsolete: true
Attachment #652552 - Flags: review?(gavin.sharp)
Attachment #652552 - Flags: review?(felipc)
Depends on: 777176
Latest patch adds show/hide dom events for chat content, a "selectedChat" for the chatbar, and tests.
Comment on attachment 652552 [details] [diff] [review]
patch with tests

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

nice tests! I'm still reading through the tests but I've gone through all the non-test code now. Here are my comments:


nit: a couple of function bodies are not indented (they are in the same depth as the CDATA opening)

::: browser/base/content/socialchat.xml
@@ +22,5 @@
> +
> +    <implementation implements="nsIDOMEventListener">
> +      <constructor>
> +        // Mark this docShell as a "browserFrame", to break script access to e.g. window.top
> +        let docShell = this.iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell);

could this be simply this.iframe.docShell ?  not sure if this shortcut works for all types of iframes

@@ +26,5 @@
> +        let docShell = this.iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell);
> +        docShell.setIsBrowserElement();
> +      </constructor>
> +
> +      <field name="iframe">

mark this field as readonly="true"

@@ +30,5 @@
> +      <field name="iframe">
> +        document.getAnonymousElementByAttribute(this, "anonid", "iframe");
> +      </field>
> +
> +      <field name="_callback"/>

I don't think you need to explicitly declare the _callback and selectedChat fields

@@ +60,5 @@
> +            this.setAttribute("minimized", "true");
> +            type = "hide";
> +          }
> +          // let the chat frame know if it is being shown or hidden
> +          let evt = this.iframe.contentDocument.createEvent("CustomEvent");

are these for test purposes only? I wonder if this could be avoided if it's only needed for tests (use the worker perhaps? or have the test listeners in the parent). But let's leave this investigation for a follow-up

In any case, the events will bubble all the way up, so they need a more specific name than simply show/hide. I'd suggest using just one event ("chatbox-visibility-changed") and use the event detail to set the type.

@@ +120,5 @@
> +            return;
> +        } catch(e) {
> +          return; // Refuse to load if we can't do a security check.
> +        }
> +        uri.userPass = "";

set the userPass inside a try-catch block as it may fail   (some URLs are immutable.. although it's unlikely they will ever be used here)

@@ +186,5 @@
> +          let focusedWindow = document.commandDispatcher.focusedElement.ownerDocument.defaultView;
> +          focusedBrowser = focusedWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIWebNavigation)
> +                                  .QueryInterface(Ci.nsIDocShell)
> +                                  .chromeEventHandler;

you should now be able to remove this focusedBrowser part and in the loop just check "this.selectedChat == child" instead of "child.iframe == focusedBrowser"

@@ +273,5 @@
> +        <parameter name="aCallback"/>
> +        <body><![CDATA[
> +        let cb = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "chatbox");
> +        this.appendChild(cb);
> +        this.selectedChat = cb;

set selectedChat before appendChild so that when a new chat makes the chatbar overflow, the newly created chat can't be the one picked to be hidden

(I'm assuming that overflow handles well the addition of a new chat, and you don't need to do any special checks here to see if it will overflow before)

::: browser/themes/winstripe/browser-aero.css
@@ +111,5 @@
>      color: graytext;
>    }
> +
> +  .chatbar-button,
> +  chatbar > chatbrowser {

s/chatbrowser/chatbox/
Attachment #652552 - Flags: review?(felipc)
Attached patch updated from comments (obsolete) — Splinter Review
update from comments, also am allowing all chats to collapse into button if the browser width is too narrow for a single chat.
Attachment #652552 - Attachment is obsolete: true
Attachment #652552 - Flags: review?(gavin.sharp)
Attachment #652870 - Flags: review?(gavin.sharp)
Attachment #652870 - Flags: review?(felipc)
Comment on attachment 652870 [details] [diff] [review]
updated from comments

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

::: browser/base/content/socialchat.xml
@@ +56,5 @@
> +            type = "show";
> +          } else {
> +            this.setAttribute("minimized", "true");
> +            type = "hide";
> +          }

so let's land these events for now but using a more specific name. Also "hide" is misleading since this is about toggling to minimized state.. So I suggest ChatboxOpen and ChatboxMinimized. And we will iterate on this in a follow-up (the hideChat on overflow will also probably need some similar event)

Remember that you'll need to change the event names on the tests after renaming them here

@@ +67,5 @@
> +    </implementation>
> +
> +    <handlers>
> +      <handler event="focus" phase="capturing">
> +        dump("update the selected chat to us\n");

leftover dump stmt

@@ +72,5 @@
> +        this.parentNode.selectedChat = this;
> +      </handler>
> +      <handler event="DOMContentLoaded" action="if (this._callback) this._callback(this.iframe.contentWindow);"/>
> +      <handler event="DOMTitleChanged" action="this.setAttribute('label', this.iframe.contentDocument.title);"/>
> +      <handler event="DOMLinkAdded"><![CDATA[

This looks good. I talked with Gavin and we wondered if we could factor out this security checks part and put in a standalone function in browser.js so that we are not duplicating code here   (So both this DOMLinkAdded handler and the one in browser.js just calls the function)
Attached patch updated from comments (obsolete) — Splinter Review
factored domlink handler, changed event name, etc
Attachment #652870 - Attachment is obsolete: true
Attachment #652870 - Flags: review?(gavin.sharp)
Attachment #652870 - Flags: review?(felipc)
Attachment #652950 - Flags: review?(gavin.sharp)
Attachment #652950 - Flags: review?(felipc)
Comment on attachment 652950 [details] [diff] [review]
updated from comments

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

r=felipe! I pushed the previous patch to try: https://tbpl.mozilla.org/?tree=Try&rev=c5e6df448152 (removed the dependency on the changes from bug 779360)

waiting on results and will push the latest version to try as well
Attachment #652950 - Flags: review?(gavin.sharp)
Attachment #652950 - Flags: review?(felipc)
Attachment #652950 - Flags: review+
Attached patch fixed test (obsolete) — Splinter Review
this fixes the test breakage
Attachment #652950 - Attachment is obsolete: true
Attachment #652997 - Flags: review?(felipc)
Attachment #652997 - Attachment is patch: true
There is still something weird going on. See latest patch pushed at: https://tbpl.mozilla.org/?tree=Try&rev=1080de95897c

IIRC Jared noticed it is one of the existing tests that starts causing problems when a new test that uses runSocialTestWithProvider is added
Overlaying parts of the content area with unrelated UI seems like a misguided approach. How are (1) web content that expects a rectangular view port and (2) the user who might want to both chat and interact with the content supposed to handle this?
Comment on attachment 652997 [details] [diff] [review]
fixed test

>--- a/browser/base/content/browser-social.js
>+++ b/browser/base/content/browser-social.js

>+  get canShow() {
>+    let docElem = document.documentElement;
>+    let chromeless = docElem.getAttribute('disablechrome') ||
>+                     docElem.getAttribute('chromehidden').indexOf("extrachrome") >= 0;

' -> "

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css

>+chatbar {
>+  -moz-appearance: none;

chatbar isn't going to have some appearance by default.

>+chatbar > chatbox[minimized="true"] {
>+  border-bottom: none;

>+chatbar > chatbox + chatbox {
>+  -moz-margin-start: -1px;

This doesn't belong in browser/base/content/browser.css.
(In reply to Dão Gottwald [:dao] from comment #39)
> Overlaying parts of the content area with unrelated UI seems like a
> misguided approach. How are (1) web content that expects a rectangular view
> port and (2) the user who might want to both chat and interact with the
> content supposed to handle this?

I don't really understand the concern. Users who wish to interact with the content under the chat windows can either collapse them or close them. We'll probably want to also investigate "temporarily hide all" mechanisms or something like that, but we can sort that out in followup bugs once we have more experimental feedback.
Collapsing them just makes them smaller, they'll still block the bottom of the content area. You need to actually close them to fully unblock the page, which seems unreasonable. "Temporary hide all" seems like a strange workaround that shouldn't be necessary in the first place. Of course, users _should_ be able to close all chats if they want to, but not just to interact with something at the bottom of the content area.

This isn't a completely uncommon type of UI, so I'm not sure why we're trying to go a new way. There are legitimate reasons for UI pieces to completely take over the content area -- some dev tools do this -- but otherwise they should reside next to the content area. This looks like it tries to be somewhere in between, and I don't really see why. It seems like the boxes should just have their own bar at the bottom or sit in a side bar or something.
Attached patch chat window patch (obsolete) — Splinter Review
moved css to platform
Attachment #652997 - Attachment is obsolete: true
Attachment #652997 - Flags: review?(felipc)
Attachment #653571 - Flags: review?(gavin.sharp)
Attachment #653571 - Flags: review?(felipc)
Attachment #653571 - Flags: review?(felipc) → review+
(In reply to Dão Gottwald [:dao] from comment #42)

I'm not sure what you mean by "new way" - I don't really know of any other chatting+browsing UIs, but I admit I haven't explored the space much (does RockMelt do this?).

The idea is that people are going to be browsing, and occasionally chatting while they browse. We're not implementing a chat client, so we should assume that the chat UI will be transient, and not optimize for having many chats open for long periods of time.

Losing a few pixels from the bottom corner of your content window seems unlikely to be problematic in the majority of cases, and it nicely avoids taking up an entire row of the window for what will often be transient UI. If obscuring small amounts of content while chat is active ends up being too annoying, we can revisit and investigate "chat bar"-like options, but I don't think we need to settle that now.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #44)
> (In reply to Dão Gottwald [:dao] from comment #42)
> 
> I'm not sure what you mean by "new way" - I don't really know of any other
> chatting+browsing UIs, but I admit I haven't explored the space much (does
> RockMelt do this?).

By new way I mean boxes floating on top of the content area. The closes thing to that is the status panel, and it's not interactive. The closes thing to the chat boxes done properly would be something like a download status bar (e.g. the Firefox add-on or the download status bar built in to Chrome).

Of course this floating boxes model also doesn't scale. Back in the Firefox 4 dev cycle there was the idea to have the add-on bar overlay the corner of the content area. We didn't do this, for obvious reasons, but if we had done it, we'd now have to somehow group or stack different kind of boxes.

> The idea is that people are going to be browsing, and occasionally chatting
> while they browse. We're not implementing a chat client, so we should assume
> that the chat UI will be transient, and not optimize for having many chats
> open for long periods of time.

I don't get why the boxes can be collapsed, then. It seems collapsing is meant to temporarily get them out of the way, except that it won't really work.

> Losing a few pixels from the bottom corner of your content window seems
> unlikely to be problematic in the majority of cases

Please take a close look at attachment 650968 [details]. We don't even need to start imagining what important areas of the page could be blocked when even the ordinary vertical scroll bar becomes unusable.

> If obscuring small amounts of content while chat is active ends up being too
> annoying, we can revisit and investigate "chat bar"-like options, but I
> don't think we need to settle that now.

I think the issues are pretty obvious at this stage, not sure why it should be revisited later. I'd suggest doing what's known to work now and going back to the drawing board with the overlay boxes.
(In reply to Dão Gottwald [:dao] from comment #45)
> By new way I mean boxes floating on top of the content area. The closes
> thing to that is the status panel, and it's not interactive. The closes
> thing to the chat boxes done properly would be something like a download
> status bar (e.g. the Firefox add-on or the download status bar built in to
> Chrome).

Perhaps obviously, I meant "closest thing" when I wrote "closes thing" twice...
Comment on attachment 653571 [details] [diff] [review]
chat window patch

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

>+let SocialChatBar = {

As discussed on IRC, we'll need some code to handle cleaning out chat windows when social gets disabled. That might be as simple as adding a SocialChatBar.updateChats() that is called from the social:pref-changed observer, and which calls chatBar.clearAllChats if !Social.uiVisible.

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+chatbar {

>+  margin-top: -20px;

Should this be in the theme as well?

>diff --git a/browser/base/content/socialchat.xml b/browser/base/content/socialchat.xml

>+  <binding id="chatbox">

>+      <xul:iframe anonid="iframe" class="chat-frame" flex="1" mozframetype="content"
>+                  xbl:inherits="src,origin,collapsed=minimized" type="content"/>

mozframetype is only used on html:iframes, type="content here is sufficient.

>+      <constructor>

>+        let docShell = this.iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell);

this.iframe.docShell, but maybe we should just use set "mozbrowser" on the <xul:iframe> instead? I'm not sure what the difference is.

>+      <method name="toggle">

>+          // let the chat frame know if it is being shown or hidden
>+          let evt = this.iframe.contentDocument.createEvent("CustomEvent");
>+          evt.initCustomEvent(type, true, true, {});
>+          this.iframe.contentDocument.documentElement.dispatchEvent(evt);

This probably needs to handle the document not being loaded yet (when we first show the chatbox?). Or at the very least, it needs to null check documentElement.

>+  <binding id="chatbar">

>+      <method name="hideChat">

>+          Object.defineProperty(menu, "browser", { value: aChatbox, configurable: true });
>+          Object.defineProperty(aChatbox, "menu", { value: menu, configurable: true });

why not just use menu.browser = aChatbox; aChatbox.menu = menu; ? But can we avoid the references in both directions? Do we need to worry about cycles causing leaks?

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

>+function openServiceWindow(provider, contentWindow, url, name, options) {

>+  let windowName = provider.origin + name;

>-  chromeWindow.name = "social-service-window-" + name;
>+  chromeWindow.name = windowName;

This change seems unnecessary?
Attachment #653571 - Flags: review?(gavin.sharp) → review-
will update for the other items, but to responses below.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #47)
> Comment on attachment 653571 [details] [diff] [review]
> chat window patch

> >+      <method name="toggle">
> 
> >+          // let the chat frame know if it is being shown or hidden
> >+          let evt = this.iframe.contentDocument.createEvent("CustomEvent");
> >+          evt.initCustomEvent(type, true, true, {});
> >+          this.iframe.contentDocument.documentElement.dispatchEvent(evt);
> 
> This probably needs to handle the document not being loaded yet (when we
> first show the chatbox?). Or at the very least, it needs to null check
> documentElement.

toggle is called from the toggle button in the chat window title bar.

> >diff --git a/toolkit/components/social/MozSocialAPI.jsm b/toolkit/components/social/MozSocialAPI.jsm
> 
> >+function openServiceWindow(provider, contentWindow, url, name, options) {
> 
> >+  let windowName = provider.origin + name;
> 
> >-  chromeWindow.name = "social-service-window-" + name;
> >+  chromeWindow.name = windowName;
> 
> This change seems unnecessary?

It is actually fixing a bug, windowName is defined higher up in that function and is used to create the window, but is using provider.origin + name.
(In reply to Shane Caraveo (:mixedpuppy) from comment #48)
> toggle is called from the toggle button in the chat window title bar.

Ah, so we don't fire the event against the window when it's first opened? Makes sense, I guess.

> It is actually fixing a bug, windowName is defined higher up in that
> function and is used to create the window, but is using provider.origin +
> name.

All that is new in this patch, though. What was the bug with the existing code?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #49)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #48)
> > toggle is called from the toggle button in the chat window title bar.
> 
> Ah, so we don't fire the event against the window when it's first opened?
> Makes sense, I guess.

right.

> > It is actually fixing a bug, windowName is defined higher up in that
> > function and is used to create the window, but is using provider.origin +
> > name.
> 
> All that is new in this patch, though. What was the bug with the existing
> code?

sorry, didn't read your comment closely.  I was being preemptive on multiprovider by including the provider.origin as part of the window name.
Attached patch chat window patch (obsolete) — Splinter Review
updated from comments
Attachment #653571 - Attachment is obsolete: true
Attachment #653594 - Flags: review?(gavin.sharp)
Attachment #653594 - Flags: review?(felipc)
Comment on attachment 653594 [details] [diff] [review]
chat window patch

I think we should use .collapsed instead of .hidden for hiding/showing the chats.
Attachment #653594 - Flags: review?(gavin.sharp) → feedback+
(In reply to Dão Gottwald [:dao] from comment #45)
> Please take a close look at attachment 650968 [details].

That's an intentionally small window, I don't think it's representative of a commonly used window size (though some telemetry for window size might be interesting!). But in any case, I was referring to the "minimized" state, which is much less intrusive than the state in that screenshot.

It's trivial to change to a chat-bar like approach if experimentation reveals that blocking content is too problematic, so there's really no reason not to try this approach out.
change to using collapsed, move removeAll to chatbar
Attachment #653594 - Attachment is obsolete: true
Attachment #653594 - Flags: review?(felipc)
Attachment #653604 - Flags: review?(gavin.sharp)
Attachment #653604 - Flags: review?(felipc)
Comment on attachment 653604 [details] [diff] [review]
chat window patch

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

>+  update: function(state) {
>+    if (state == "disabled") {
>+      this.chatbar.removeAll();

Can you make this:

if (!this.canShow)
  this.chatbar.removeAll();

?
Comment on attachment 653604 [details] [diff] [review]
chat window patch

Pushed with that change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e778a1713d9b
Attachment #653604 - Flags: review?(gavin.sharp)
Attachment #653604 - Flags: review?(felipc)
Attachment #653604 - Flags: review+
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #53)
> (In reply to Dão Gottwald [:dao] from comment #45)
> > Please take a close look at attachment 650968 [details].
> 
> That's an intentionally small window, I don't think it's representative of a
> commonly used window size (though some telemetry for window size might be
> interesting!). But in any case, I was referring to the "minimized" state,
> which is much less intrusive than the state in that screenshot.
> 
> It's trivial to change to a chat-bar like approach if experimentation
> reveals that blocking content is too problematic, so there's really no
> reason not to try this approach out.

Less intrusive doesn't mean non-intrusive. Just like the bottom being overlayed may be unproblematic for the majority of sites doesn't make it entirely unproblematic. This isn't some afternoon project, we're building a browser for the whole web. The problems here could be avoided, and there's no reason why they shouldn't be.

Why did this land without UI review? Making a mockup with a blank content area is one thing, but I'm going to be seriously disappointed if boriss gives a positive ui-review on attachment 650968 [details].
https://hg.mozilla.org/mozilla-central/rev/e778a1713d9b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to Dão Gottwald [:dao] from comment #57)
> Why did this land without UI review? Making a mockup with a blank content
> area is one thing, but I'm going to be seriously disappointed if boriss
> gives a positive ui-review on attachment 650968 [details].

This UI is disabled by default, and there's no exposed activation mechanism. We don't need to get it perfect before landing it, and landing it has the very real benefit that people can easily experiment with it and discover issues.
Attachment #650968 - Flags: ui-review?(jboriss)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.