809733 - Add CSS transitions for minimizing and restoring the chatboxes

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

I talked with Stephen and he said that we should have animations between the various chatbox states (appearing, minimizing, restoring).

Comment 1

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

Attachment: Patch

This patch adds the following transitions:
minimized -> restored -> minimized (the box transitions the width and height)
minimized or restored -> closed (the box transitions the width)

I played with transitioning the opacity on close but it didn't feel right when the other chat took its place.

Comment 2

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

Comment on attachment 679793 [details] [diff] [review]
Patch

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

Missed the fadeout transition for the other platforms.

Comment 3

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

Attachment: Patch v1.1

Comment 4

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

Comment on attachment 679798 [details] [diff] [review]
Patch v1.1

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

I'll try to animate opening as well, and also I need to change the attribute name.

Comment 5

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

Attachment: Patch v1.2

I fixed the attribute name and added a transition for opening.

Comment 6

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

Attachment: Patch v1.3

I talked offline with Felipe about this patch, and I made the closing animation now do a fadeout -> width(100% -> 0%).

Also, I was able to remove the "closing" attribute by simply removing the "opened" attribute when the tab should close.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 680022 [details] [diff] [review]
Patch v1.3

>         <xul:toolbarbutton class="chat-close-button chat-toolbarbutton"
>-                           oncommand="document.getBindingParent(this).close();"/>
>+                           onclick="document.getBindingParent(this).close(event);"/>

>       <method name="close">
>+        <parameter name="aEvent"/>

>+          aEvent.stopPropagation();

Why is this needed?

You need to set the transition (or a basic transition which themes can override) in a content stylesheet such that the chat box can still be closed if a third-party theme doesn't specify a transition.

Comment 8

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

Attachment: Patch v1.4

Moved the transition out of the theme files and removed the need for stopPropagation.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 680306 [details] [diff] [review]
Patch v1.4

>         <xul:toolbarbutton class="chat-close-button chat-toolbarbutton"
>-                           oncommand="document.getBindingParent(this).close();"/>
>+                           onclick="document.getBindingParent(this).close(event);"/>

Why this change? The method doesn't actually use the event argument anymore.

Comment 10

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

Attachment: Patch v2

0:08.69 Browser Chrome Test Summary
0:08.69 Passed: 440
0:08.69 Failed: 0
0:08.69 Todo: 1

Comment 11

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

Attachment: Patch v2.1

There is a bug with either the Facebook code or in Gecko that is causing the Facebook content to fail to respond to the transitioned width of the chatbox, leaving the contents of the chatbox only displaying in half of the width of the box.

This updated patch only transitions the height of the box, which removes the aforementioned bug. The transition still looks pretty good, and the change of widths isn't very obvious/janky looking.

Comment 12

[Stephen Horlander [:shorlander]]

Comment on attachment 710725 [details] [diff] [review]
Patch v2.1

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

Still seeing the settings button and header getting clipped on OS X: http://cl.ly/image/0j0N420t0D3G

Might be worth exploring if this is a layout bug that should be fixed. If not I guess we can't purse transitions here unless we can work around it.

Comment 13

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

Attachment: Un bit-rot the transition and wait for transition to complete before notifying the content.

(In reply to Stephen Horlander from comment #12)
> Still seeing the settings button and header getting clipped on OS X:
> http://cl.ly/image/0j0N420t0D3G
> 
> Might be worth exploring if this is a layout bug that should be fixed. If
> not I guess we can't purse transitions here unless we can work around it.

I also see this on Windows, but only for Facebook.  I guessed that the problem was that the content code somehow seeing an intermediate height, so added a 'transitionend' event and only sent the SocialFrameShow/SocialFrameHide events after the transition was complete, and this solved the problem for me.

I'm attaching the simple patch that does this, and requesting feedback from Jaws to see if this is something we should develop further.  As the old patch has bit-rotted and all social styling is now in a shared.css, the patch also includes the CSS transition changes.  (It also looks nicer IMO if we also transition the width too, but I haven't done that here)

Comment 14

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

Comment on attachment 725957 [details] [diff] [review]
Un bit-rot the transition and wait for transition to complete before notifying the content.

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

Yeah if this will fix it, then let's do it! :)

Comment 15

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

<markh> jaws: so re that transition event. The risk I see is that user styles or future changes might mean the event never fires. Is that a risk worth considering?
<jaws> markh: yeah you're right. there is a way that we can do it so it doesn't affect themes, let me think about it for a moment
<markh> my first thought was a timer as well (ie, no transitionend event in 500ms? Send it anyway) That's pretty ugly though...

Comment 16

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

