Closed Bug 1124942 Opened 5 years ago Closed 5 years ago
Implement popup blocker UI
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.
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.
Summary: window.open() without user action doesn't work → Implement popup blocker UI
Don't remember much discussion about popup blocking UI or settings, so setting tracking-fennec to get this on the radar.
tracking-fennec: --- → ?
Assignee: nobody → bnicholson
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.
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.
(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).
I'm not sure where to put constants atm, so expect to be dinged on having magic strings in a couple of places.
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.
Addressed nits, and merged.
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.
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.
Attachment #8616740 - Flags: review?(sarentz) → review+
You need to log in before you can comment on or make changes to this bug.