977155 - Home banner can appear on non-default panel if panels swiped before gecko is running

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Comment 1

[:Margaret Leibovic]

Attachment: Don't set HomeBanner to VISIBLE unless it is enabled

This logic is tricky and annoying, but hopefully this patch makes it clearer.

I found that the initial translation on the view actually wasn't doing anything, so this patch makes us more explicit about when we show and hide the banner. As part of this, I made animateUp dumber, and consolidated the enabled/visibility logic in setEnabled/handleMessage (since they're the two different places when we might actually try to show the banner).

The other piece of the puzzle here is that we use the TextView's text as another indicator of whether or not we should show the banner, so I had clear that in the close button handler in order to avoid showing the banner again in setEnabled. However, I think this is correct, and a step in the right direction, since in the future we may want to show a new message at some point during the app lifetime (bug 975239).

Josh, let me know what you think of this, and I'll request review from a peer once you think it's good.

Comment 2

[Josh Dover [:jdover]]

Comment on attachment 8382694 [details] [diff] [review]
Don't set HomeBanner to VISIBLE unless it is enabled

Good simplification of the logic :)

Comment 3

[:Margaret Leibovic]

Comment on attachment 8382694 [details] [diff] [review]
Don't set HomeBanner to VISIBLE unless it is enabled

Normally I would ask lucasr for review, but he's gone until next week. So... bnicholson, can you review this? It would probably be good to get some fresh eyes on this logic anyway.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8382694 [details] [diff] [review]
Don't set HomeBanner to VISIBLE unless it is enabled

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

Looks good, just withholding r+ since I'm curious about the translationY change. Other comments are things I noticed in HomeBanner.java that aren't related to this patch.


We currently call setTag() directly from handleMessage at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeBanner.java#130. setTag() is a method in View, so this should be moved to the UI thread.

::: mobile/android/base/home/HomeBanner.java
@@ +171,2 @@
>  
>      public void setEnabled(boolean enabled) {

Note that this overrides View#setEnabled. Was this intentional? If so, please add missing @Override; if not, you might want to rename it.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ -45,5 @@
>                                                     android:gravity="center_vertical"
>                                                     android:visibility="gone"
>                                                     android:clickable="true"
> -                                                   android:focusable="true"
> -                                                   android:translationY="@dimen/home_banner_height"/>

I'm confused about this change. It looks like having translationY here offsets the banner by putting it below the Home fragment. Then, when we call animateUp, we see that the banner is off the screen, so we animate to its natural position of 0 Y translation.

Removing this will make it start at 0 Y translation. Won't that break animateUp, which first checks `ViewHelper.getTranslationY(this) == 0`? Or am I missing something?

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8382694 [details] [diff] [review]
Don't set HomeBanner to VISIBLE unless it is enabled

Just switching to r- until the question in comment 4 gets answered.

Comment 6

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 8382694 [details] [diff] [review]
> Don't set HomeBanner to VISIBLE unless it is enabled
> 
> Review of attachment 8382694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just withholding r+ since I'm curious about the translationY
> change. Other comments are things I noticed in HomeBanner.java that aren't
> related to this patch.
> 
> 
> We currently call setTag() directly from handleMessage at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomeBanner.java#130. setTag() is a method in View, so this should be moved
> to the UI thread.
> 
> ::: mobile/android/base/home/HomeBanner.java
> @@ +171,2 @@
> >  
> >      public void setEnabled(boolean enabled) {
> 
> Note that this overrides View#setEnabled. Was this intentional? If so,
> please add missing @Override; if not, you might want to rename it.

Good catch, we didn't notice that in previous reviews. We should rename it, I can make a follow-up patch here to do that.

> ::: mobile/android/base/resources/layout/gecko_app.xml
> @@ -45,5 @@
> >                                                     android:gravity="center_vertical"
> >                                                     android:visibility="gone"
> >                                                     android:clickable="true"
> > -                                                   android:focusable="true"
> > -                                                   android:translationY="@dimen/home_banner_height"/>
> 
> I'm confused about this change. It looks like having translationY here
> offsets the banner by putting it below the Home fragment. Then, when we call
> animateUp, we see that the banner is off the screen, so we animate to its
> natural position of 0 Y translation.
> 
> Removing this will make it start at 0 Y translation. Won't that break
> animateUp, which first checks `ViewHelper.getTranslationY(this) == 0`? Or am
> I missing something?

I realized that I was a bit confused while writing this patch, I'll post a new one shortly.

Comment 7

[:Margaret Leibovic]

Attachment: Don't animate HomeBanner when it's not visible

I did a bad job diagnosing the problem here. It looks like the real problem here is caused by the fact that we're calling animateDown() whenever we setEnabled(false). However, when the banner isn't visible, animateDown isn't actually doing what we would expect it to do, since the banner height is 0, so it actually goes ahead and animates the banner *up*. Then when we set the banner to visible after setting the text, the banner is already translated up.

This patch just prevents up from animating the banner when it's not visible, and this fixes the problem without actually changing where we call setVisible.

Luckily, this allows us to get rid of the dependency on the state of the text in the TextView, since we only call animateUp when the banner is visible, and it's only visible when there's text in it.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 2) Properly override HomeBanner.setEnabled

Feeling too lazy to actually rename this :)

