1043027 - Refine clear button hiding/showing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

Feedback from bug 1041738.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Just wanted to note that there is also an animation that is triggered when the 'x' pops up.

Same parameters as the new tool bar outlined in bug 1014848

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ I meant (the new tool bar)'s 'x' icon appearing animation

Comment 3

[:Margaret Leibovic]

I'll expand the scope of this bug to cover the animation in addition to the visible/hidden states.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: search_suggexpand2.mov

attaching .mov for reference to animation

Comment 5

[:Margaret Leibovic]

So, I'm seeing two main animations here:

1) Close button growing/shrinking: this should be straightforward to implement, since we can animate the scaleX/scaleY instead of just changing the visibility immediately like we're currently doing.

2) Orange border line growing/shrinking: this might be trickier to implement, since that line is currently drawn as a background on the EditText. In order to animate this, we may need to replace that background drawable with a separate view. Also, how will this work with the current progress bar idea we're pursuing in bug 1042937? Should we hold off on adding this animation until we change the progress style as well?

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #5)
> So, I'm seeing two main animations here:
> 
> 1) Close button growing/shrinking: this should be straightforward to
> implement, since we can animate the scaleX/scaleY instead of just changing
> the visibility immediately like we're currently doing.

Yes! but there's a also slight bounce to it as well that makes it feel more "personal" in practice 

> 2) Orange border line growing/shrinking: this might be trickier to
> implement, since that line is currently drawn as a background on the
> EditText. In order to animate this, we may need to replace that background
> drawable with a separate view. Also, how will this work with the current
> progress bar idea we're pursuing in bug 1042937? Should we hold off on
> adding this animation until we change the progress style as well?

I think we could do this independent of the temporary orange loading laser show. I do realize how tricky the bounce can be (I'm trying to use the same bounce with Lucas in our tool bar efforts), but since it also disappears as soon as the user submits a search, I think it should be fine.

Comment 7

[:Margaret Leibovic]

I made a WIP pull request here:
https://github.com/ericedens/FirefoxSearch/pull/40

And here's a link to the APK:
http://people.mozilla.org/~mleibovic/search-anim.apk

I used a BounceInterpolator when then clear button expands, but kept the default linear transition when it shrinks.

This animation seems to stutter a bit on my Nexus 4, so maybe I'll need to look into how to improve the performance here.

I also noticed that this doesn't match with the video from bug 1042189 - the search bar should transition to inactive as soon as the search suggestion is tapped. I think that was probably just an oversight while implementing the patch for bug 1042189, and I can address that separately.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yeah I noticed it wasn't smooth on my Nexus 5 either. It's getting there though! 

I know this is a WIP but just thought I'd mention a couple points of feedback:
 - the icon sort of shrinks to the bottom left corner when it dismisses? the point should be in the center
 - It's appearing for me as soon as I activate the input box (before typing) so I think hitting the 'x' at that point should dismiss the keyboard too (might not be in the scope of this build :P)

That being said, where this appears: after the first character is typed vs. as soon as when the input box is active really seems to change user expectation for the 'x' (just thought i'd make a note of that here so I remember :)

I'll keep testing!

Comment 9

[:Margaret Leibovic]

P1 for showing/hiding at the correct time. The animation is a nice touch, but if we can't get it to perform well, we don't need to worry about it right now.

Comment 10

[:Margaret Leibovic]

Attachment: Only show clear button when there is text in the search bar

I played around with animating this, but I think it will take some work to get it right, so I'd rather just land this functional change now, and animate it in a follow-up bug.

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ sounds good to me!

Comment 12

[:Margaret Leibovic]

Comment on attachment 8472608 [details] [diff] [review]
Only show clear button when there is text in the search bar

Eric could also review this.

Comment 13

[Eric Edens [:eedens]]

Comment on attachment 8472608 [details] [diff] [review]
Only show clear button when there is text in the search bar

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

LGTM.

There was a reject when merging against fx-team; but it looks like a rebase against https://hg.mozilla.org/integration/fx-team/rev/35343a2e0abd will fix it.

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/89304862b06e

I'll let you merge a PR.

Comment 15

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #14)
> https://hg.mozilla.org/integration/fx-team/rev/89304862b06e
> 
> I'll let you merge a PR.

Oops, this is my bug! I'll do my own PR :)

Comment 16

[:Margaret Leibovic]

https://github.com/mozilla/fennec-search/commit/785b84566f1256f399f3e9232f92ebf3c46a89e5

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/89304862b06e