1083879 - Regression: Doorhanger for popup blocker not working

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

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

Comment 1

[Aaron Train [:aaronmt]]

Double checking that range, I don't think it's accurate.

Comment 2

[Aaron Train [:aaronmt]]

Ok, I also get https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a5f925b1237e&tochange=c5e310d17e58

Likely https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e310d17e58 via bug 1067576?

Comment 3

[:Margaret Leibovic]

Is there anything in the log?

Comment 4

[Aaron Train [:aaronmt]]

no errors or warnings

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 7

[: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.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-android-popup

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: android-popup-test-wip

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.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8507092 [details] [diff] [review]
fix-android-popup

This fixes the issue for me.

Comment 11

[:Margaret Leibovic]

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.

Comment 12

[:Margaret Leibovic]

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.

Comment 13

[:Margaret Leibovic]

Attachment: Add robocop test for popup blocker

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".

Comment 14

[Richard Newman [: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?

Comment 15

[:Margaret Leibovic]

(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!

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9adfd589574c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bac6005df5f

Thanks!

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9adfd589574c
https://hg.mozilla.org/mozilla-central/rev/6bac6005df5f

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Can we get an uplift request to get this on Fx35?

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e40a9b0757b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/16b3bd0d5ebf

Comment 21

[u421692]

Verified as fixed on latest Nightly build.

Comment 22

[u421692]

Verified as fixed on latest Aurora build.