967085 - Investigate moving the home banner into about:home content instead of making it an overlay

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

ibarlow from IRC: i think the homepage banner design needs to change, a small bit. My original idea of it being an overlay seemed like a nice way of quickly hiding it, but as a side effect it really feels like an ad. I sense that if we were to move it into the page content, it would feel a little nicer. Something like this, where we stick it to the top and it scrolls *with* content, instead of against it http://cl.ly/image/1y320B2H3O0x/o

Making this part of the default panel would solve bug 960359. However, this is more complicated than the approach that was taken in bug 960359, since we would need to somehow dynamically inject the banner view into the default panel.

Comment 1

[Ian Barlow (:ibarlow)]

In addition to moving it into the panel as shown in the mockup, we should hook up logic to show the banner on whichever panel is designated the default.

Comment 3

[:Margaret Leibovic]

Josh has been working on this.

Comment 4

[Josh Dover [:jdover]]

Attachment: WIP patch

This is a work in progress patch isn't quite complete. In particular it doesn't 1) Hide properly when there is no message or the user dismisses the banner, 2) Show up if the user sets HistoryPanel as the default (need to find a way to cleanly designate HistoryPanel's fragments as banner-eligible).

The most confusing part of this patch is probably why the banner is part of HomeListView. This is due make the banner properly scrollable with the rest of the content. Looking for some feedback on basic design.

Comment 5

[Josh Dover [:jdover]]

Attachment: WIP patch

Fixed a couple things

Comment 6

[:Margaret Leibovic]

Great, I'll take a look at this patch.

In the meantime, could you post a link to an APK for ibarlow to test? I think it's best to treat this as a prototype and get some design feedback before investing in a solid engineering solution.

Comment 7

[Josh Dover [:jdover]]

Test build: https://drive.google.com/file/d/0BwqqgfwYmRg4RE5hRGtvZTRoQ1U/edit?usp=sharing

Comment 8

[Ian Barlow (:ibarlow)]

Hm I can't seem to get that to download, Josh... mind linking it from somewhere else?

Comment 9

[Josh Dover [:jdover]]

Try this: https://www.dropbox.com/s/v14iwgl0d1vq249/NewHomeBanner.apk

Comment 10

[Ian Barlow (:ibarlow)]

Hm. Just seeing a gap where a snippet might go: http://cl.ly/image/3E0907073s3

Comment 11

[Ian Barlow (:ibarlow)]

Let me try that link again. http://cl.ly/image/3E0907073s3u

Comment 12

[:Margaret Leibovic]

Comment on attachment 8370494 [details] [diff] [review]
WIP patch

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

I like this because it feels a lot less risky than what we did in bug 960359. However, let's chat with ibarlow to see how he's feeling before we go forward with finishing this patch.

I also saw an empty space appear when there was no banner content, so that's a bug that needs to get fixed.

If we do go with this approach, I think we consider a few follow-up ideas:
* Swipe to dismiss banner (might be tricky with page swiping gestures)
* Only display banner a certain percentage of the time, so that we're not annoying
* Make sure we fix bug 920791 :)

::: mobile/android/base/home/BookmarksPanel.java
@@ +87,3 @@
>          return view;
>      }
>  

General comment: please set your diff defaults to provide 8 lines of context (you can do this in your .hgrc)

::: mobile/android/base/home/HomeBanner.java
@@ +74,5 @@
> +                public void onClick(View view) {
> +                    ((ListView) getParent()).removeHeaderView(HomeBanner.this);
> +                }
> +            });
> +        }

This refactoring is out of the scope of this bug.

@@ +79,5 @@
> +    }
> +
> +    @Override
> +    public void onAttachedToWindow() {
> +        setVisibility(GONE);

Why not just set the visibility in the XML?

@@ +104,5 @@
> +                } catch (JSONException e) {
> +                    Log.e(LOGTAG, "Exception handling " + event + " message", e);
> +                }
> +            }
> +        });

This refactoring is also out of the scope of this bug. If this refactoring is necessary, please split it into a separate patch.

