Closed Bug 1083879 Opened 5 years ago Closed 5 years ago

Regression: Doorhanger for popup blocker not working

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 --- verified
firefox36 --- verified
fennec 35+ ---

People

(Reporter: u421692, Assigned: billm)

References

(Depends on 1 open bug)

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

Steps to reproduce:
1. Open http://www.popuptest.com/popuptest1.html

Expected result:
Doorhanger is displayed

Actual result:
Doorhanger is not displayed

Regression window:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a5f925b1237e&tochange=c5e310d17e58

Probably Bug 1076868 caused the regression
Double checking that range, I don't think it's accurate.
tracking-fennec: --- → ?
Keywords: reproducible
Summary: Doorhanger not displayed for pop-ups blocked → Regression: Doorhanger for popup blocker not working
Is there anything in the log?
no errors or warnings
I'm guessing that maybe the process=main annotation isn't working on Android and so MainProcessSingleton.js isn't getting started. As a consequence, we're not loading browser-content.js.

I think this really shows that we need pop-up blocking tests on Android. I think this is the second time I've regressed this.
Flags: needinfo?(wmccloskey)
Assignee: nobody → wmccloskey
Oh, I see. The problem is that I added my files to browser/installer/package-manifest.in but not to mobile/android/installer/package-manifest.in. Oops. I'll get a patch uploaded for this tomorrow.

The main tests I see for pop-up blocking are browser-chrome tests, which presumably don't run on Android. Margaret, how hard would it be to write a test for this? I'm not familiar with how we test mobile.
Flags: needinfo?(margaret.leibovic)
A test for this sounds like a good idea. We should add a testcase for this to testDoorHanger:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testDoorHanger.java

We would just have to write a little html page that tries to open a popup window, load that in the browser, then test to make sure the popup appeared.

Here's some more information about Robocop tests:
https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop

Bill, do you have an Android build environment? If you don't, I can try writing this test for you.
Flags: needinfo?(margaret.leibovic)
Attachment #8507092 - Flags: review?(margaret.leibovic)
Attached patch android-popup-test-wip (obsolete) — Splinter Review
Thanks Margaret. I don't have an Android environment. I'd appreciate it if you could write the test. I feel guilty asking you, so I started writing something that might help. Assuming it runs, I think the main thing it's missing is a check for whether the popup opens or not. The desktop test uses a TabOpen event, but I doubt that will work in Android.
Attachment #8507096 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8507092 [details] [diff] [review]
fix-android-popup

This fixes the issue for me.
Attachment #8507092 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8507096 [details] [diff] [review]
android-popup-test-wip

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

This is looking good. I can work on finishing this, since I'm a bit embarrassed that we didn't already have a test for this :)

::: mobile/android/base/tests/testDoorHanger.java
@@ +167,5 @@
> +        inputAndLoadUrl(POPUP_URL);
> +        waitForText(POPUP_MESSAGE);
> +        mAsserter.is(mSolo.searchText(POPUP_MESSAGE), true, "Popup blocker is displayed");
> +
> +        // FIXME: Check here that the popup was not shown.

If the popup were shown, the click on the allow button below will fail, so that's probably enough to catch this case.

@@ +173,5 @@
> +        mSolo.clickOnButton(POPUP_ALLOW);
> +        waitForTextDismissed(POPUP_MESSAGE);
> +        mAsserter.is(mSolo.searchText(POPUP_MESSAGE), false, "Popup blocker is hidden when popup allowed");
> +
> +        // FIXME: Check here that the popup was shown.

We can check that a new tab was opened, and then we can close it. We do something similar in this test:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBookmarksPanel.java#36

@@ +184,5 @@
> +        mSolo.clickOnButton(POPUP_DENY);
> +        waitForTextDismissed(POPUP_MESSAGE);
> +        mAsserter.is(mSolo.searchText(POPUP_MESSAGE), false, "Popup blocker is hidden when popup denied");
> +
> +        // FIXME: Check here that the popup was not shown.

We can verify that the urlbar still has the same url, since opening popups in Fennec opens them in newly selected tabs.
Attachment #8507096 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 8507096 [details] [diff] [review]
android-popup-test-wip

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

::: mobile/android/base/tests/testDoorHanger.java
@@ +169,5 @@
> +        mAsserter.is(mSolo.searchText(POPUP_MESSAGE), true, "Popup blocker is displayed");
> +
> +        // FIXME: Check here that the popup was not shown.
> +
> +        mSolo.clickOnButton(POPUP_ALLOW);

Note to self: We should also uncheck the "don't ask again" checkbox before hitting the allow button here.
Passing for me locally. Here's a try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8c3e56a1e7a8

I contemplated splitting this off into its own test, but I feel like the cost of setting up/tearing down a test is non-trivial, and this still falls under the umbrella of "doorhanger testing".
Attachment #8507096 - Attachment is obsolete: true
Attachment #8507231 - Flags: review?(rnewman)
Comment on attachment 8507231 [details] [diff] [review]
Add robocop test for popup blocker

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

LGTM.

::: mobile/android/base/tests/testDoorHanger.java
@@ +164,5 @@
> +    private void testPopupBlocking() {
> +        String POPUP_URL = getAbsoluteUrl(StringHelper.ROBOCOP_POPUP_URL);
> +        String POPUP_MESSAGE = "prevented this site from opening";
> +        String POPUP_ALLOW = "Show";
> +        String POPUP_DENY = "Don't show";

Shouldn't these be in StringHelper?
Attachment #8507231 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #14)
> Comment on attachment 8507231 [details] [diff] [review]
> Add robocop test for popup blocker
> 
> Review of attachment 8507231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: mobile/android/base/tests/testDoorHanger.java
> @@ +164,5 @@
> > +    private void testPopupBlocking() {
> > +        String POPUP_URL = getAbsoluteUrl(StringHelper.ROBOCOP_POPUP_URL);
> > +        String POPUP_MESSAGE = "prevented this site from opening";
> > +        String POPUP_ALLOW = "Show";
> > +        String POPUP_DENY = "Don't show";
> 
> Shouldn't these be in StringHelper?

The other individual UI strings in this test are similarly defined as local variables, so I was just following suit. I can make a mentor bug to move all these strings into StringHelper.

Bill, the try run looks green, so feel free to land this test when you land your patch!
https://hg.mozilla.org/mozilla-central/rev/9adfd589574c
https://hg.mozilla.org/mozilla-central/rev/6bac6005df5f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Can we get an uplift request to get this on Fx35?
tracking-fennec: ? → 35+
Comment on attachment 8507092 [details] [diff] [review]
fix-android-popup

Approval Request Comment
[Feature/regressing bug #]: bug 1067576
[User impact if declined]: popup blocker broken
[Describe test coverage new/current, TBPL]: works on trunk
[Risks and why]: very low
[String/UUID change made/needed]: none
Attachment #8507092 - Flags: approval-mozilla-aurora?
Attachment #8507092 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on latest Nightly build.
Verified as fixed on latest Aurora build.
Status: RESOLVED → VERIFIED
Depends on: 1135102
You need to log in before you can comment on or make changes to this bug.