1481296 - Add API for controlling pop-up blocking.

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Randall Barker [:rbarker]]

GeckoView does not seem to have a way to control pop-up blocking. This breaks things like facebook login on pinterest. Possibly add support via the Prompt Delegate?

Comment 1

[Chris Peterson [:cpeterson]]

Fennec shows a pop-up warning. Chrome doesn't. We shouldn't block pop-ups or show a warning for pop-ups on user action.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Eugen said he'd take a look.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

FYI this needs a new owner. It can wait for triage unless you feel it is more urgent.

Comment 4

[Randall Barker [:rbarker]]

There are a lot of urgent things, this can probably wait until next triage.

Comment 5

[Dylan Roeh (:droeh) (he/him)]

Taking this over after talking with David yesterday.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

I wonder if we just want to disable the built-in popup blocking for GV. Apps should be able to implement this themselves via onLoadRequest(), right?

Comment 7

[Randall Barker [:rbarker]]

I don't think there is any way for the embedding apps to know if the load was triggered by a user action or a js action.

Comment 8

[Dylan Roeh (:droeh) (he/him)]

In addition to Randall's point, I think pop-up blocking is going to be a common enough desire that it seems like a bad idea to effectively disable Gecko's pop-up blocking and force every app to handle it. I guess you could make an argument for it being an android component, but that still seems kind of like reinventing the wheel when Gecko already does it.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Attachment: Popup blocking WIP

Here's a first pass at pop-up blocking; we listen for DOMPopupBlocked events, cache the popup, and (ultimately) call a delegate to decide whether or not to allow the popup. On response from the delegate, we attempt to open the popup (if desired) and delete it from the cache either way.

The GeckoSession API for this is minimal: you get a target URI string, and return a GeckoResult<Boolean> indicating whether or not to allow the popup. I think it's best to leave higher level features (like whitelisting popups from some domains, etc) to be implemented by the app on top of this.

I've tested Randall's use-case of Facebook login for Pinterest in GVE and everything seems to work correctly (note that GVE is modified in this patch to always allow popups).

Some existing issues:
* We need to periodically clean up the cache to avoid leaking memory from non-responses.
* When opening a popup in GVE we get a hung progress bar, even though the page seems to load correctly otherwise.
* The naming is a bit confusing at the moment possibly -- onPopupRequest is the name of the function, and I've gone with the convention that returning true allows the popup while allowing false blocks it, but there may be a better/clearer convention to use here.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9007318 [details] [diff] [review]
Popup blocking WIP

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

- <browser> already keeps a list of popups [1] and sends the "DOMUpdateBlockedPopups" event. It seems unnecessary to keep another GV-specific list.

- I think consumers are looking for more of a prompt API than a request API (see comment 0); i.e. this feels like it should be in PromptDelegate.

- It's probably better to use event callbacks (i.e. sendRequestForResult) than to keep a list of popups in `this._popupData`.

[1] https://searchfox.org/mozilla-central/rev/6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/toolkit/content/widgets/browser.xml#931

Comment 11

[Chris Peterson [:cpeterson]]

Does this bug affect Focus+GV? Will we need to uplift the fix to GV 62?

Comment 12

