1034067 - Disable arrowpanel animation in aurora/beta

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

Attachment: Disable animation, already reviewed from bug 994117

It will be re-enabled once some fixes and improvements are made.

Comment 1

[Neil Deakin]

Comment on attachment 8450182 [details] [diff] [review]
Disable animation, already reviewed from bug 994117

Approval Request Comment
[Feature/regressing bug #]: This is a backout of part of bug 610545. It reverts back to the original animation. 
[User impact if declined]: Because of some issues with the animation, it is disabled on the main toolbar panel. This is felt to be inconsistent, so a backout is suggestion to go back to the old behaviour.
[Describe test coverage new/current, TBPL]: tested at https://tbpl.mozilla.org/?tree=Try&rev=ea00e0596341
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

It's very late in beta to be discussing a feature backout. So, some questions:

(In reply to Neil Deakin from comment #1)
> [Feature/regressing bug #]: This is a backout of part of bug 610545. It
> reverts back to the original animation. 
Is this a complete and clean backout? 

> [User impact if declined]: Because of some issues with the animation, it is
> disabled on the main toolbar panel. This is felt to be inconsistent, so a
> backout is suggestion to go back to the old behaviour.
Who feels this is inconsistent? How bad is the inconsistency? As well, how was this issue identified (beta feedback?) and why wasn't it identified earlier?

> [Describe test coverage new/current, TBPL]: tested at
> https://tbpl.mozilla.org/?tree=Try&rev=ea00e0596341
Have you applied the patch and tested the backout yourself to ensure the animation behaviour has correctly been reverted?

Comment 3

[Neil Deakin]

I think Philipp would be better to answer the second question. I personally don't think a backout is necessary,

> Have you applied the patch and tested the backout yourself to ensure the animation
> behaviour has correctly been reverted?

Yes, on beta.

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Lawrence Mandel [:lmandel] from comment #2)
> > [User impact if declined]: Because of some issues with the animation, it is
> > disabled on the main toolbar panel. This is felt to be inconsistent, so a
> > backout is suggestion to go back to the old behaviour.
> Who feels this is inconsistent? How bad is the inconsistency? As well, how
> was this issue identified (beta feedback?) and why wasn't it identified
> earlier?

Not having any animation on the main panel is a visible regression by itself. Changing the animation elsewhere makes it even more visible.
As to why this wasn't identified earlier: it was. I first started bugging people about this when the original patch landed and then again when it hit Aurora. Evidently I wasn't persistent enough.
On the bright side: I'm in an active conversation with Gavin about setting up a lightweight process to make sure these things don't happen again. The current plan is to have a short meeting on uplift to identify and back out any patches from Aurora that aren't finished yet or make more sense to ship together with other (later) patches.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Philipp Sackl [:phlsa] from comment #4)
> Not having any animation on the main panel is a visible regression by
> itself. Changing the animation elsewhere makes it even more visible.
Can you confirm that you, on behalf of UX, feel strongly enough that this change needs to be backed out to warrant the risk of doing so very late in the beta cycle?

> As to why this wasn't identified earlier: it was. I first started bugging
> people about this when the original patch landed and then again when it hit
> Aurora. Evidently I wasn't persistent enough.
> On the bright side: I'm in an active conversation with Gavin about setting
> up a lightweight process to make sure these things don't happen again. The
> current plan is to have a short meeting on uplift to identify and back out
> any patches from Aurora that aren't finished yet or make more sense to ship
> together with other (later) patches.
Thanks for pushing on this. Marco and Jenn should be able to help with execution. When setting up the new desktop process it was our intention to have a checkpoint with product shortly before the Aurora uplift in order to determine what changes were not ready to go to Aurora. The desire was to pref off or backout those changes before the merge.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

As Lawrence, I am a bit surprise to hear about that late in the beta cycle...
Beta 8 is going to be built tomorrow (Monday). Beta 9 is going to be too late.

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Lawrence Mandel [:lmandel] from comment #5)
> (In reply to Philipp Sackl [:phlsa] from comment #4)
> > Not having any animation on the main panel is a visible regression by
> > itself. Changing the animation elsewhere makes it even more visible.
> Can you confirm that you, on behalf of UX, feel strongly enough that this
> change needs to be backed out to warrant the risk of doing so very late in
> the beta cycle?

Yes. I hope it's not too late for Beta 8 now.


> > As to why this wasn't identified earlier: it was. I first started bugging
> > people about this when the original patch landed and then again when it hit
> > Aurora. Evidently I wasn't persistent enough.
> > On the bright side: I'm in an active conversation with Gavin about setting
> > up a lightweight process to make sure these things don't happen again. The
> > current plan is to have a short meeting on uplift to identify and back out
> > any patches from Aurora that aren't finished yet or make more sense to ship
> > together with other (later) patches.
> Thanks for pushing on this. Marco and Jenn should be able to help with
> execution. When setting up the new desktop process it was our intention to
> have a checkpoint with product shortly before the Aurora uplift in order to
> determine what changes were not ready to go to Aurora. The desire was to
> pref off or backout those changes before the merge.

That sounds good!

Comment 8

[Neil Deakin]

I have no laptop today so I won't be able to check these in today.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

So, since it hasn't been checked in yet, does that mean we missed the deadline for Beta 8?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Sorry, this slipped through the cracks somehow. Anyway, Sylvestre gave me the go-ahead to land it still.

https://hg.mozilla.org/releases/mozilla-aurora/rev/9babdeaf1bdbcd
https://hg.mozilla.org/releases/mozilla-beta/rev/dab08e1fc630

Comment 11

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified this in Firefox 31 Beta 9 (BuildID: 20140710141843) and the latest Firefox 30 Aurora (BuildID: 20140711004006), on Windows 7 x64, Windows 8 x64, Mac OS X 10.9.4, Ubuntu 13.04 x64. 

From what I've seen, the animation now looks much more like the "fade-in slide-down" in Nightly from April 2nd (before the fix for 610545 landed), then like the "fade-in slide-down growing-size" from Firefox 31 Beta 3. I've verified panels like: geolocation, security, bookmarks, downloads, Panel Menu.

Comment 12

[Alice0775 White]

Due to bug 1186559,
This should be uplifted to 38esr and also 40beta at least.