Closed Bug 1481296 Opened Last year Closed Last year

Add API for controlling pop-up blocking.

Categories

(GeckoView :: General, enhancement, P1)

enhancement

Tracking

(geckoview62 wontfix, firefox-esr60 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
geckoview62 --- wontfix
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: rbarker, Assigned: droeh)

References

Details

(Whiteboard: [geckoview:fxr:p1])

Attachments

(1 file, 2 obsolete files)

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?
Whiteboard: [geckoview:fxr]
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.
Whiteboard: [geckoview:fxr] → [geckoview:fxr:p1]
Eugen said he'd take a look.
Assignee: nobody → esawin
Assignee: esawin → nobody
FYI this needs a new owner. It can wait for triage unless you feel it is more urgent.
Flags: needinfo?(rbarker)
There are a lot of urgent things, this can probably wait until next triage.
Flags: needinfo?(rbarker)
Taking this over after talking with David yesterday.
Assignee: nobody → droeh
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?
Flags: needinfo?(droeh)
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.
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.
Flags: needinfo?(droeh)
Attached patch Popup blocking WIP (obsolete) — Splinter Review
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.
Attachment #9007318 - Flags: feedback?(snorp)
Attachment #9007318 - Flags: feedback?(rbarker)
Attachment #9007318 - Flags: feedback?(nchen)
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
Attachment #9007318 - Flags: feedback?(nchen) → feedback+
Does this bug affect Focus+GV? Will we need to uplift the fix to GV 62?
(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 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.
Attachment #9007318 - Flags: feedback?(rbarker) → feedback+
Attached patch Popup blocking API (obsolete) — Splinter Review
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.
Attachment #9007318 - Attachment is obsolete: true
Attachment #9007318 - Flags: feedback?(snorp)
Attachment #9010978 - Flags: review?(snorp)
Attachment #9010978 - Flags: review?(nchen)
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.
Attachment #9010978 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 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).
Attachment #9010978 - Flags: review?(nchen) → review+
Thanks, I forgot to fold in the Callbacks.kt changes before exporting the patch. Other issues are addressed, carrying over the r+'s.
Attachment #9010978 - Attachment is obsolete: true
Attachment #9012347 - Flags: review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50c67bb42d23
Add a popup blocking API to PromptDelegate. r=jchen,snorp
https://hg.mozilla.org/mozilla-central/rev/50c67bb42d23
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Dylan said he will uplift this API to GV 63 Beta. We should probably uplift the API's tests from bug 1494486, too.
Depends on: 1494486
(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 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:
Attachment #9012347 - Flags: approval-mozilla-beta?
Comment on attachment 9012347 [details] [diff] [review]
Popup blocking API

Uplift approved for 63 beta 12, thanks.
Attachment #9012347 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 64 → mozilla64
You need to log in before you can comment on or make changes to this bug.