Issue 14886012: Implement layout transition animations for the Autofill dialog

Issue 14886012: Implement layout transition animations for the Autofill dialog (Closed)

Created:
7 years, 7 months ago by Yusuf

Modified:
7 years, 7 months ago

Reviewers:
Ted C, David Trainor- moved to gerrit

CC:
chromium-reviews, Raman Kakilate, benquan, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Implement layout transition animations for the Autofill dialog Starts using the Dialog as the base class and rewrites AutofillDailogCOntentView as AutofillDialogView that contains the title and the footer with the buttons. BUG=180980 NOTRY=true Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200092

Patch Set 1 #

Patch Set 2 : Got rid of TABs #

Total comments: 50

Patch Set 3 : Ted's comments #

Patch Set 4 : Forgot to add new res folder to commit #

Patch Set 5 : Fixed rotation logic, wasn't calling super. #

Total comments: 2

Patch Set 6 : Added right resizing for keyboard appearing #

Patch Set 7 : Forgotten nits #

Patch Set 8 : findbugs errors #

Created: 7 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+588 lines, -793 lines)			Patch
M	chrome/android/java/res/layout/autofill_dialog_content.xml	View	1 2	1 chunk	+56 lines, -24 lines	0 comments	Download
M	chrome/android/java/res/layout/autofill_dialog_title.xml	View	1 2 3 4 5	1 chunk	+25 lines, -19 lines	0 comments	Download
M	chrome/android/java/res/layout/autofill_editing_layout_credit_card.xml	View	1 2 3 4 5	2 chunks	+25 lines, -32 lines	0 comments	Download
M	chrome/android/java/res/layout/autofill_editing_layout_shipping.xml	View		1 chunk	+27 lines, -34 lines	0 comments	Download
M	chrome/android/java/res/layout/autofill_general_layout.xml	View	1 2 3 4 5	1 chunk	+34 lines, -43 lines	0 comments	Download
M	chrome/android/java/res/layout/autofill_menu_item.xml	View	1 2 3 4 5	1 chunk	+1 line, -2 lines	0 comments	Download
A	chrome/android/java/res/layout/autofill_simple_menu_item.xml	View	1 2 3 4 5	1 chunk	+33 lines, -0 lines	0 comments	Download
A +	chrome/android/java/res/values-land/dimens.xml	View	1 2 3	1 chunk	+3 lines, -3 lines	0 comments	Download
M	chrome/android/java/res/values/dimens.xml	View	1 2 3 4 5	1 chunk	+5 lines, -0 lines	0 comments	Download
M	chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java	View	1 2 3 4 5 6 7	22 chunks	+77 lines, -56 lines	0 comments	Download
D	chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogContentView.java	View	1 2	1 chunk	+0 lines, -522 lines	0 comments	Download
M	chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogTitleView.java	View		2 chunks	+8 lines, -9 lines	0 comments	Download
M	chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogUtils.java	View		2 chunks	+24 lines, -2 lines	0 comments	Download
A +	chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java	View	1 2 3 4 5 6 7	14 chunks	+270 lines, -47 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

Yusuf

I still need to fix orientation changes, but that will just be a change in ...

7 years, 7 months ago (2013-05-06 20:30:25 UTC) #1

Ted C

https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java (right): https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java#newcode288 chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:288: private void setButton(int which, String text, android.view.View.OnClickListener listener) { ...

7 years, 7 months ago (2013-05-07 21:49:07 UTC) #3

Yusuf

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java (right): https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java#newcode288 chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:288: private void setButton(int which, String text, android.view.View.OnClickListener listener) { ...

