868222 - Tab increment animation displays an artifact every other count change.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Screenshot: artifact in tab increment #

When loading a non-about:home page, increasing tab count from 1 to 2, a thin white line flashes on the edge and leaves an artifact. No idea why this doesn't happen in subsequent tab increments. This artifact doesn't remain when loading about:home, although there is still a flash of a thin white line.

STR:
1. Have a single tab open.
2. Click on "..." and select "+ New Tab"
3. Select something (not about:home) from list

Expected:
Clean tab animation.

Actual:
White line flashes and leaves an artifact.

Nightly 5/02/2013 on HTC One V, running 4.0.3

Comment 1

[Lucas Rocha (:lucasr)]

Might be related to hw layers in toolbar animations. Or maybe clipping-related. Wes?

Comment 2

[Wesley Johnston (:wesj)]

Hmm.. The new transition requires that the textViews actually move outside the button bounds, so I had to turn off clipping. It should like Android is just not invalidating correctly. Maybe we should invalidate the entire bar after the transform is over... Will play.

Comment 3

[Mark Capella [:capella]]

Hey guys, I can repro this on my S3 ... I found by trial and error if you tweak the CENTER_Y value here from 1.25f to 1.12f, the issue clears up:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabCounter.java#25

My STR was:

Start FF, and wait for the about:home screen to completely load (the awesomebar icon turns into the blue nightly version)

1) Long tap a Top Site tile and click "Open in New Tab"
2) TabCounter value animates and changes to "2" leaving the artifact ...

Interestingly, if you repeat steps 1 and 2, when TabCounter animates into "3", there will be no artifact ...

Repeat 1 and 2 for TabCounter "4" produces the artifact
Repeat 1 and 2 for TabCounter "5" does not produce the artifact
...
and so on
...

Comment 4

[Mark Capella [:capella]]

Attachment: Artifact as I saw it ...

Comment 5

[Mark Capella [:capella]]

Attachment: Patch (v1)

The UI changed a little over the last month but the TabCounter artifact / glitch is basically the same / still exists ... 

This is the patch I mentioned in previous comment ... it's pretty simple but works without being too hacky / special invalidations etc ... do you see a better way to repair it?

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 756905 [details] [diff] [review]
Patch (v1)

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

This will change the animation slightly, right? I'd like to know why it fixes what it fixes. Can you look at some of the clipChildren properties on the awesomebar resources? I suspect that since we're not clipping children here, Android isn't properly invalidating things.

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v2)

Is it important to understand it if I can just make it go away? :-P Ok, forget about the hacky Patch (v1) ... I don't like it either.

I looked at the clipChildren and clipToPadding properties in TabCounters' TabCount <style> and that of its parent <BrowserToolbarLayout> as you added in bug 863828 and couldn't see anything wrong.

I did dig further into the underlying classes though and found an explanation for what I mentioned in comment 3 where slow motion testing of the in / out animations looked different and displayed the artifact on every-other execution.

Even though the TabCounter explicitly sets In / Out animation each time here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabCounter.java#73, its extended class ViewAnimator keeps track of the last animated child View (mWhichChild).

The first call to setText() to trigger the animation displays (in order but interleaved) View 1 (our In animation) then View 2 (our Out animation) in the ViewAnimators' setDisplayedChild() member.

The next call to setText() to trigger the animation displays (in order but interleaved) View 2 (our Out animation) then View 1 (our In animation), and so on each susbsequent execution.

This mucks something up internally (maybe how the function tries to handle child focus). This new patch explicitly sets the displayedChild order, and corrects the problem more gracefully, while not producing the slight change to animation that you correctly point out in comment 7.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 759009 [details] [diff] [review]
Patch (v2)

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

Nice find.

::: mobile/android/base/TabCounter.java
@@ +84,5 @@
> +            setOutAnimation(mFlipOutBackward);
> +        }
> +
> +        setDisplayedChild(0);
> +        setCurrentText(String.valueOf(mCount));

Do we need to do this before the setDisplayedChild call? Also, lets add a comment explaining why we have to do this explicitly.

Comment 10

[Mark Capella [:capella]]

