813288 - Fennec's Doorhanger animations should follow desktop

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Desktop slides down the doorhangers and fades in them from the "anchor". Fennec zooms in the doorhangers from the "anchor". We should probably have similar approach for animations across all the products.

Comment 1

[Wesley Johnston (:wesj)]

Looks like desktop does this 20px slide in thing: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/popup.css#32

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Fix

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 741512 [details] [diff] [review]
Patch v1

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

Maybe provide a test build for Ian's feedback before pushing? Just avoid another round of tweaks later.

::: mobile/android/base/resources/values/styles.xml
@@ +427,5 @@
>          <item name="android:padding">10dp</item>
>          <item name="android:textAppearance">@style/TextAppearance</item>
>      </style>
>  
> +    <style name="Popup" />

This seems unnecessary, no?

Comment 4

[Wesley Johnston (:wesj)]

Build at:

http://people.mozilla.com/~wjohnston/menuAnim.apk

Comment 5

[Ian Barlow (:ibarlow)]

This looks awesome Wes. Can we try it with a transition that's about 50% faster?

Comment 6

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 741512 [details] [diff] [review]
> Patch v1
> 
> ::: mobile/android/base/resources/values/styles.xml
> @@ +427,5 @@
> >          <item name="android:padding">10dp</item>
> >          <item name="android:textAppearance">@style/TextAppearance</item>
> >      </style>
> >  
> > +    <style name="Popup" />
> 
> This seems unnecessary, no?

Popup.Animation means the style is inheriting from Popup (see http://developer.android.com/guide/topics/ui/themes.html#Inheritance), so the Popup style must be declared.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > Comment on attachment 741512 [details] [diff] [review]
> > Patch v1
> > 
> > ::: mobile/android/base/resources/values/styles.xml
> > @@ +427,5 @@
> > >          <item name="android:padding">10dp</item>
> > >          <item name="android:textAppearance">@style/TextAppearance</item>
> > >      </style>
> > >  
> > > +    <style name="Popup" />
> > 
> > This seems unnecessary, no?
> 
> Popup.Animation means the style is inheriting from Popup (see
> http://developer.android.com/guide/topics/ui/themes.html#Inheritance), so
> the Popup style must be declared.

Yeah, I know. We discussed this on IRC. I meant more that creating an empty style as a "namespace" is not really a good practice. I suggested simply adding a PopupAnimation style.

Comment 8

[Wesley Johnston (:wesj)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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