7 years, 7 months ago (2013-05-08 21:39:00 UTC) #4

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java
(right):

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:288:
private void setButton(int which, String text, android.view.View.OnClickListener
listener) {
On 2013/05/07 21:49:07, Ted C wrote:
> I don't think you need the android.view.View before OnClickListener

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:290:
assert(button != null);
On 2013/05/07 21:49:07, Ted C wrote:
> Should handle the case where it's null gracefully but leave the assert for
> debugging.

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:299:
button = (Button) mView.findViewById(R.id.positive_button);
On 2013/05/07 21:49:07, Ted C wrote:
> I would either return directly in these conditions without saving a local
button
> variable.  Or have the local variable be the resource ID and share the same
> lookup code.

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialog.java:794:
return mFocusedField;
On 2013/05/07 21:49:07, Ted C wrote:
> looks like you added a space here that messes up the indent

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java
(right):

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:132:
for (int i = 0; i < NUM_SECTIONS; i++) {
This is just to avoid getting to a state where we are animating to a layout we
haven't measured before. These layout newer change their widths and heights
depending on user input. I was worried measuring them every time would be
inefficient and unnecessary.

On 2013/05/07 21:49:07, Ted C wrote:
> should these go in measure instead?

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:139:
public void onLayoutChange(View arg0, int arg1, int arg2, int arg3, int arg4,
int arg5,
On 2013/05/07 21:49:07, Ted C wrote:
> get the argument names from the sdk so they're not so obfuscated

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:179:
R.dimen.autofill_dialog_max_height_portrait);
On 2013/05/07 21:49:07, Ted C wrote:
> Hmm...can you use the same dimension name but use the resource filtering to
> provide you a different one based on the current orientation?  We currently
> don't have a values-land/ dir, but we could?

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:188:
if (mCurrentOrientation != newConfig.orientation) {
On 2013/05/07 21:49:07, Ted C wrote:
> I would check for equality and early out instead since this is all in one if
> block

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:200:
if (!mTransitionAnimationPlaying) return super.drawChild(canvas, child,
drawingTime);
On 2013/05/07 21:49:07, Ted C wrote:
> Maybe make this:
> 
> !mTransitionAnimationPlaying || child.getId() != R.id.content
> 
> Then you can remove the if check below as well as the final super.drawChild

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:202:
float top = mContent.getTop()
On 2013/05/07 21:49:07, Ted C wrote:
> These aren't used outside of the if block, so probably should live in there
> (unless you remove it as I mention above).

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:220:
mTitle.setTranslationY(mContent.getMeasuredHeight() / 2 - mVisibleContentHeight
/ 2);
On 2013/05/07 21:49:07, Ted C wrote:
> maybe extra some of these as variables and share them

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:222:
mFooter.setTranslationY(- mContent.getMeasuredHeight() / 2 +
mVisibleContentHeight / 2);
On 2013/05/07 21:49:07, Ted C wrote:
> unnecessary space between - and mContent

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:226:
mBackground.setBounds(getLeft(),(int) (mTitle.getTop() +
mTitle.getTranslationY())
On 2013/05/07 21:49:07, Ted C wrote:
> space after getLeft()

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:227:
, getRight(),(int) (mFooter.getBottom()+mFooter.getTranslationY()));
On 2013/05/07 21:49:07, Ted C wrote:
> leading comma should go on the previous line.  Spaces before and after +

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:292:
measuredContentHeight = mSteadyLayout.getMeasuredHeight();
Yes ,they are always in the hierarchy. This measures how long they would like to
be if parent is not restricting them. So it will be the same when we rotate
also.


On 2013/05/07 21:49:07, Ted C wrote:
> What happens if you rotate?  Will all these measured heights be wrong?  (or
are
> all the views in the hierarchy so they're get resized correctly).

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:301:
layout.measure(widthSpec, heightSpec);
On 2013/05/07 21:49:07, Ted C wrote:
> Maybe a TODO to only do this if the mode that was measured before and is
wrong?

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:312:
e.printStackTrace();
On 2013/05/07 21:49:07, Ted C wrote:
> probably should remove this :-)
> 
> and for future reference:
> http://developer.android.com/reference/java/lang/Thread.html#dumpStack%28)

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:319:
mContent.setTranslationY(0);
On 2013/05/07 21:49:07, Ted C wrote:
> why do you need to reset these here?  Maybe comment why.

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:330:
long content = SystemClock.uptimeMillis();
On 2013/05/07 21:49:07, Ted C wrote:
> not used?

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:338:
setMeasuredDimension(mFrameWidth, mFrameHeight);
On 2013/05/07 21:49:07, Ted C wrote:
> why use getWidth() above and mFrameWidth here?

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:345:
int space = (getMeasuredHeight() - totalHeight) / 2 ;
On 2013/05/07 21:49:07, Ted C wrote:
> I would rename space to specify something about the verticalness of it.

Done.

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:496:
ObjectAnimator.ofFloat(this, "VisibleContentHeight", fromHeight, toHeight);
I am not sure if the objectAnimator framework uses reflection with this or not.
The SDK says you should be defining a getter and setter and set them as public.
Is there anything I am missing here?


On 2013/05/07 21:49:07, Ted C wrote:
> Maybe use a Property to avoid the reflection?
> 
>     private final Property<AutofillDialogView, Float> mContentPropertyHeight =
>             new Property<AutofillDialogView, Float>(Float.class, "") {
>         @Override
>         public Float get(AutofillDialogView view) {
>             return view.getVisibleContentHeight();
>         }
> 
>         @Override
>         public void set(AutofillDialogView view, Float height) {
>             view.setVisibleContentHeight(height);
>         }
>     };

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:501:
invalidate();
The header and the footer is also moving, so every child of the parent is moving
and we have to redraw everything.


On 2013/05/07 21:49:07, Ted C wrote:
> If you can, it should be "slightly" faster to invalidate just the dirty rect
> that needs to be drawn

https://codereview.chromium.org/14886012/diff/2001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:670:
int layoutID) {
On 2013/05/07 21:49:07, Ted C wrote:
> lowercase d in layoutId

Done.

David Trainor- moved to gerrit

A few small comments. One change requested about the object animator. https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java (right): ...

7 years, 7 months ago (2013-05-10 22:59:55 UTC) #5

A few small comments.  One change requested about the object animator.

https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java
(right):

https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:496:
ObjectAnimator.ofFloat(this, "VisibleContentHeight", fromHeight, toHeight);
It uses reflection by looking for a method called "get" + "VisibleContentHeight"
and then "set" + "VisibleContentHeight".  It's just done behind the scenes.  You
should probably use the property.

On 2013/05/08 21:39:00, Yusuf wrote:
> I am not sure if the objectAnimator framework uses reflection with this or
not.
> The SDK says you should be defining a getter and setter and set them as
public.
> Is there anything I am missing here?
> 
> 
> On 2013/05/07 21:49:07, Ted C wrote:
> > Maybe use a Property to avoid the reflection?
> > 
> >     private final Property<AutofillDialogView, Float> mContentPropertyHeight
=
> >             new Property<AutofillDialogView, Float>(Float.class, "") {
> >         @Override
> >         public Float get(AutofillDialogView view) {
> >             return view.getVisibleContentHeight();
> >         }
> > 
> >         @Override
> >         public void set(AutofillDialogView view, Float height) {
> >             view.setVisibleContentHeight(height);
> >         }
> >     };
>

https://chromiumcodereview.appspot.com/14886012/diff/25001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java
(right):

https://chromiumcodereview.appspot.com/14886012/diff/25001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:295:
protected void onMeasure (int widthMeasureSpec, int heightMeasureSpec) {
No space between onMeasure and (

Yusuf

Also fixed resizing for keyboard. https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java (right): https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java#newcode496 chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:496: ObjectAnimator.ofFloat(this, "VisibleContentHeight", fromHeight, toHeight); ...

7 years, 7 months ago (2013-05-14 02:15:10 UTC) #6

Also fixed resizing for keyboard.

https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java
(right):

https://chromiumcodereview.appspot.com/14886012/diff/2001/chrome/android/java...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:496:
ObjectAnimator.ofFloat(this, "VisibleContentHeight", fromHeight, toHeight);
On 2013/05/10 22:59:55, David Trainor wrote:
> It uses reflection by looking for a method called "get" +
"VisibleContentHeight"
> and then "set" + "VisibleContentHeight".  It's just done behind the scenes. 
You
> should probably use the property.
> 
> On 2013/05/08 21:39:00, Yusuf wrote:
> > I am not sure if the objectAnimator framework uses reflection with this or
> not.
> > The SDK says you should be defining a getter and setter and set them as
> public.
> > Is there anything I am missing here?
> > 
> > 
> > On 2013/05/07 21:49:07, Ted C wrote:
> > > Maybe use a Property to avoid the reflection?
> > > 
> > >     private final Property<AutofillDialogView, Float>
mContentPropertyHeight
> =
> > >             new Property<AutofillDialogView, Float>(Float.class, "") {
> > >         @Override
> > >         public Float get(AutofillDialogView view) {
> > >             return view.getVisibleContentHeight();
> > >         }
> > > 
> > >         @Override
> > >         public void set(AutofillDialogView view, Float height) {
> > >             view.setVisibleContentHeight(height);
> > >         }
> > >     };
> > 
> 

Done.

https://chromiumcodereview.appspot.com/14886012/diff/25001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java
(right):

https://chromiumcodereview.appspot.com/14886012/diff/25001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogView.java:295:
protected void onMeasure (int widthMeasureSpec, int heightMeasureSpec) {
On 2013/05/10 22:59:55, David Trainor wrote:
> No space between onMeasure and (

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/14886012/54001

7 years, 7 months ago (2013-05-14 22:53:36 UTC) #8

Message was sent while issue was closed.

Change committed as 200092

Expand Messages | Collapse Messages