Closed Bug 1034067 Opened 5 years ago Closed 5 years ago

Disable arrowpanel animation in aurora/beta

Categories

(Toolkit :: XUL Widgets, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- unaffected
firefox31 + verified
firefox32 + verified
firefox33 --- affected

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

It will be re-enabled once some fixes and improvements are made.
Attachment #8450182 - Flags: review+
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
Attachment #8450182 - Flags: approval-mozilla-beta?
Attachment #8450182 - Flags: approval-mozilla-aurora?
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?
Assignee: nobody → enndeakin
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin) → needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
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.
(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!
Flags: needinfo?(philipp)
Attachment #8450182 - Flags: approval-mozilla-beta?
Attachment #8450182 - Flags: approval-mozilla-beta+
Attachment #8450182 - Flags: approval-mozilla-aurora?
Attachment #8450182 - Flags: approval-mozilla-aurora+
I have no laptop today so I won't be able to check these in today.
So, since it hasn't been checked in yet, does that mean we missed the deadline for Beta 8?
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1186559
Due to bug 1186559,
This should be uplifted to 38esr and also 40beta at least.
Flags: needinfo?(enndeakin)
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.