[Dylan Roeh (:droeh) (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> Does this bug affect Focus+GV? Will we need to uplift the fix to GV 62?

AFAIK this was considered a P2 for Focus+GV, and Focus devs would likely need to do some UI work on their end even if we uplifted to 62. I wouldn't really recommend going that route. 63 is a more reasonable target.

Comment 13

[Randall Barker [:rbarker]]

Comment on attachment 9007318 [details] [diff] [review]
Popup blocking WIP

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

I agree with :jchen, I would expect it to be part of the prompt delegate.

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Attachment: Popup blocking API

Alright, moved it to PromptDelegate and made changes according to Jim's feedback.

One note: without adding strings to BasicGeckoViewPrompt it's hard to have a sensible default behavior here, for the moment I've opted to just always allow for ease of manual testing in GVE.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9010978 [details] [diff] [review]
Popup blocking API

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

I think it would be nicer if we could route this through onNewSession() by adding some popup information there. That would allow the app to easily do something special with a GeckoSession if it wishes (like not immediately show it in a new tab).

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +2004,5 @@
> +
> +                res.then(new GeckoResult.OnValueListener<Boolean, Void>() {
> +                    @Override
> +                    public GeckoResult<Void> onValue(Boolean value) throws Throwable {
> +                        ThreadUtils.assertOnUiThread();

I'm not sure if this is necessary

@@ +3089,5 @@
>          void onFilePrompt(GeckoSession session, String title, @FileType int type,
>                            String[] mimeTypes, FileCallback callback);
> +
> +        /**
> +         * Display a popup request prompt.

This doesn't display anything, but I guess that phrasing is the convention for other stuff in the class...

Still, maybe a bit more detail on what causes this would be good. I'm not sure if everyone understands the circumstances under which a new would be considered a popup.

@@ +3097,5 @@
> +         *
> +         * @return A {@link GeckoResult} resolving to a Boolean which indicates
> +         *         whether or not the popup should be allowed to open.
> +         */
> +        GeckoResult<Boolean> onPopupRequest(GeckoSession session, String targetUri);

I think this is a good opportunity to introduce some new ALLOW/DENY constants instead of booleans. As we discussed before, I think people have a hard time remembering whether `true` means "I handled it" or "I want to allow it". Clear ALLOW/DENY or BLOCK/CONTINUE constants (maybe we have both sets?) would be help.

Comment 16

[Dylan Roeh (:droeh) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #15)
> I think this is a good opportunity to introduce some new ALLOW/DENY
> constants instead of booleans. As we discussed before, I think people have a
> hard time remembering whether `true` means "I handled it" or "I want to
> allow it". Clear ALLOW/DENY or BLOCK/CONTINUE constants (maybe we have both
> sets?) would be help.

I thought about this, but ultimately decided it would make more sense to do this in a separate bug and replace all of these sort of booleans at once, rather than introducing an inconsistency in the API. Do you think there are advantages to introducing ALLOW/DENY constants for just this delegate first?

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9010978 [details] [diff] [review]
Popup blocking API

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

Does this build? I don't see changes for "Callbacks.kt"

::: mobile/android/components/geckoview/GeckoViewPrompt.js
@@ +284,5 @@
> +    prompt.asyncShowPrompt({
> +      type: "popup",
> +      targetUri: popupWindowURISpec
> +    }, allowed => {
> +      if (allowed && dwi && dwi.document == aEvent.requestingWindow.document) {

Why would `dwi.document != aEvent.requestingWindow.document` here? Can `aEvent.requestingWindow` change?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1990,5 @@
>                  delegate.onFilePrompt(session, title, intMode, mimeTypes, cb);
>                  break;
>              }
> +            case "popup": {
> +                if (GeckoAppShell.isFennec()) {

I don't think we're ever in Fennec here (Fennec doesn't use the prompt delegate).

Comment 18

[Dylan Roeh (:droeh) (he/him)]

Attachment: Popup blocking API

Thanks, I forgot to fold in the Callbacks.kt changes before exporting the patch. Other issues are addressed, carrying over the r+'s.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50c67bb42d23
Add a popup blocking API to PromptDelegate. r=jchen,snorp

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/50c67bb42d23

Comment 21

[Chris Peterson [:cpeterson]]

Dylan said he will uplift this API to GV 63 Beta. We should probably uplift the API's tests from bug 1494486, too.

Comment 22

[Dylan Roeh (:droeh) (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #21)
> Dylan said he will uplift this API to GV 63 Beta. We should probably uplift
> the API's tests from bug 1494486, too.

Yup. I'll put in the beta approval requests for both once I've landed the tests.

Comment 23

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9012347 [details] [diff] [review]
Popup blocking API

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: There will be no way for GeckoView apps to allow blocked popups.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1494486

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Currently, all popups in a GV app will be silently blocked; this gives apps the ability to unblock the popups. Any app wishing to maintain the previous behavior can simply block all popups in the onPopupRequest delegate function.

String changes made/needed:

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9012347 [details] [diff] [review]
Popup blocking API

Uplift approved for 63 beta 12, thanks.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2b5cb2f178c