786073 - Lock animation distracting/confusing while switching tabs, loading pages/etc

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Steps to reproduce:
- Visit a bugzilla page: https://bugzilla.mozilla.org
- Visit a non-ssl page: http://people.mozilla.org/~mwargers/tests/
- Switch tabs or reload the https://bugzilla.mozilla.org page

Expected result:
- Lock icon disappears/appears immediately when switching tabs
- On reload, the lock icon should immediately disappear when the old document has finally gone away, not when reload has just been tapped upon

The lock animation that I'm currently seeing is distracting. It makes me take a second look at the screen to find out why something has just moved.

Comment 1

[Lucas Rocha (:lucasr)]

I agree that showing the animation too often is a bit distracting. Maybe we should animate the lock icon after loading a new page? We should definitely avoid the animation when switching tabs.

Ian, what's your take here?

Comment 2

[Lucas Rocha (:lucasr)]

hg qseri

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Don't animate lock icon when switching or restoring tabs

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Only animate lock icon when showing it

After using Nightly with the animation for a bit, I feel that we should also drop the animation when hiding the lock. It feels like there's just too much smooth movement in the toolbar (especially when you use the back/forward buttons).

Here's a test build that only animates when lock becomes visible:
https://dl.dropbox.com/u/1187037/fennec-anim-lock-show.apk

Ian, let me know what you think. If you feel strongly about keeping the lock-hiding animation, I'm ok with keeping it. I'll request review on this patch otherwise.

Comment 5

[Ian Barlow (:ibarlow)]

I actually find the "lock hiding" animation just as useful as the "lock showing" animation. It's a helpful visual cue for when you are moving from a more secure state to a less secure state. I think we should keep it in.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 658435 [details] [diff] [review]
Only animate lock icon when showing it

Based on the discussion in IRC, we've decided to keep the hiding animation. I'm marking this one as obsolete then. Mark, it's ok to review the other patch independently by the way.

Comment 7

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

Comment on attachment 658434 [details] [diff] [review]
Don't animate lock icon when switching or restoring tabs

Doesn't the "fall through" for SELECTED mean mAnimateSiteSecurity will always be true?

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 658434 [details] [diff] [review]
> Don't animate lock icon when switching or restoring tabs
> 
> Doesn't the "fall through" for SELECTED mean mAnimateSiteSecurity will
> always be true?

Yep. The intention is to disable the animation temporarily just for the selected/restored event. The key thing here is that the animation is re-enabled *after* the refresh() call (when handling selected/restored events).

Comment 9

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

Comment on attachment 658434 [details] [diff] [review]
Don't animate lock icon when switching or restoring tabs

I assume you checked that "refresh()" doesn't spawn some thread that would make it return quickly and race the code that sets "mAnimateSiteSecurity" to false.

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Comment on attachment 658434 [details] [diff] [review]
> Don't animate lock icon when switching or restoring tabs
> 
> I assume you checked that "refresh()" doesn't spawn some thread that would
> make it return quickly and race the code that sets "mAnimateSiteSecurity" to
> false.

Double-checked that. Refresh is always run immediately and in the main thread (as expected).

Comment 11

[Lucas Rocha (:lucasr)]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a21df306c3cd

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Cristian Nicolae (:xti)]

This issue seems to be fixed on the latest Nightly. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-11)
Device: Galaxy Note
OS: Android 4.0.4

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 658434 [details] [diff] [review]
Don't animate lock icon when switching or restoring tabs

[Approval Request Comment]
User impact if declined: This patch fixed a very annoying/distracting site effect of the lock icon animation in the browser toolbar.
Testing completed (on m-c, etc.): It's been on nightly for a few days now, no issues found.
Risk to taking this patch (and alternatives if risky): Very low, simply disables the animation when switching or restoring tabs.
String or UUID changes made by this patch: None.

Comment 15

[Lucas Rocha (:lucasr)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/006174be2306

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/c17f628688fc

Comment 16

[Cristian Nicolae (:xti)]

I cannot reproduce it either on the latest Aurora and Beta build. 

--
Firefox 17.0a2 (2012-09-26)
Firefox 16.0b5 (2012-09-26)
Device: Galaxy Note
OS: Android 4.0.4