If I understand the question, no we can't ... reversing the two lines and doing:

> +        setCurrentText(String.valueOf(mCount));
> +        setDisplayedChild(0);

Then for multiple long-tap / Open Link in New Tab's, User sees TabCounter 1->2, 1->3, 1->4 ...

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v3)

Comment 12

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=947506e7fc18

Comment 13

[Mark Capella [:capella]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9387dd4af2e2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9387dd4af2e2

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 761342 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 863828
User impact if declined: Graphical glitch in the button
Testing completed (on m-c, etc.): Landed on mc this week.
Risk to taking this patch (and alternatives if risky): This is pretty low risk stuff. Just changing how we trigger the animation. There are some alternatives listed in this bug (changing the animation slightly) that seem to work around the issue...
String or IDL/UUID changes made by this patch: None.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this doesn't apply to Aurora at all. Will need a branch-specific patch.

Comment 17

[Mark Capella [:capella]]

Bouncing back to wesj ... not sure where you wanted this promoted to ...

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 761342 [details] [diff] [review]
Patch (v3)

At this point, the patch will need to be for beta (Fx23) due to today's merge festivities.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 761342 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Same as before

Comment 21

[Ryan VanderMeulen [:RyanVM]]

That doesn't address comment 16, though.

Comment 22

[Mark Capella [:capella]]

In version v22 and prior of FF / Mobile, this issue (and solution) didn't exist.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

We're talking about version 23 here. Was Aurora as of comment 16, now is beta as of yesterday's merge. What landed for 24 doesn't apply to 23. Is something not clear about what I've been saying?

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 761342 [details] [diff] [review]
Patch (v3)

approving the request, please carry over to branch specific patch.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Capella - Are you OK with working on the specific changes needed to apply the patch to beta?

Comment 26

[Mark Capella [:capella]]

mfinkle: Sure, but I'm unfamiliar with the process here since my changes usually only affect nightly, so I'll just explain what I found and let someone tell me how to proceed ...

Moving this patch to v23 / beta relies on two other patches having been moved first ... The first one, bug 874036, is in my opinion fairly noticible and I'm surprised it hadn't been moved up already. The second one, bug 880592, fixes a small regression caused by that bug.

Moving those patches now, requires a slight rebase due to other changes nearby in BrowserToolbar.java confusing Mercurial ... but otherwise imply no code changes.

This patch for bug 868222 will then apply cleanly on top of those and we're done.

I've tested these against a beta repo / v23 and can confirm all patch STR's before (as broken), and after (as corrected).

I can post the new versions of the patches, if you or margaret can help me with the mechanics of approving / moving them from there on ... let me know -- mark

Comment 27

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #26)
> mfinkle: Sure, but I'm unfamiliar with the process here since my changes
> usually only affect nightly, so I'll just explain what I found and let
> someone tell me how to proceed ...
> 
> Moving this patch to v23 / beta relies on two other patches having been
> moved first ... The first one, bug 874036, is in my opinion fairly noticible
> and I'm surprised it hadn't been moved up already. The second one, bug
> 880592, fixes a small regression caused by that bug.

Let's nominate the patches in those bugs for beta then. It sounds like bug 874036 is a regression in 23, so you're right that that must have slipped through the cracks.

> Moving those patches now, requires a slight rebase due to other changes
> nearby in BrowserToolbar.java confusing Mercurial ... but otherwise imply no
> code changes.

That's okay, we just need to nominate the patches in those bugs for beta.

> This patch for bug 868222 will then apply cleanly on top of those and we're
> done.
> 
> I've tested these against a beta repo / v23 and can confirm all patch STR's
> before (as broken), and after (as corrected).
> 
> I can post the new versions of the patches, if you or margaret can help me
> with the mechanics of approving / moving them from there on ... let me know

The mechanics are that we should get approval for those other bugs, then we can land the patches all together on beta. You don't need to upload a new patch for review unless the logic has changed (we trust you on simple un-bitrotting :).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Gabriela [:gaby2300]]

I can verify this is fixed in a Galaxy S2 phone with Android 4.0.3 and the latest Firefox Beta.