Closed Bug 1102275 Opened 10 years ago Closed 9 years ago

Search history entries from Search Activity changes its sizes

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox35 verified, firefox36 verified, fennec35+)

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- verified
firefox36 --- verified
fennec 35+ ---

People

(Reporter: cos_flaviu, Assigned: Margaret)

References

Details

Attachments

(2 files)

Environment: 
Device: HTC ONE M7 (Android 4.2.2);
Build: Nightly 36.0a1 (2014-11-22);

Steps to reproduce:
1. Launch Search Activity;
2. Perform a search long enough to occupy at least 2 rows in the search history;
3. Perform a search no longer that 2 to 3 words.
4. Swipe to remove the long search from the history.

Expected result:
The search entry is removed and the next search entry slides up without modifying its size.

Actual result:
After the search entry is removed from history the next search entry takes its size.

Notes:
Please check the attached screenshot.
Component: Screencasting → Search Activity
Blocks: search
tracking-fennec: --- → ?
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 35+
I was able to reproduce this. I looks like it's a view recycling problem, like we're re-using the tall view that was dismissed.

bnicholson, do we need to make sure to reset the height on the view after the transition is done? Can you help investigate this bug? I don't think this is very high priority, sine it's an edge case and the app is still usable, but it would be good to fix if it's a problem in code we're sharing elsewhere.
Flags: needinfo?(bnicholson)
We store the height at [1], then reset the height when the view is recycled at [2]. I think the problem is that we call getHeight() at [1], which gives us a fixed height value, meaning it will never be reset to WRAP_CONTENT at [2].

What happens if you try changing this:
    final int originalHeight = dismissView.getHeight();
to this:
    final int originalHeight = lp.height;
? I think LayoutParams height will return the WRAP_CONTENT/MATCH_PARENT constants instead of a fixed value, so hopefully that will carry over when we recycle it.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/SwipeDismissListViewTouchListener.java#329
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/SwipeDismissListViewTouchListener.java#179
Flags: needinfo?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> We store the height at [1], then reset the height when the view is recycled
> at [2]. I think the problem is that we call getHeight() at [1], which gives
> us a fixed height value, meaning it will never be reset to WRAP_CONTENT at
> [2].
> 
> What happens if you try changing this:
>     final int originalHeight = dismissView.getHeight();
> to this:
>     final int originalHeight = lp.height;
> ? I think LayoutParams height will return the WRAP_CONTENT/MATCH_PARENT
> constants instead of a fixed value, so hopefully that will carry over when
> we recycle it.

Thanks for the helpful info. Trying out the change locally appears to break the animation, since we do want to animate from that calculated height. I tried using dismissView.getHeight() in the ValueAnimator, and then lp.height in the onAnimationEnd listener, but then that didn't properly reset the height of the view (it looks like it stays scrunched to 1px). I can keep playing around with this to see if I come to a solution.
(In reply to :Margaret Leibovic from comment #3)
> (In reply to Brian Nicholson (:bnicholson) from comment #2)
> > We store the height at [1], then reset the height when the view is recycled
> > at [2]. I think the problem is that we call getHeight() at [1], which gives
> > us a fixed height value, meaning it will never be reset to WRAP_CONTENT at
> > [2].
> > 
> > What happens if you try changing this:
> >     final int originalHeight = dismissView.getHeight();
> > to this:
> >     final int originalHeight = lp.height;
> > ? I think LayoutParams height will return the WRAP_CONTENT/MATCH_PARENT
> > constants instead of a fixed value, so hopefully that will carry over when
> > we recycle it.
> 
> Thanks for the helpful info. Trying out the change locally appears to break
> the animation, since we do want to animate from that calculated height. I
> tried using dismissView.getHeight() in the ValueAnimator, and then lp.height
> in the onAnimationEnd listener, but then that didn't properly reset the
> height of the view (it looks like it stays scrunched to 1px). I can keep
> playing around with this to see if I come to a solution.

Oh, I'm dumb, I was holding onto a reference to the layout params, and then the height changed.

Patch forthcoming...
Attachment #8529225 - Flags: review?(bnicholson) → review+
This works for me locally, but I'll request uplift once this is verified.
Keywords: verifyme
https://hg.mozilla.org/mozilla-central/rev/c389376589b5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Verified as fixed in build 36.0a1 2014-11-27;
Device: Nexus 4 (Android 4.4.4).
Comment on attachment 8529225 [details] [diff] [review]
Use layout params height instead of the dismiss view's calculated height when resetting the original height

Approval Request Comment
[Feature/regressing bug #]: bug 1030896
[User impact if declined]: search history items in search activity may appear with the wrong height
[Describe test coverage new/current, TBPL]: no automated tests, tested locally and on nightly
[Risks and why]: low-risk, tweaking animation in search activity 
[String/UUID change made/needed]: none
Attachment #8529225 - Flags: approval-mozilla-aurora?
Comment on attachment 8529225 [details] [diff] [review]
Use layout params height instead of the dismiss view's calculated height when resetting the original height

36 is aurora now.
I guess you meant beta!
Attachment #8529225 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8529225 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed in Firefox 35 beta 1 build 2;
Device: Nexus 4 (Android 4.4.4).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.