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

RESOLVED FIXED in Firefox 29

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: jdover)

Tracking

({regression, reproducible})

29 Branch
Firefox 29
ARM
Android
regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox29 fixed, fennec29+)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8368631 [details]
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.
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)
(Assignee)

Comment 3

4 years ago
Created attachment 8368841 [details] [diff] [review]
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).
Attachment #8368841 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Depends on: 920791
(Reporter)

Updated

4 years ago
Duplicate of this bug: 966791

Updated

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

Updated

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

Comment 7

4 years ago
FIXED by backout in bug 960359.
Status: ASSIGNED → RESOLVED
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.