920791 - Hide home banner when in editing mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

The home banner eats up valuable real estate, we shouldn't show it when the keyboard is open.

Comment 1

[:Margaret Leibovic]

Whatever we do in this bug, we'll probably use a similar solution for bug 907857. While trying to come up with a solution here, I tried listening for config changes, but that wasn't working well. Maybe I'll try experimenting with urlbar focus like Lucas suggested in bug 907857 comment 2.

However, I realized these bugs aren't a major problem, since as soon as the user starts typing we show the BrowserSearch list, which overlays the about:home content anyway.

Comment 2

[Josh Dover [:jdover]]

Attachment: Hide banner when URL bar is focused

I'm looking for some other ideas on this one, as this does work, I feel like the BrowserToolbar really shouldn't know about HomeBanner, however I can't think of a particularly good way to do this because:

1) As far as I could find, there is no reliable way to detect that the keyboard is present between Android versions and devices.
2) You cannot register more than one onFocusChangeListener to a view.

I think this would be a case where having an event bus (eg. Otto) would be a good way to decouple this functionality.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8366997 [details] [diff] [review]
Hide banner when URL bar is focused

I'm going to re-route this to lucasr, as he has more experience with the toolbar :)

I agree it feels pretty dirty to be poking at the home banner from inside BrowserToolbar... maybe there's a notification we can fire to let consumers know when we enter/exit editing mode.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8366997 [details] [diff] [review]
Hide banner when URL bar is focused

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

Yeah, this is a bit too hand-wavy :-) Here are some possible solutions:
- Track window resizes (which are only really triggered by the activity anyway) and hide banner accordingly.
- Add a OnFocusListener to BrowserToolbar and let BrowserApp tell HomePager that it needs to hide the banner.

Experiment with them and see which one looks cleaner?

Comment 5

[Josh Dover [:jdover]]

Attachment: Hide banner when URL bar is focused

Much better this time :)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8368053 [details] [diff] [review]
Hide banner when URL bar is focused

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

Nice. Here's what I suggest:
1. Add an onToolbarFocusChange(boolean hasFocus) method to HomePager
2. In the OnFocusChangeListener, call onToolbarFocusChange() You must check isHomePagerVisible() before doing so btw.
3. You already have a mHomeBannger member in HomePager (added in bug 960359), call hideBanner()/showBanner() in HomePager.onToolbarFocusChange().

::: mobile/android/base/GeckoApp.java
@@ +198,5 @@
>      protected DoorHangerPopup mDoorHangerPopup;
>      protected FormAssistPopup mFormAssistPopup;
>      protected TabsPanel mTabsPanel;
>      protected ButtonToast mToast;
> +    protected HomeBanner mHomeBanner;

