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

RESOLVED FIXED in Firefox 29



Firefox for Android
4 years ago
4 years ago


(Reporter: aaronmt, Assigned: jdover)


({regression, reproducible})

29 Branch
Firefox 29
regression, reproducible

Firefox Tracking Flags

(firefox29 fixed, fennec29+)



(2 attachments)



4 years ago
Created attachment 8368631 [details]

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.
Regression from bug 960359?
Flags: needinfo?(margaret.leibovic)

Comment 2

4 years ago
(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?
Assignee: nobody → jdover
Flags: needinfo?(margaret.leibovic)

Comment 3

4 years ago
Created attachment 8368841 [details] [diff] [review]

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).
Attachment #8368841 - Flags: review?(margaret.leibovic)


4 years ago
Depends on: 920791


4 years ago
Duplicate of this bug: 966791


4 years ago
Blocks: 960359
tracking-fennec: ? → 29+
Keywords: regression


4 years ago
No longer depends on: 920791

Comment 5

4 years ago
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

4 years ago
Comment on attachment 8368841 [details] [diff] [review]

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/
@@ +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/
@@ +292,1 @@
>        ;

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.
Attachment #8368841 - Flags: review?(margaret.leibovic) → review-

Comment 7

4 years ago
FIXED by backout in bug 960359.
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-firefox29: affected → fixed
Target Milestone: --- → Firefox 29
Duplicate of this bug: 966632
You need to log in before you can comment on or make changes to this bug.