Closed Bug 1043027 Opened 7 years ago Closed 7 years ago

Refine clear button hiding/showing

Categories

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

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(2 files)

Feedback from bug 1041738.
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
^ I meant (the new tool bar)'s 'x' icon appearing animation
I'll expand the scope of this bug to cover the animation in addition to the visible/hidden states.
Summary: Clear button should be hidden when there is no text in the search bar → Refine clear button hiding/showing
Attached video search_suggexpand2.mov
attaching .mov for reference to animation
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?
(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.
Assignee: nobody → 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.
Flags: needinfo?(alam)
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!
Flags: needinfo?(alam)
Status: NEW → ASSIGNED
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.
Priority: -- → P1
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.
Attachment #8472608 - Flags: review?(wjohnston)
Depends on: 1053491
Comment on attachment 8472608 [details] [diff] [review]
Only show clear button when there is text in the search bar

Eric could also review this.
Attachment #8472608 - Flags: review?(eric.edens)
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.
Attachment #8472608 - Flags: review?(eric.edens) → review+
Attachment #8472608 - Flags: review?(wjohnston)
(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 :)
https://hg.mozilla.org/mozilla-central/rev/89304862b06e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.