Neither GeckoApp nor BrowserApp should know anything about HomeBanner. This should be encapsulated behind an API in HomePager.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +88,5 @@
>      public interface OnActivateListener {
>          public void onActivate();
>      }
>  
> +    public interface onDeactivateListener {

Unrelated? Or did you mean OnFocusChangeListener here?

@@ +319,5 @@
>          mUrlEditLayout.setOnFocusChangeListener(new View.OnFocusChangeListener() {
>              @Override
>              public void onFocusChange(View v, boolean hasFocus) {
>                  setSelected(hasFocus);
> +                mFocusChangeListener.onFocusChange(v, hasFocus);

You should check if mFocusChangeListener is not null before calling it. I'd skip the view argument from the focus as this is pretty much breaking the encapsulated views in BrowserToolbar i.e. users of BrowserToolbar's focus listener should need to know which specific view got focus.

Comment 7

[Lucas Rocha (:lucasr)]

Ah, I think the HomeBanner methods should actually be hide()/show() instead of the redundant hideBanner()/showBanner() btw.

Comment 8

[Josh Dover [:jdover]]

Attachment: patch

Moved logic into HomePager as well as renamed showBanner/hideBanner -> show/hide

Comment 9

[:Margaret Leibovic]

Comment on attachment 8368182 [details] [diff] [review]
patch

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

::: mobile/android/base/home/HomePager.java
@@ +318,5 @@
> +    public void onToolbarFocusChange(boolean hasFocus) {
> +        if (hasFocus) {
> +            mHomeBanner.hide();
> +        } else {
> +            mHomeBanner.show();

Will this make the banner show up on non-default panels?

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8368182 [details] [diff] [review]
> patch
> 
> Review of attachment 8368182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +318,5 @@
> > +    public void onToolbarFocusChange(boolean hasFocus) {
> > +        if (hasFocus) {
> > +            mHomeBanner.hide();
> > +        } else {
> > +            mHomeBanner.show();
> 
> Will this make the banner show up on non-default panels?

Is that part of the design? I wasn't aware of this. What about the case where the user has hidden all panels?

Comment 11

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> (In reply to :Margaret Leibovic from comment #9)
> > Comment on attachment 8368182 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8368182 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/home/HomePager.java
> > @@ +318,5 @@
> > > +    public void onToolbarFocusChange(boolean hasFocus) {
> > > +        if (hasFocus) {
> > > +            mHomeBanner.hide();
> > > +        } else {
> > > +            mHomeBanner.show();
> > 
> > Will this make the banner show up on non-default panels?
> 
> Is that part of the design? I wasn't aware of this. What about the case
> where the user has hidden all panels?

The banner should only show up on the default panel (that's the logic jdover recently added in bug 960359). In that bug I suggested we file a follow-up to make sure we just never show the banner if no panels are enabled (I think it would be weird to show the banner if the rest of the screen is empty).

Comment 12

[Josh Dover [:jdover]]

Attachment: patch

(In reply to Lucas Rocha (:lucasr) from comment #10)
> (In reply to :Margaret Leibovic from comment #9)
> > ::: mobile/android/base/home/HomePager.java
> > @@ +318,5 @@
> > > +    public void onToolbarFocusChange(boolean hasFocus) {
> > > +        if (hasFocus) {
> > > +            mHomeBanner.hide();
> > > +        } else {
> > > +            mHomeBanner.show();
> > 
> > Will this make the banner show up on non-default panels?
> 
> Is that part of the design? I wasn't aware of this. What about the case
> where the user has hidden all panels?

Good catches, updated patch to handle these cases, verified to work!

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8368243 [details] [diff] [review]
patch

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

Awesome.

Comment 14

[Josh Dover [:jdover]]

To clarify, the way this works is for the banner to show up even if there are no panels. Will file a bug for showing no banner when there are no panels. May need to have a discussion about whether there will be any important snippets that the user could miss?

Comment 15

[Josh Dover [:jdover]]

See bug 966047 for hiding banner when there are no panels

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Josh Dover from comment #14)
> To clarify, the way this works is for the banner to show up even if there
> are no panels. Will file a bug for showing no banner when there are no
> panels. May need to have a discussion about whether there will be any
> important snippets that the user could miss?

Yeah, I think we should show the banner even when all panels are disabled. In any case, let's use bug 966047 to discuss that.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a4b73c79ac44

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[:Margaret Leibovic]

I had to back this out because of its dependency on bug 960359:
https://hg.mozilla.org/integration/fx-team/rev/b1f8c61bb310

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Merge of backout (still 29):
https://hg.mozilla.org/mozilla-central/rev/b1f8c61bb310

Comment 21

[:Margaret Leibovic]

Updating the summary to make this bug more general.

Comment 22

[Lucas Rocha (:lucasr)]

This is not an accurate summary btw. The UI can be in editing mode with the keyboard hidden, in which case we do want to show the banner, no?

Comment 23

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #22)
> This is not an accurate summary btw. The UI can be in editing mode with the
> keyboard hidden, in which case we do want to show the banner, no?

Do we? I thought this would be a good way to err on the side of not showing the banner too often. When the user is in editing mode, I assume they want to be going somewhere, not getting distracted by the banner.

Comment 24

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #23)
> (In reply to Lucas Rocha (:lucasr) from comment #22)
> > This is not an accurate summary btw. The UI can be in editing mode with the
> > keyboard hidden, in which case we do want to show the banner, no?
> 
> Do we? I thought this would be a good way to err on the side of not showing
> the banner too often. When the user is in editing mode, I assume they want
> to be going somewhere, not getting distracted by the banner.

Ah, now I understand the intent here. Then the summary should be something along the lines of "Only show home banner when accessing about:home directly". But that would also include when you create a new tab. I assume we only want to show snippets on startup? How does this behaviour interact with add-on snippets?

Comment 25

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #24)
> (In reply to :Margaret Leibovic from comment #23)
> > (In reply to Lucas Rocha (:lucasr) from comment #22)
> > > This is not an accurate summary btw. The UI can be in editing mode with the
> > > keyboard hidden, in which case we do want to show the banner, no?
> > 
> > Do we? I thought this would be a good way to err on the side of not showing
> > the banner too often. When the user is in editing mode, I assume they want
> > to be going somewhere, not getting distracted by the banner.
> 
> Ah, now I understand the intent here. Then the summary should be something
> along the lines of "Only show home banner when accessing about:home
> directly". But that would also include when you create a new tab. I assume
> we only want to show snippets on startup?

I think showing the banner on a new tab is fine, since that's similar to the startup experience. Basically, to generalize, I think we should show the banner when the user is actually on "about:home", but not when they get to this interface from tapping on the toolbar.

> How does this behaviour interact with add-on snippets?

I imagine any fix we make here will apply to the home banner, which does not know whether a banner message is coming from the snippets server or an add-on. I don't think we should worry about differentiating the two, at least for now, since I haven't actually seen any add-ons using this banner API (besides the add-ons I've made to test it).

Comment 26

[Josh Dover [:jdover]]

Attachment: patch

Updated to reflect new changes in 960359 and added a test.

Comment 27

[:Margaret Leibovic]

Comment on attachment 8379887 [details] [diff] [review]
patch

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

Looking good, r+ with comments addressed.

::: mobile/android/base/BrowserApp.java
@@ +510,5 @@
>  
> +        mBrowserToolbar.setOnFocusChangeListener(new View.OnFocusChangeListener() {
> +            @Override
> +            public void onFocusChange(View v, boolean hasFocus) {
> +                mHomePager.onToolbarFocusChange(hasFocus);

The original patch checked isHomePagerVisible in here. We probably still want that, right?

::: mobile/android/base/home/HomePager.java
@@ +285,5 @@
>          return super.dispatchTouchEvent(event);
>      }
>  
> +    public void onToolbarFocusChange(boolean hasFocus) {
> +        boolean enabled = !hasFocus && getCurrentItem() == mDefaultPageIndex;

Nit: final. Also, add a comment in here explaining the goal of this logic.

::: mobile/android/base/tests/testHomeBanner.java
@@ +99,5 @@
>          mAboutHome.assertBannerNotVisible();
>      }
>  
> +    private void hideOnToolbarFocusTest() {
> +        addBannerMessage();

Nothing is removing the banner that was added in addBannerTest, so we don't need to add another one. However, if we re-enable removeBannerTest, we will need to do something to add a banner back, so perhaps just put a comment about that in here.

These small testcase methods make it easier to read the code, but unfortunately, the app does maintain state from one to the next.

@@ +108,5 @@
> +        mToolbar.enterEditingMode();
> +        mAboutHome.assertBannerNotVisible();
> +
> +        mToolbar.dismissEditingMode();
> +        mAboutHome.assertBannerVisible();

Sweet, this is a nice little test.

Comment 28

[:Margaret Leibovic]

Also please make a try run!

Comment 29

[Josh Dover [:jdover]]

Attachment: patch

Fixed! Running on try: https://tbpl.mozilla.org/?tree=Try&rev=c46874207fa9

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ee23500859a4

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[:Margaret Leibovic]

This was set to tracking+ long ago, but we should really try to get this on 29, since that's when we're going to start showing users banners.

Comment 33

[:Margaret Leibovic]

Comment on attachment 8380013 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling snippets and sync promo banner
User impact if declined: banner appears while keyboard is open
Testing completed (on m-c, etc.): landed on m-c 2/25
Risk to taking this patch (and alternatives if risky): This caused a regression (bug 981614), so we'll need to uplift that as well. Other than that, no regressions found.
String or IDL/UUID changes made by this patch: none

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81fa76ec7c16

I made sure to take bug 977155 into account since that was already uplifted to Aurora prior to this.