Closed Bug 1124942 Opened 5 years ago Closed 5 years ago

Implement popup blocker UI

Categories

(Firefox for iOS :: Browser, defect)

All
iOS 8
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec + ---

People

(Reporter: bnicholson, Assigned: jhugman)

References

Details

Attachments

(2 files)

STR:
1) Go to http://people.mozilla.com/~bnicholson/test/window.html
2) Wait 5 seconds

A new google.com tab should open (or, a popup blocker notification should appear). Nothing happens, and our webView createWebViewWithConfiguration method isn't called. This may be a WKWebView issue, since Safari shows the same behavior, and Chrome does not.
Maybe this has something to do with WKPreferences.javaScriptCanOpenWindowsAutomatically ?

"A Boolean value indicating whether JavaScript can open windows without user interaction. The default value is false in iOS and true in OS X."
The test page works in Safari if I turn 'Block Popups' off in Settings -> Safari.

With Block Popups off, I get a confirmation dialog 'This site is attempting to open a pop-up window. [Block] [Allow]'.

Highly likely that this is connected to WKPreferences.javaScriptCanOpenWindowsAutomatically.

We should probably do the same and make this an (advanced?) setting?
Cool, good find. Rather than silently hide the popup like Safari, we should probably show a notification like we do on desktop/Android that allows the user to control the setting for the site.
Can we do a tri-state global setting for this? Yes/No/Ask?
Morphing this bug a bit. As mentioned in comment 2, it should be easy enough to flip this setting on so popups appear, but we'll need some kind of UI to prevent popup spam.
Mentor: bnicholson
Summary: window.open() without user action doesn't work → Implement popup blocker UI
Blocks: iosbrowsingui
No longer blocks: 1123501
Don't remember much discussion about popup blocking UI or settings, so setting tracking-fennec to get this on the radar.
tracking-fennec: --- → ?
tracking-fennec: ? → +
Blocks: 1130526
No longer blocks: iosbrowsingui
Assignee: nobody → bnicholson
Mentor: bnicholson
WKPreferences.javaScriptCanOpenWindowsAutomatically seems to be an all or nothing thing. 

If we want to offer the user some differential control, or want to be block on a per-URL basis, then we need to do something more different (e.g. checking for WKNavigationType in WKNavigationDelegate of BrowserViewController).
WKPreferences javaScriptCanOpenWindowsAutomatically blocks a few classes of popups (see http://popuptest.com ).

I can't see a way of detecting which popups would be opened or even that they have been blocked, without doing a bunch of bookwork in BrowserViewController.WKNavigationDelegate.

You /can/ reset `webView.configuration.preferences.javaScriptCanOpenWindowsAutomatically` at anytime, and it will be respected after it is set. We could enable this on a site by site basis. However, without a way of detecting blocking, populating the block/noblock list will be painful for the user.

If we can't detect popups that have been blocked, then the minimum we should do is a checkbox in prefs.
Assignee: bnicholson → jhugman
Status: NEW → ASSIGNED
Adding a toggle in SettingsViewController.

This will toggle the profile prefs object. onClick also needs to mutate the existing tabs, via the tabManager.
Currently, we're not using NSUserDefaults. However, existing settings/prefs are using a collection of actions (e.g. Clear Private Data).

The exception here is Search engine prefs, which uses our own 'Prefs'. Why don't we use NSUserDefaults?

Added a KVO addObserver to the Prefs object, so new Browser objects can listen for a change of preference.
NSUserDefaults being used in the implementation NSUserDefaultsProfilePrefs.
James, I saw your comment on our WebKit Wish List https://etherpad.mozilla.org/LAbr1f81wN


"It is not possible to detect popup blocking when it has happened via javaScriptCanOpenWindowsAutomatically. It is not possible to overide this on a per site basis."


Is this true? Why can't we set javaScriptCanOpenWindowsAutomatically to true and then put our decision logic into - webView:createWebViewWithConfiguration:forNavigationAction:windowFeatures: ?

That method would then always be called when WebKit asks us to create a window for a popup (or _target="blank"). We can return nil to ignore the request.

We can see that this is a request for a popup window by looking at navigationAction.targetFrame, which is nil in case of a popup. (I guess that is always the case when this is called)

The originating site is in navigationAction.sourceFrame and possibly navigationAction.request.

I think that gives us enough info to show a Snackbar to confirm the action? We would just have to hold on to the naviationAction and the configuration for a while until the user confirms or rejects the request. But I think that is ok since both are probably pretty lightweight.
Implemented the suggested from comment #12, testing with http://popuptest.com 

Unfortunately, this overblocks – none of the links in http://popuptest.com/goodpopups.html when they should.

We can further filter on navigationAction.navigationType, but this will still overblock.

And this is what concerns me: we can go further and further into the 'it's just…' but we're a) /at best/ duplicating what's already in Webkit, b) and /at worst/ overblocking or underblocking popups.
Small patch demonstrating overblocking of popups. We're calling it a popup if there is no targetFrame.

