If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Regression: Doorhanger for popup blocker not working

VERIFIED FIXED in Firefox 35

Status

()

Firefox for Android
General
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: u421692, Assigned: billm)

Tracking

(Depends on: 1 bug, {regression, reproducible})

Trunk
Firefox 36
All
Android
regression, reproducible
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 unaffected, firefox34 unaffected, firefox35 verified, firefox36 verified, fennec35+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

3 years ago
Keywords: regressionwindow-wanted
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?
Flags: needinfo?(wmccloskey)
Keywords: regressionwindow-wanted

Comment 3

3 years ago
Is there anything in the log?
no errors or warnings
(Assignee)

Comment 5

3 years ago
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)

Updated

3 years ago
Assignee: nobody → wmccloskey
(Assignee)

Comment 6

3 years ago
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)

Comment 7

3 years ago
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)
(Assignee)

Comment 8

3 years ago
Created attachment 8507092 [details] [diff] [review]
fix-android-popup
Attachment #8507092 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 9

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

Comment 10

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

This fixes the issue for me.
Attachment #8507092 - Flags: review?(margaret.leibovic) → review+

Comment 11

3 years ago
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 12

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

3 years ago
Created attachment 8507231 [details] [diff] [review]
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".
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+

Comment 15

3 years ago
(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!
(Assignee)

Comment 16

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

Thanks!
https://hg.mozilla.org/mozilla-central/rev/9adfd589574c
https://hg.mozilla.org/mozilla-central/rev/6bac6005df5f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Can we get an uplift request to get this on Fx35?
tracking-fennec: ? → 35+
(Assignee)

Comment 19

3 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e40a9b0757b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/16b3bd0d5ebf
status-firefox35: affected → fixed
status-firefox36: affected → fixed
Flags: in-testsuite+
(Reporter)

Comment 21

3 years ago
Verified as fixed on latest Nightly build.
status-firefox36: fixed → verified
(Reporter)

Comment 22

3 years ago
Verified as fixed on latest Aurora build.
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified

Updated

3 years ago
Depends on: 1135102
You need to log in before you can comment on or make changes to this bug.