We don't want to interfere with the default setEnabled behavior, so I think we should still call the super method.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8383979 [details] [diff] [review]
Don't animate HomeBanner when it's not visible

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

So basically, you're using the visibility state to flag that we've received the banner text from Gecko? I guess this works, though our terminology seems backwards: setVisible() is actually controlling whether it's enabled, and setEnabled() is determining whether it's visible!

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8383987 [details] [diff] [review]
(Part 2) Properly override HomeBanner.setEnabled

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

If we go down this road, we should also override isEnabled() to prevent mEnabled and isEnabled() from being out of sync: http://developer.android.com/reference/android/view/View.html#isEnabled%28%29

I would probably just rename setEnabled() to something else, but I don't have any strong opinions. After all, the API does say that "the interpretation of the enabled state varies by subclass", so that implies that we're free to define its behavior.

Comment 11

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #10)
> Comment on attachment 8383987 [details] [diff] [review]
> (Part 2) Properly override HomeBanner.setEnabled
> 
> Review of attachment 8383987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If we go down this road, we should also override isEnabled() to prevent
> mEnabled and isEnabled() from being out of sync:
> http://developer.android.com/reference/android/view/View.html#isEnabled%28%29
> 
> I would probably just rename setEnabled() to something else, but I don't
> have any strong opinions. After all, the API does say that "the
> interpretation of the enabled state varies by subclass", so that implies
> that we're free to define its behavior.

I agree we should probably just name it something else, especially given your last comment noting the confusion the current naming can cause. Maybe setActive? Basically this flag just keeps track of whether or not the user is on the default panel on the home pager, so maybe we should pick a name that reflects that, although it's nice that the home banner doesn't know about the home pager implementation right now.

Comment 12

[Brian Nicholson (:bnicholson)]

Sure, setActive works for me.

Comment 13

[:Margaret Leibovic]

Attachment: (Part 2) Rename HomeBanner setEnabled to setActive

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/f1910593bd40
https://hg.mozilla.org/integration/fx-team/rev/2fb022dbbe72

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f1910593bd40
https://hg.mozilla.org/mozilla-central/rev/2fb022dbbe72

Comment 16

[:Margaret Leibovic]

Ian, how do you feel about the message rotating every time we create the home page? This actually matches the original banner behavior, although that didn't get much use in the wild, so maybe that design wasn't intentional :)

The benefit of rotating every time we create the home page is that users will be more likely to see multiple messages if there are multiple messages to show.

However, we'll also want to think about how this will interact with the close button. With the patch I posted, if the user hits the close button, we'll remove that message from the rotation, but if there are other messages in the rotation, we'd end up showing one of those the next time the user opens about:home.

Comment 17

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #16)
> Ian, how do you feel about the message rotating every time we create the
> home page? This actually matches the original banner behavior, although that
> didn't get much use in the wild, so maybe that design wasn't intentional :)

This part works for me. 

> However, we'll also want to think about how this will interact with the
> close button. With the patch I posted, if the user hits the close button,
> we'll remove that message from the rotation, but if there are other messages
> in the rotation, we'd end up showing one of those the next time the user
> opens about:home.

Yeah, we want to make sure that we are respecting the close button action here. If they hit close, it should mark the message as read *and* put a pause on other banners for some period of time.

Comment 18

[:Margaret Leibovic]

Comment on attachment 8383979 [details] [diff] [review]
Don't animate HomeBanner when it's not visible

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 960359
User impact if declined: home banner can appear on non-default panel
Testing completed (on m-c, etc.): baked on m-c since 2/28
Risk to taking this patch (and alternatives if risky): low-risk, simplifies logic for showing home banner
String or IDL/UUID changes made by this patch: none

Comment 19

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #17)
> (In reply to :Margaret Leibovic from comment #16)
> > Ian, how do you feel about the message rotating every time we create the
> > home page? This actually matches the original banner behavior, although that
> > didn't get much use in the wild, so maybe that design wasn't intentional :)
> 
> This part works for me. 
> 
> > However, we'll also want to think about how this will interact with the
> > close button. With the patch I posted, if the user hits the close button,
> > we'll remove that message from the rotation, but if there are other messages
> > in the rotation, we'd end up showing one of those the next time the user
> > opens about:home.
> 
> Yeah, we want to make sure that we are respecting the close button action
> here. If they hit close, it should mark the message as read *and* put a
> pause on other banners for some period of time.

Wow, I was confused and commented in the wrong bug, this convo belongs in bug 975239.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/362f089cdd29
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bd7736f316e