Closed Bug 815937 Opened 8 years ago Closed 8 years ago

Search suggestions animation glitchy on some devices

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- verified
b2g18 --- fixed
fennec 20+ ---

People

(Reporter: bnicholson, Assigned: bnicholson)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → bnicholson
tracking-fennec: ? → 20+
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.
Attachment #686852 - Flags: review?(lucasr.at.mozilla)
Also reproducible on Galaxy Note, Android 2.3.5.
Review ping!
Flags: needinfo?(lucasr.at.mozilla)
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.
Attachment #686852 - Flags: review?(lucasr.at.mozilla) → review+
Flags: needinfo?(lucasr.at.mozilla)
Depends on: 820107
(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.
https://hg.mozilla.org/mozilla-central/rev/d55211c8eba4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
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
Attachment #686852 - Flags: approval-mozilla-beta?
Attachment #686852 - Flags: approval-mozilla-aurora?
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.
Attachment #686852 - Flags: approval-mozilla-beta?
Attachment #686852 - Flags: approval-mozilla-beta+
Attachment #686852 - Flags: approval-mozilla-aurora?
Attachment #686852 - Flags: approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.