815937 - Search suggestions animation glitchy on some devices

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open AwesomeScreen
2) Type "he"
3) Wait for suggestions to appear
4) Type "y"

The suggestions row quickly resizes several times from a single row height to the correct height (which is 3 rows on the Galaxy S). It's hard to describe without a screencast, but it looks pretty terrible.

So far, I've only been able to reproduce this on a SGS with Android 2.2.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Give the search suggestion row its own view type

Adding some logging, I see this in onMeasure:

D/BRN     (16366): (view id=0) Google
D/BRN     (16366):     animating!
D/BRN     (16366): (view id=1) Amazon.com
D/BRN     (16366): (view id=2) Twitter
D/BRN     (16366): (view id=3) Wikipedia
D/BRN     (16366): (view id=3) Google
D/BRN     (16366):     animating!
D/BRN     (16366): (view id=2) Amazon.com
D/BRN     (16366): (view id=1) Twitter
D/BRN     (16366): (view id=0) Wikipedia

Each line shows the view ID and the search engine being displayed in that view. My assumption was that as long as the view is being displayed, ListView would use that same for any future getView() calls. Apparently, that's not guaranteed behavior; you can see that views 0-3 were used in order for the first pass, then those rows were used in reverse order for the second pass (and I never scrolled the list, so there were being displayed the whole time).

This glitchy animation happens because we first try to animate the height to one size, but then we're given another recycled view with a different size, so the animation jumps around. If we give the suggestion row its own view type, we can make sure it won't be sharing other search engine recycled views.

Comment 2

[Brian Nicholson (:bnicholson)]

Also reproducible on Galaxy Note, Android 2.3.5.

Comment 3

[Brian Nicholson (:bnicholson)]

Review ping!

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 686852 [details] [diff] [review]
Give the search suggestion row its own view type

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

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +365,5 @@
>          public int getItemViewType(int position) {
> +            int engine = getEngineIndex(position);
> +            if (engine == -1) {
> +                return ROW_STANDARD;
> +            } else if (engine == 0 && mSuggestionsEnabled) {

Is it really safe to assume only the first engine has suggestions? Just checking.

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 686852 [details] [diff] [review]
> Give the search suggestion row its own view type
> 
> Review of attachment 686852 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/awesomebar/AllPagesTab.java
> @@ +365,5 @@
> >          public int getItemViewType(int position) {
> > +            int engine = getEngineIndex(position);
> > +            if (engine == -1) {
> > +                return ROW_STANDARD;
> > +            } else if (engine == 0 && mSuggestionsEnabled) {
> 
> Is it really safe to assume only the first engine has suggestions? Just
> checking.

Good point - I think it's possible for custom installed search engines to include a suggestion URL, in which case those would likely exhibit the same bug. This should at least fix the behavior for the vast majority of users who won't install such engines. I've filed bug 820107 as a follow-up for other engines.

Comment 6

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d55211c8eba4

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 686852 [details] [diff] [review]
Give the search suggestion row its own view type

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 769145 (Fx18)
User impact if declined: search suggestions animation is broken on certain devices, causing the row to flicker
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 686852 [details] [diff] [review]
Give the search suggestion row its own view type

Low risk flicker fix, and if this regresses anything in our testing, we'll make sure to back out.

Comment 10

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea923e42cde3

Comment 11

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e689241c7283

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e689241c7283