966307 - Snippet banner shown over content; overlays over content like an annoying popup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: screenshot.png

Currently the snippet banner can be shown over content. It overlays over content when scrolling like an annoying popup.

Steps to reproduce

On my Galaxy SII

i) Launch the browser
ii) Tap a thumbnail on about:home

Content loads before the snippet, and thus the snippet is overlaid over content.

Comment 1

[Lucas Rocha (:lucasr)]

Regression from bug 960359?

Comment 2

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> Regression from bug 960359?

Sounds like it. I think we need to make sure that the banner is always hidden whenever the home pager is hidden.

Josh, can you look into this?

Comment 3

[Josh Dover [:jdover]]

Attachment: patch

After we removed the knowledge of the HomePager state from HomeBanner, late events could trigger the banner to be shown. This patch allows HomePager tell HomeBanner whether or not it is allowed to be shown, regardless of what other methods are called (ie. hide(), show(), or received JS message).

Comment 5

[:Margaret Leibovic]

Josh, was this regression caused by bug 920791 or by bug 960359?

I thought it was bug 960359, but the tracking flags were set otherwise. I want to back out whatever caused the regression, and I need to know if I should back out both bugs or just the first one. Right now I'm leaning towards just backing out both.

Comment 6

[:Margaret Leibovic]

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

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

I don't feel comfortable landing this patch.

I don't want us to rush to try to come up with a quick fix, so once the tree re-opens, I'm just going to back out the patches from bug 960359 and bug 920791. Also, ibarlow mentioned to me that he wants to re-think the design of this banner to make it feel like like a banner ad, so we may need to take a different approach anyway.

::: mobile/android/base/home/HomeBanner.java
@@ +101,5 @@
> +     * Used by HomePager to help override visibility that any hideBanner() or showBanner() calls
> +     * may change.
> +     */
> +    public void shouldAllowShow(boolean shouldAllowShow) {
> +        setVisibility(shouldAllowShow ? VISIBLE : GONE);

This feels fragile - this method is called shouldAllowShow, but it sets the visibility of the banner. If we're going to make HomePager in charge of whether or not the banner is showing, we should just go all-out and have HomePager call mHomeBanner.setVisibility.

::: mobile/android/base/home/HomePager.java
@@ +292,1 @@
>                  mHomeBanner.show();

This has a lot of code smell... why can't HomeBanner's show/hide methods take case of this for you?

I'm nervous that this patch just adds another layer of complexity, without addressing possible flaws in the design of the logic.

Comment 7

[:Margaret Leibovic]

FIXED by backout in bug 960359.