::: mobile/android/base/home/HomeFragment.java
@@ +57,5 @@
>      // This is used to defer data loading until the editing
>      // mode animation ends.
>      private boolean mCanLoadHint;
>  
> +    // Whether the fragment should display its snippets banner.

Nit: Don't use the word "snippets" anywhere in the home banner code.

@@ -68,2 @@
>  
> -        mIsLoaded = false;

Why this change?

::: mobile/android/base/home/HomeListView.java
@@ +63,5 @@
>      }
>  
> +    public void setBanner(BannerDisplayDelegate delegate) {
> +        if (delegate.shouldDisplayBanner()) {
> +            View banner = delegate.getLayoutInflater().inflate(R.layout.home_banner, this, false);

Why do we need to get the layout inflater from the delegate? I think we should remove that method from the interface and just use LayoutInflater.from(getContext()) instead.

Good news: this lazy inflation would make bug 968640 invalid.

@@ +65,5 @@
> +    public void setBanner(BannerDisplayDelegate delegate) {
> +        if (delegate.shouldDisplayBanner()) {
> +            View banner = delegate.getLayoutInflater().inflate(R.layout.home_banner, this, false);
> +            addHeaderView(banner);
> +        }

Hm, I can see that a header view is a natural way to implement this banner, but this approach isn't generic to all types of panels, such as a panel that just holds a grid view.

For a first version of this feature, I would be willing to compromise that we only support banners in HomeListViews, since that's what all of our built-in panels are. However, if this is the case, we should put the logic about banners all in HomeListView, not split between HomeFragment and HomeListView. And we should document all of our assumptions.

::: mobile/android/base/home/LastTabsPanel.java
@@ +105,4 @@
>  
>          mList = (HomeListView) view.findViewById(R.id.list);
>          mList.setTag(HomePager.LIST_TAG_LAST_TABS);
> +        mList.setBanner(this);

This is misleading... when I first read this I thought "wait, we're adding the banner to every panel?" But actually this just maybe sets the banner.

If you get rid of getLayoutInflater from BannerDisplayDelegate, you could instead update this to do something like:

if (shouldDisplayBanner()) {
  mList.setBanner();
}

or better, these methods could be renamed:

if (shouldAddBanner()) {
  mList.addBanner();
}

Then HomeListView doesn't even need to know about BannerDisplayDelegate. In fact, you don't even need that interface, you could just have the method on HomeFragment.

Comment 13

[Josh Dover [:jdover]]

Attachment: Make home banner part of panel content

Fixed up and simplified interactions between panels and the HomeBanner.

> Hm, I can see that a header view is a natural way to implement this banner,
> but this approach isn't generic to all types of panels, such as a panel that
> just holds a grid view.
> 
> For a first version of this feature, I would be willing to compromise that
> we only support banners in HomeListViews, since that's what all of our
> built-in panels are. However, if this is the case, we should put the logic
> about banners all in HomeListView, not split between HomeFragment and
> HomeListView. And we should document all of our assumptions.

As far as I could find, the only way to make the banner scrollable with the ListView was to either 1) Add it as a header view, or 2) do custom touch handling to move the banner up. The header view approach seems much safer (and probably more performant). We can overload addBanner() to accept other types of views in the future.

Comment 14

[Ian Barlow (:ibarlow)]

Josh, got a build with a snippet in it I could try?

Comment 15

[:Margaret Leibovic]

To test the snippets rotation, when you make the build, you can change the "browser.snippets.updateUrl" pref to point to your own test file with multiple snippets, something like: 
http://people.mozilla.org/~mleibovic/snippets.json

Comment 16

[:Margaret Leibovic]

Comment on attachment 8373554 [details] [diff] [review]
Make home banner part of panel content

Clearing review because we're going to put this approach on hold and instead focus on reviving the patch in bug 960359.

Comment 17

[:Margaret Leibovic]

Removing the dependency on bug 960359 and re-noming, this might just be something we explore in a future release.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 21

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.