Works just fine with the 'bad' popups from http://popuptest.com, however, fails for http://popuptest.com/goodpopups.html

The navigation type is Other (-1) on both good popups and bad so this on its own, these two tests are insufficient.
Flags: needinfo?(sarentz)
(Why not put this on a PR branch? We don't really use attached patches.)

James, I think you can completely skip the navigationAction.targetFrame == nil check because that seems to be always the case. No matter what kind of popup or window is being opened.

I would just look at navigationAction.navigationType and only allow the new tab to be created in case of WKNavigationTypeLinkActivated. That looks only to be happening on user interaction on a <a href= target="_blank"> and not on JS being executed.
Flags: needinfo?(sarentz) → needinfo?(jhugman)
Checking navigationAction.navigationType isn't sufficient: in the case of bad popups, and good, the navigation type is WKNavigationTypeOther.

If we disallow everything but WKNavigationTypeLinkActivated, this ends in us not being able to do things like Persona logins.

(sorry patch; out of habit and time constraints).
Flags: needinfo?(jhugman)
Two strategies: 

 * use javaScriptCanOpenWindowsAutomatically, and not be able to offer the user opening a blocked popup - because we don't get a hint from the webview. Any UI is limited to the preferences.
 * don't use javaScriptCanOpenWindowsAutomatically, and re-implement popup detection in the manner we're trying to do here.

I'm going to do the former, while looking out for ways of doing the latter.
James, this came up in triage today. Is it possible to land a first iteration of this with the following properties:

* We expose a global 'Block Pop-ups' setting
* This setting maps directly to the WKPreferences.javaScriptCanOpenWindowsAutomatically property

So, no per-site. Nothing fancy. This would be the same as Safari does.

We can do a more complicated implementation after v1.
Flags: needinfo?(jhugman)
I'm not sure where to put constants atm, so expect to be dinged on having magic strings in a couple of places.
Flags: needinfo?(jhugman)
Attachment #8616740 - Flags: review?(sarentz)
Attachment #8616740 - Flags: review?(bnicholson)
Comment on attachment 8616740 [details] [review]
Exposed javaScriptCanOpenWindowsAutomatically to the user.

This looks good to me, but can we add a UI test? Should be fairly straightforward to include a minimal page that triggers a popup (like in the test case from comment 0), then testing it for both pref cases.
Attachment #8616740 - Flags: review?(bnicholson) → review+
Will add a KIFTest, and land.
This is great. Couple of observations:

* The popups from http://www.popuptest.com/ are correctly blocked!
* When logging in with Persona on https://mozillians.org, the login window correctly pops up and lets me login

There is one issue, but I think that is not related this bug, and I will file a follow up: when you finish the Persona login, the tab in which Persona ran is supposed to automatically close. But it does not. The login was succesfull though because when I manually close it and go back to mozillians.org, i am logged in.

Not sure what piece we miss, may be some WKWebView delegate that we need to implement.
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1172928 as a followup. Lets fix that in a separate bug.
Added a view more comments to the pull request about some small fixes that we need.
Flags: needinfo?(jhugman)
Addressed nits, and merged.
Flags: needinfo?(jhugman)
James, if still possible, would you mind rolling back the commit and then rebase and squash the 7 individual commits in your PR branch into a single one with subject "Fixes 1124942 - Implement popup blocker UI".

I see the note about the prefsDidChange notification was not addresses. So you are chaning the webview settings when you receive that notification, but the Prefs actually never sends it. So that still needs to be implemented. Either as part of this bug or as a followup bug.
Flags: needinfo?(jhugman)
I'm not sure I understand the problem with prefsDidChange – it's not a notification, it's a selector being called by the notification center.

The notification is being sent by changing the user defaults in /via/ the prefs.setObject call at https://github.com/mozilla/firefox-ios/commit/946320008ecd75b42a566d1ed9eba82ffca9b655#diff-6209ecb7e871159425f66f16eed90d7dR442
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jhugman) → needinfo?(sarentz)
Resolution: --- → FIXED
(In reply to James Hugman [:jhugman] [@jhugman] from comment #27)
> I'm not sure I understand the problem with prefsDidChange – it's not a
> notification, it's a selector being called by the notification center.

You are right, ignore my comment.
Flags: needinfo?(sarentz)
Comment on attachment 8616740 [details] [review]
Exposed javaScriptCanOpenWindowsAutomatically to the user.

Looks good to me. Next time please make sure to squash the commits into one single commit before merge please.
Attachment #8616740 - Flags: review?(sarentz) → review+
You need to log in before you can comment on or make changes to this bug.