Ok, so the way that we could do this is to put a zero (or near zero if necessary, 0.01s) duration in the content styling. Our theme can change the duration so that it is a noticeable transition, and 3rd party themes (as long as they don't entirely remove the transition) won't be affected.

Comment 17

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

Attachment: Transition both width and height

This patch has both height and width transitions and all tests pass.

> Ok, so the way that we could do this is to put a zero (or near 
> zero if necessary, 0.01s) duration in the content styling.

I'm not sure what you mean by "in the content styling" here.  Also, a quick mxr shows there are a few things that seem to depend on transitionend to work correctly - eg:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3733

seems to rely on that event to finalize a tab close, and a number of similar examples exist.  So I guess you could either f+ this and tell me where the "default" transition should be applied, or just r+ it ;)

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond (:markh) from comment #17)
> I'm not sure what you mean by "in the content styling" here.  Also, a quick
> mxr shows there are a few things that seem to depend on transitionend to
> work correctly - eg:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3733

The transition that that depends on is defined in the "content" stylesheet (browser/base/content/browser.css), as opposed to the "skin" stylesheet (the various browser.css files under browser/themes). The former is not overridden by third-party themes, while the latter are. I think for this bug you can just do the same thing, and put this transition styling in that same browser/base/content/browser.css.

Comment 19

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

Attachment: Add transition to the content styling

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #18)
> The transition that that depends on is defined in the "content" stylesheet
> (browser/base/content/browser.css), as opposed to the "skin" stylesheet (the
> various browser.css files under browser/themes). The former is not
> overridden by third-party themes, while the latter are. I think for this bug
> you can just do the same thing, and put this transition styling in that same
> browser/base/content/browser.css.

Ahh, thanks!  This new patch does just this.

Comment 20

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

Comment on attachment 732653 [details] [diff] [review]
Add transition to the content styling

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

LGTM, sorry for the confusion.

Comment 21

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

(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #20)
> Comment on attachment 732653 [details] [diff] [review]
> Add transition to the content styling
> 
> Review of attachment 732653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, sorry for the confusion.

No need to apologize - the problem was my naievity ;)

try at https://tbpl.mozilla.org/?tree=Try&rev=2ddf99570abd

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f916756bbcbd

Comment 23

[Ed Morley [:emorley]]

Backed out for browser_social_chatwindow.js failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=21584450&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21584979&tree=Mozilla-Inbound
etc

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/091fe3675996

Comment 24

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

Attachment: Disable transitions while measuring elements

The problem we struck on some test runs is that we were measuring the width of the chatbox while it was transitioning.

This should only happen in edge-cases - eg, the test was explicitly minimizing a chat and immediately creating a new one - but in theory this could happen "for real" - eg, a new chat created by the provider at the same instant the user minimizes/restores an existing chat.

The solution I came up with was to disable the transition while we were measuring it.  This means that in the same edge-cases, the transition simply will not be seen rather than us getting the wrong value.  Note that this does *not* mean the first chat loses the transition - the transition is only lost in edge-cases, not the first time a chat is minimized or restored.

This chat measuring code is already somewhat suspect - ideally we wouldn't need to know the width of a chat and things like overflow/underflow events could drive this - however, that's currently not practical (for reasons I can elaborate on if desired).  So given we already have this measuring code and this work-around isn't really *that* bad, and that the transitions look quite nice, I think we should just go for it!

FWIW, the change between this patch and the previous one is below, and I'm re-asking for review/feedback on just this portion.  Flagging Gavin and Jaws as the change may be contentious.

       <method name="calcTotalWidthOf">
         <parameter name="aElement"/>
         <body><![CDATA[
+          // Prevent transitions to avoid getting an "intermediate" width...
+          aElement.style.transition = "none";
           let cs = document.defaultView.getComputedStyle(aElement);
           let margins = parseInt(cs.marginLeft) + parseInt(cs.marginRight);
-          return aElement.getBoundingClientRect().width + margins;
+          let result = aElement.getBoundingClientRect().width + margins;
+          aElement.style.removeProperty("transition");
+          return result;

Comment 25

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

Comment on attachment 737314 [details] [diff] [review]
Disable transitions while measuring elements

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

::: browser/base/content/socialchat.xml
@@ +144,5 @@
>          // we made it this far, use it
>          this.setAttribute('image', uri.spec);
>          this.parentNode.updateTitlebar(this);
>        ]]></handler>
> +      <handler event="transitionend">

Are we sure that the transitionend event will be fired in all cases still? The other option that we could do would be to duplicate the chatbox size in both the CSS file and in this file.

Comment 26

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

Attachment: Duplicate width in css and code.

OK, here is a version with duplicated values.  The CSS for width and height was moved to content CSS and these defined as consts in the function that uses them (I don't see how they could be added to the top of the file as would be common with modules etc).

Comment 27

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

Comment on attachment 737317 [details] [diff] [review]
Duplicate width in css and code.

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

As discussed on IRC, the net-benefit of the transition here exceeds the loss of ability for themes to change the dimensions of the chatboxes.

Comment 28

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b29cbd75a2df

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b29cbd75a2df

Comment 30

[u279076]

Verified fixed in Firefox Nightly 23.0a1 2013-04-18.

Comment 31

[Asa Dotzler [:asa]]

So hot! Thank you Jared and Mark!