Closed Bug 1339550 Opened 8 years ago Closed 7 years ago

Allow to change popup_allowed_events in WebExtensions

Categories

(WebExtensions :: Untriaged, defect, P2)

Product:
WebExtensions
Component:
Untriaged
Type:
defect

Tracking

(firefox57 verified, firefox58 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: mojtabad2003, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][browserSettings] triaged)

Attachments

(2 files)

Bug 1339550 - Implement browser.settings.allowPopupsForUserEvents,
(deleted), text/x-review-board-request
aswan
: review+
Details
Bug1339550.zip
(deleted), application/x-zip-compressed
Details
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID: 20161208153507 Steps to reproduce: Hi, I need a function to allow me to change value of popup_allowed_events in about:config I just need this function to migrate to WE. Sincerely Actual results: by setting popup_allowed_events to "empty" we prevent popups that triggered by mouse clicks. (Default popup blocker of browsers can't blocks them) Expected results: A function should be created to allow developers to change popup_allowed_events in about:config. Example: popupAllowedEvents(""); popupAllowedEvents("change click dblclick mouseup notificationclick reset submit touchend"); More than 160000 users need this function. and atleast 3 addons need this. Sincerely
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Whiteboard: [design-decision-needed]
Approved; P3.
Flags: needinfo?(amckay)
Flags: needinfo?(amckay)
Priority: -- → P3
Whiteboard: [design-decision-needed] → [design-decision-approved]
Whiteboard: [design-decision-approved] → [design-decision-approved]. triaged
Assignee: nobody → bob.silverberg
Blocks: 1363856
Priority: P3 → P2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Can this be considered a duplicate of bug 1364971? That bug will allow extensions to disable popups by setting the value of `dom.disable_open_during_load`. Will that satisfy the requirement described in this bug? More specific questions: 1. Do you only need to prevent popups, or do you have a requirement to be able to specify specific events for dom.popup_allowed_events? In your use case you discuss setting it to an empty string. 2. Will setting dom.disable_open_during_load to true prevent the popups you are concerned about? There seemed to be a suggestion in your bug description that there is something special about popups triggered by mouse clicks.
Flags: needinfo?(mojtabad2003)
Whiteboard: [design-decision-approved]. triaged → [design-decision-approved][browserSettings] triaged
No, It is not enough, 1-Yes,I need to empty dom.popup_allowed_events and reset it. this "dom.disable_open_during_load" is require too but it enabled by default. It's better to allow me to set both. 2- Both setting are important and I need to change them. I will send my API that I written via email to you. Please modify it to return these settings also. Sincerely
Flags: needinfo?(mojtabad2003)
Thanks Mojtaba. I'm still not sure whether we should combine this with bug 1364971 or not. I am now thinking that if we introduce a WebExtensions API which allows extensions to block popups, it should set both of these prefs. It should set "dom.disable_open_during_load" to `true` and "dom.popup_allowed_events" to an empty string. Olli, it looks like you are one of the experts in this area. Can you please comment on this proposal for a WebExtensions API to block popups? Does the above sound reasonable?
Flags: needinfo?(bugs)
I wonder if we should add some new API to control the behavior from extensions, but still keep the prefs for "expert" users to let them to have the ultimate control over the behavior.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #6) > I wonder if we should add some new API to control the behavior from > extensions, but still keep the prefs > for "expert" users to let them to have the ultimate control over the > behavior. This is an interesting idea. To date we have just relied on flipping prefs for certain behaviours. For example, the entire privacy API [1] works by flipping prefs behind the scenes. It is still possible for users to change the prefs manually, even after an extension has made changes, but that will cause the extension's wishes to become out of sync with the system. Do you think there is a desire to add such an API and if so a chance that it would be implemented before 57? If this is an API that we are going to deliver via WebExtensions we ideally would do so before 57. If an additional API to control popups is not implemented, and we do need to rely on flipping prefs, does the plan above sound like it will give us the results we expect? [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy
(In reply to Bob Silverberg [:bsilverberg] from comment #5) > Thanks Mojtaba. I'm still not sure whether we should combine this with bug > 1364971 or not. I am now thinking that if we introduce a WebExtensions API > which allows extensions to block popups, it should set both of these prefs. > > It should set "dom.disable_open_during_load" to `true` and > "dom.popup_allowed_events" to an empty string. > > Olli, it looks like you are one of the experts in this area. Can you please > comment on this proposal for a WebExtensions API to block popups? Does the > above sound reasonable? I confirm that two prefs are required to block popups. But keep in mind we can reset it again to disable blocking popups.so need to set its valut from API. The API that is sent via mail for you works correctly. But I couldn't return pref value via API. It is not require for my porpuse but it was just a question that why it didn't work in returning values. I don't know whats the next step for implementing that API to complete migrating my addon to WEs.
Just adding a needinfo regarding comment 7 so it doesn't fall off Olli's radar...
Flags: needinfo?(bugs)
(In reply to Mojtaba Daneshi from comment #8) > > I confirm that two prefs are required to block popups. > But keep in mind we can reset it again to disable blocking popups.so need to > set its valut from API. > The API that is sent via mail for you works correctly. But I couldn't return > pref value via API. > It is not require for my porpuse but it was just a question that why it > didn't work in returning values. > I don't know whats the next step for implementing that API to complete > migrating my addon to WEs. Mojtaba, I'm still waiting on a final confirmation from Olli that our approach is appropriate, and once I receive that I plan to proceed with implementing the API, so the experiment will be unnecessary. Even if you were to complete the experiment and get it all working properly, I doubt it is something that the majority of your users could use. Experiments can only be installed on non-release builds, so they are meant for experimenting, but they are not a particularly good solution for distributing functionality to a diverse group of users. Hopefully I will be able to proceed with this soon and if that's the case it should be available in a Nightly version of Firefox in the near future.
Use of popup_allowed_events is probably ok for now, if no one implements any API to achieve the same, since the pref isn't used by Firefox UI anywhere. So, it is really for expert users only, and addons might break that behavior already. The behavior of disable_open_during_load is less clear to me. FF UI uses that. Though, again, since addons may already use that pref, allowing that same thing for web extensions shouldn't be that bad. But perhaps worth to check with some FF UI dev?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #11) > Use of popup_allowed_events is probably ok for now, if no one implements any > API to achieve the same, since the pref isn't used by Firefox UI anywhere. > So, it is really for expert users only, and addons might break that behavior > already. > > The behavior of disable_open_during_load is less clear to me. FF UI uses > that. > Though, again, since addons may already use that pref, allowing that same > thing for web extensions shouldn't be that bad. But perhaps worth to check > with some FF UI dev? I think there are two issues here: 1. How are popup_allowed_events and disable_open_during_load related? I.e., if we want to allow WebExtensions to disable popups does it make sense to set the former to "" and the latter to `true`? I think you are saying this is correct, but I'm still not entirely sure. 2. disable_open_during_load is available to users via the Preferences UI, so we need to deal with the fact that users can change this setting themselves, outside of a WebExtension. That is something that we are not discussing separately. Olli, are you able to confirm item #1, or should I ask that of someone else?
Flags: needinfo?(bugs)
I didn't say anything about how the prefs are used ;) But yes, the explanation sounds right. But perhaps you just want to use permission manager and its popup permission? http://searchfox.org/mozilla-central/source/extensions/cookie/nsPopupWindowManager.cpp is after all the code using
Flags: needinfo?(bugs)
We're going to tackle this via permissions, rather than by changing a pref, so I am going to mark this as a duplicate of bug 1364971. Once that has been implemented, allowing an extension to disable pop-ups globally via permissions, it should also cover this use case.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
This is not duplicate. It can't be reach via Permissions. Please just implement a function for changing prefs. and revert them on uninstalling.
This is not a true duplicate of bug 1364971 because, as Mojtaba mentions above, permissions don't seem to work to block popups from mouse clicks, but setting "popup_allowed_events" to "" does seem to work. Olli, I'm still a bit confused about why this is, and how "popup_allowed_events" actually interacts with permissions. Are you able to enlighten me on that? I suppose what we can do for this is introduce a browserSettings API called enablePopupsOnEvents, which can be set to "false" which would clear out the value of the "popup_allowed_events" pref, and if set to "true" would simply reset the "popup_allowed_events" pref. That should fulfill the requirement of the reporter and the add-ons that need it. Olli, Your opinion on this would be appreciated.
Status: RESOLVED → REOPENED
Flags: needinfo?(bugs)
Resolution: DUPLICATE → ---
Hey Mojtaba! I just sent you an email, but it looks like your add-on is back in the queue for review and my understanding is that it looks like the issues were fixed. We are keeping an eye on it. :)
I would need to read the code here just like anyone else. I don't have strong opinion here.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #19) > I would need to read the code here just like anyone else. I don't have > strong opinion here. When can I use it?
> Why my post removed? This bug is exclusively for discussing the proposed API. Review issues need to be discussed with reviewers.
(In reply to Bob Silverberg [:bsilverberg] from comment #16) > This is not a true duplicate of bug 1364971 because, as Mojtaba mentions > above, permissions don't seem to work to block popups from mouse clicks, but > setting "popup_allowed_events" to "" does seem to work. > > Olli, I'm still a bit confused about why this is, and how > "popup_allowed_events" actually interacts with permissions. Are you able to > enlighten me on that? > > I suppose what we can do for this is introduce a browserSettings API called > enablePopupsOnEvents, which can be set to "false" which would clear out the > value of the "popup_allowed_events" pref, and if set to "true" would simply > reset the "popup_allowed_events" pref. That should fulfill the requirement > of the reporter and the add-ons that need it. > > Olli, Your opinion on this would be appreciated. I have stress about migrating to Web Extension, Is it available at least a month before NOVEMBER?
The popup_allowed_events pref controls from which types of user interactions popups can be opened. The code that implements this can be found at [1]. Setting this pref to "" will disallow popups from all user events, and the result of this will be that Firefox will present the user with the "Firefox prevented this site from opening a pop-up window" doorhanger when a pop-up is attempted from a user interaction. This doorhanger gives the user the ability to override this via a permission, which is a nice workaround for a user who does in fact want to allow pop-ups for the given site/page. I'm going to suggest we call this setting enablePopupsForUserEvents, which will have a boolean value. When set to `false` it will set `popup_allowed_events` to "", and when set to `true` it will reset `popup_allowed_events` to its default value. I think we can now proceed with implementing this API. Note that the API to interact with site permissions for pop-up windows is something separate which is being tracked by bug 1364971. [1] https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/events/Event.cpp#695-893
Comment on attachment 8892168 [details] Bug 1339550 - Implement browser.settings.allowPopupsForUserEvents, https://reviewboard.mozilla.org/r/163142/#review168550
Attachment #8892168 - Flags: review?(aswan) → review+
Pushed by bsilverberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b89bf45144d8 Implement browser.settings.allowPopupsForUserEvents, r=aswan
https://hg.mozilla.org/mozilla-central/rev/b89bf45144d8
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Hi Thanks for implementing this API, I downloaded latest version of FF Nightly [56.0b1 (32-bit)] then added "browserSettings" : "permissions": [ "storage", "tabs", "<all_urls>", "notifications", "browserSettings" ], then in my code : browser.settings.allowPopupsForUserEvents(false); then get this error: Unchecked lastError value: Error: browser.settings is undefined ExtensionCommon.jsm:304 What's problem? Is that implemented in latest FF Nightly? 56.0b1 (32-bit)
You need to call it like so: browser.browserSettings.allowPopupsForUserEvents.set({value: false}); Note that this returns a promise, so you may want to `await` it. This is documented in general for BrowserSetting.set() at [1], but the specific documentation for the browser.browserSettings API has not been published yet. [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting/set
Keywords: dev-doc-needed
Looks good, thanks Will!
Flags: needinfo?(bob.silverberg)
Attached file Bug1339550.zip (deleted) —
I can reproduce this issue on Firefox 56.0.2 (20171024165158) under Wind 7 64-bit. The next error message is displayed in the browser console: browser.browserSettings.allowPopupsForUserEvents is undefined popup.js:7 This issue is verified as fixed on Firefox 58.0a1 (2017-11-06) and Firefox 57.0b14 (20171102181127) under Wind 7 64-bit and Mac OS X 10.13. Please see the attached video.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: --- → verified
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: