We should allow some notifications, in particular the permission prompts, to display the secondary action as a top-level choice outside of the drop-down menu. This depends on having the necessary breadcrumbs to find where the decision can be reverted, in particular the blocked permission icons in the identity block.

I don't understand the need to remove the drop down menu for the laggard prompts. Since option A is about doing the least amount of work possible, why not leave it as it is? Also, if we take out "Not Now" as we planned, don't most cases boil down to Accept and Reject anyway (without a need for a drop down)?

(In reply to Panos Astithas [:past] from comment #1) > I don't understand the need to remove the drop down menu for the laggard > prompts. Since option A is about doing the least amount of work possible, > why not leave it as it is? The minimum amount of work is to remove the dropdown for the permission prompts only. There might be easy cases for other prompts though. > Also, if we take out "Not Now" as we planned, don't most cases boil down to > Accept and Reject anyway (without a need for a drop down)? The implications of moving the secondary option (when there is one) out of the drop-down menu can be significant or minimal, based on the prompt. Permission prompts have a clear design where we analyzed how to restore a "deny" decision. The same analysis has not been done for other prompts.

I have read this many times and I still don't understand your reasoning. I believe this is because you speak in abstract terms, which apart from making understanding challenging doesn't make this an actionable bug. Please either clarify the work needed to be done here so that someone outside the team can jump in and do it, or resolve this (or fold it in another permission-specific bug).

Screenshots can help! Here is how a permissions notification would approximately look when bug 1267604 lands: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/1a7ee99af5d06ef7b3bc79e499f109122f473b534d80c8a33fa30fd007a94650c41384ff90543d48f56e230b7bbc5777bd8192b4dfdc47edb34abc782a52d032 And here is how it should look in the end: https://mozilla.invisionapp.com/share/9J7RMC2G7#/screens/170569580 This bug is about the fact that in the first screenshot you see an empty space to the left of the main icon, and a dropdown arrow near the main action. For permission notifications, you should see the secondary action and no dropdown arrow instead.

Geolocation, Camera and Microphone, Desktop Notifications, Pointer Lock and Offline Storage are the permission notifications that will surely be changed here.

I can investigate the functional changes while the restyling in bug 1267604 is in progress.

Renaming this to underline that in this bug we intend to not show the dropdown menu for secondary actions anymore (and incorporate the checkbox that will be added in Bug 1291642).

We'll need to figure out what to do with security.notification_enable_delay. The simplest solution would be to have it apply to both of the buttons I guess.

(In reply to Matthew N. [:MattN] from comment #15) > We'll need to figure out what to do with security.notification_enable_delay. > The simplest solution would be to have it apply to both of the buttons I > guess. The latest version of the patch implements this approach.

What's the purpose of popup-notification-button-wrapper? Can you get rid of that box? The --panel-background-color variable name makes no sense as the panel background color is something else (see --panel-arrowcontent-background).

(In reply to Dão Gottwald [:dao] from comment #19) > What's the purpose of popup-notification-button-wrapper? Can you get rid of > that box? I'm using it to force the secondary button + drop marker combo (when the latter is visible) to fit at 50% of the container width. This is the spec page that calls for this: https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290118 > The --panel-background-color variable name makes no sense as the panel > background color is something else (see --panel-arrowcontent-background). Thanks, I've renamed it to --panel-arrowcontent-footer-background (and renamed --panel-background-active-color to --panel-arrowcontent-footer-active-background).

(In reply to Panos Astithas [:past] from comment #20) > (In reply to Dão Gottwald [:dao] from comment #19) > > What's the purpose of popup-notification-button-wrapper? Can you get rid of > > that box? > > I'm using it to force the secondary button + drop marker combo (when the > latter is visible) to fit at 50% of the container width. This is the spec > page that calls for this: > > https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290118 Can you please rename it to that end? A "button wrapper" in a "button container" seems like arbitrary and unhelpful naming.

Actually scratch that, I've figured out a way to avoid it by removing min-width. Thanks for the suggestion!

Comment on attachment 8782171 [details] Bug 1282768 - Convert permission notifications to use two buttons instead of the drop-down menu. https://reviewboard.mozilla.org/r/72386/#review75838 ::: toolkit/modules/PopupNotifications.jsm:702 (Diff revision 5) > + popupnotification.setAttribute("secondarybuttonlabel", secondaryAction.label); > + popupnotification.setAttribute("secondarybuttonaccesskey", secondaryAction.accessKey); > + popupnotification.setAttribute("secondarybuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand');"); > + > + for (let i = 1; i < n.secondaryActions.length; i++) { > + let a = n.secondaryActions[i]; This would be a good opportunity to rename "a" to action and maybe even "n" to notification or something like that.

Rebased on top of bug 1291642 and incorporated the additional patches from that bug that didn't land.

Fixed some strings per the UX spec, some lingering questions about others still remain. Fixed the checkbox operation, but there is still a bug with the warning label that I need to squash.

In this last revision I have fixed everything I could find except tests and handling of the Escape key to close the doorhanger and reject the request. Try run with the latest builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41d036538f5d

Fixed the things that were pointed out at the demo during the planning meeting. Tests and Escape still remain, as well as checking the behavior in PBM.

This is the patch that keeps the close button in the permission prompts, but makes it reject the request. We are going to test both approaches in a user study soon, so I'm parking it here in case we end up going with this approach.

Rebased on top of mconley's PermissionUI.jsm changes, fixed a couple of issues that I spotted.

Started fixing tests, more to come.

I think any remaining test failures now should be intermittents, but let's see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14d8bf38ea1f This should be ready for a first round of reviews, as we discussed. One thing I haven't implemented is handling Escape to reject the request and close the prompt, I hope we could do that in a followup. Also, this was only tested on macOS, I'd appreciate it if you guys could give it a try on Windows/Linux, too.

Comment on attachment 8782171 [details] Bug 1282768 - Convert permission notifications to use two buttons instead of the drop-down menu. https://reviewboard.mozilla.org/r/72386/#review83164 Looks good overall (would be f+ if reviewboard let me set it). I'm not thrilled by the "Will you allow" new set of strings; I don't see how they are better than the current "Would you like to" ones, but I think you just followed the spec here. ::: browser/base/content/browser.js:6165 (Diff revision 16) > // Only listen for notifications for browsers in our chrome window. > return; > } > > - var host = browser.currentURI.asciiHost; > + // Get the host name if available or the file path otherwise. > + var host = browser.currentURI.asciiHost || browser.currentURI.path; .path is potentially a huge string for data urls. Is this going to be OK in the UI, or do we need to find a way to truncate excessively long strings? ::: browser/locales/en-US/chrome/browser/browser.properties:375 (Diff revision 16) > -# If you're having trouble with the word Share, please use Allow and Block in your language. > -geolocation.shareLocation=Share Location > -geolocation.shareLocation.accesskey=a > -geolocation.alwaysShareLocation=Always Share Location > -geolocation.alwaysShareLocation.accesskey=A > -geolocation.neverShareLocation=Never Share Location > +geolocation.allowLocation.accesskey=A > +geolocation.dontAllowLocation=Don’t Allow > +geolocation.dontAllowLocation.accesskey=n > +geolocation.shareWithSite3=Will you allow %S to access your location? > +geolocation.shareWithFile3=Will you allow this local file to access your location? > +geolocation.remember=Remember this decision Do we really want the "Remember this decision" string to be translated for each prompt? Is there a way to make this the default value that PopupNotifications.jsm would fetch if nothing has been specified? ::: browser/locales/en-US/chrome/browser/browser.properties:383 (Diff revision 16) > -geolocation.shareWithFile2=Would you like to share your location with this file? > - > -webNotifications.receiveForSession=Receive for this session > -webNotifications.receiveForSession.accesskey=s > -webNotifications.alwaysReceive=Always Receive Notifications > -webNotifications.alwaysReceive.accesskey=A > +webNotifications.rememberForSession=Remember decision for this session > +webNotifications.allow=Allow Notifications > +webNotifications.allow.accesskey=A > +webNotifications.dontAllow=Don’t Allow > +webNotifications.dontAllow.accesskey=n > +webNotifications.receiveFromSite2=Will you allow %S to send notifications? This string is surprising. We are changing "would you like to receive notifications" to "allow to send notifications". Send to whom? ::: browser/locales/en-US/chrome/browser/browser.properties:471 (Diff revision 16) > social.error.closeSidebar.accesskey=C > > # LOCALIZATION NOTE: %1$S is the label for the toolbar button, %2$S is the associated badge numbering that the social provider may provide. > social.aria.toolbarButtonBadgeText=%1$S (%2$S) > > # LOCALIZATION NOTE (getUserMedia.shareCamera.message, getUserMedia.shareMicrophone.message, This localization note needs updating. ::: browser/locales/en-US/chrome/browser/browser.properties:478 (Diff revision 16) > # getUserMedia.shareScreenAndMicrophone.message, getUserMedia.shareCameraAndAudioCapture.message, > # getUserMedia.shareAudioCapture.message, getUserMedia.shareScreenAndAudioCapture.message): > # %S is the website origin (e.g. www.mozilla.org) > -getUserMedia.shareCamera.message = Would you like to share your camera with %S? > -getUserMedia.shareMicrophone.message = Would you like to share your microphone with %S? > -getUserMedia.shareScreen.message = Would you like to share your screen with %S? > +getUserMedia.shareCamera2.message = Will you allow %S to use your video camera? > +getUserMedia.shareMicrophone2.message = Will you allow %S to use your microphone? > +getUserMedia.shareScreen2.message = Will you allow %S to see your screen or application window? Why are we adding "or application window" here? I don't find the "application window" wording very clear, and the "or" makes me (as a potential user) wonder if it's the site that decides if it's the screen or an application window that's going to be shared. ::: browser/locales/en-US/chrome/browser/browser.properties:506 (Diff revision 16) > # Replacement for #1 is the name of the application. > # Replacement for #2 is the number of windows currently displayed by the application. > getUserMedia.shareApplicationWindowCount.label=#1 (#2 window);#1 (#2 windows) > -# LOCALIZATION NOTE (getUserMedia.shareSelectedDevices.label): > -# Semicolon-separated list of plural forms. See: > -# http://developer.mozilla.org/en/docs/Localization_and_Plurals > +getUserMedia.allowScreen.label = Allow Screen Access > +getUserMedia.allowWindow.label = Allow Window Access > +getUserMedia.allowApplication.label = Allow Application Access I find these Allow {Screen,Window,Application} Access strings confusing. What does accessing the screen means? Does it mean display something on it? Accessing window could mean listing existing windows and sending messages to them. Accessing application could be communicating data with the application, or taking control of the application. What's the rationale for changing these strings? ::: browser/locales/en-US/chrome/browser/browser.properties:509 (Diff revision 16) > -# LOCALIZATION NOTE (getUserMedia.shareSelectedDevices.label): > -# Semicolon-separated list of plural forms. See: > -# http://developer.mozilla.org/en/docs/Localization_and_Plurals > -# The number of devices can be either one or two. > -getUserMedia.shareSelectedDevices.label = Share Selected Device;Share Selected Devices > -getUserMedia.shareSelectedDevices.accesskey = S > +getUserMedia.allowScreen.label = Allow Screen Access > +getUserMedia.allowWindow.label = Allow Window Access > +getUserMedia.allowApplication.label = Allow Application Access > +getUserMedia.allowAccess.label = Allow Access > +getUserMedia.allowAccess.accesskey = A > +getUserMedia.dontAllow.label = Don’t Allow I don't like this "Don't Allow" string. Negations are cognitively hard to process. Could this string say "Deny" instead? But I see the previous "Don't Share" string had the same issue... ::: browser/locales/en-US/chrome/browser/browser.properties:512 (Diff revision 16) > -# The number of devices can be either one or two. > -getUserMedia.shareSelectedDevices.label = Share Selected Device;Share Selected Devices > -getUserMedia.shareSelectedDevices.accesskey = S > -getUserMedia.shareScreen.label = Share Screen > -getUserMedia.shareApplication.label = Share Selected Application > -getUserMedia.shareWindow.label = Share Selected Window > +getUserMedia.allowAccess.label = Allow Access > +getUserMedia.allowAccess.accesskey = A > +getUserMedia.dontAllow.label = Don’t Allow > +getUserMedia.dontAllow.accesskey = D > +getUserMedia.remember=Remember this decision > +# LOCALIZATION NOTE (getUserMedia.dontAllowHTTP, getUserMedia.dontAllowHTTPAudio, getUserMedia.dontAllowHTTPVideo) - %S is brandShortName This l10n note missed the last 2 strings (dontAllowScreenAlways and dontAllowAudioAlways). The choice of the "dontAllowHTTP" surprises me here, it sounds like a user action. I would have picked something like "noPersistentDeviceAccessOverInsecureConnection". But maybe it's just me :-). ::: browser/modules/PermissionUI.jsm:433 (Diff revision 16) > + options.checkbox = { > + show: !PrivateBrowsingUtils.isWindowPrivate(this.browser.ownerGlobal) > - }; > + }; > + } > + > + if (options.checkbox) { This test is always true, you meant to test options.checkbox.show ::: browser/modules/PermissionUI.jsm:493 (Diff revision 16) > - callback: function() { > + callback: function(state) { > + if (state && state.checkboxChecked) { > secHistogram.add(NEVER_SHARE); > - }, > - }); > - } > + } Should we record something for the non-persistent "block" case? See also http://searchfox.org/mozilla-central/rev/e8dd21b1673c07307068303ab3e99360c78f4d12/security/manager/ssl/nsISecurityUITelemetry.idl#96 ::: browser/modules/PermissionUI.jsm:570 (Diff revision 16) > - { > + { > - label: gBrowserBundle.GetStringFromName("webNotifications.receiveForSession"), > + label: gBrowserBundle.GetStringFromName("webNotifications.allow"), > - accessKey: > + accessKey: > - gBrowserBundle.GetStringFromName("webNotifications.receiveForSession.accesskey"), > + gBrowserBundle.GetStringFromName("webNotifications.allow.accesskey"), > - action: Ci.nsIPermissionManager.ALLOW_ACTION, > + action: Ci.nsIPermissionManager.ALLOW_ACTION, > - expireType: Ci.nsIPermissionManager.EXPIRE_SESSION, > + expireType: PrivateBrowsingUtils.isBrowserPrivate(this.browser) ? This will bitrot with bug 1206232. ::: browser/modules/webrtcUI.jsm:22 (Diff revision 16) > "resource://gre/modules/AppConstants.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "PluralForm", > "resource://gre/modules/PluralForm.jsm"); > > +XPCOMUtils.defineLazyGetter(this, "gBrandBundle", function() { > + return Services.strings.createBundle('chrome://branding/locale/brand.properties'); nit: every other string around here uses double quotes. I'm not sure it's useful to keep a reference to this bundle until the end of the process, given that we only use the brandShortName value from it. Maybe we could instead make the lazy getter return brandShortName? But maybe you did this so that it's consistent with gBrandBundle in PermissionUI.jsm (where it seems to never actually be used?). ::: browser/modules/webrtcUI.jsm:305 (Diff revision 16) > let chromeWin = chromeDoc.defaultView; > let stringBundle = chromeWin.gNavigatorBundle; > - let stringId = "getUserMedia.share" + requestTypes.join("And") + ".message"; > + let stringId = "getUserMedia.share" + requestTypes.join("And") + "2.message"; > let message = stringBundle.getFormattedString(stringId, [host]); > > - let mainLabel; > + let mainLabel = stringBundle.getString("getUserMedia.allowAccess.label"); You don't need this variable anymore. ::: browser/modules/webrtcUI.jsm:325 (Diff revision 16) > denyRequest(notification.browser, aRequest); > - // Let someone save "Never" for http sites so that they can be stopped from > + if (aState && aState.checkboxChecked) { > - // bothering you with doorhangers. > - let perms = Services.perms; > + let perms = Services.perms; > - if (audioDevices.length) > + if (audioDevices.length) > - perms.add(uri, "microphone", perms.DENY_ACTION); > + perms.add(uri, "microphone", perms.DENY_ACTION); This will also bitrot with bug 1206232. ::: browser/modules/webrtcUI.jsm:334 (Diff revision 16) > - mainAction.callback(aState, true); > - } > + } > - }); > + ]; > + > + let productName = gBrandBundle.GetStringFromName("brandShortName"); > + let disabledInfo = stringBundle.getFormattedString("getUserMedia.dontAllowHTTP", [productName]); There's a smell of code duplication here. Could you instead make this variable contain only the id of the string eg. "dontAllowHTTP" and at the end of the block do the getFormattedString call? Could we only get the string when it's actually going to be displayed? let id; if (sharingScreen) id = ... else if (sharingAudio) ... else if (!secure) { if (audio && !video) id = else if (!audio && video) id = else id = } Then disableMainAction becomes !!id ::: toolkit/content/widgets/notification.xml (Diff revision 16) > - <xul:menupopup anonid="menupopup" > + <xul:menupopup anonid="menupopup" > - position="after_end" > + position="after_end" > - xbl:inherits="oncommand=menucommand"> > + xbl:inherits="oncommand=menucommand"> > - <children/> > + <children/> > - <xul:menuitem class="menuitem-iconic popup-notification-closeitem" > - label="&closeNotificationItem.label;" Looks like the closeNotificationItem.label string should be removed from notification.dtd ::: toolkit/content/widgets/notification.xml:550 (Diff revision 16) > document.getAnonymousElementByAttribute(this, "anonid", "closebutton"); > </field> > <field name="button" readonly="true"> > document.getAnonymousElementByAttribute(this, "anonid", "button"); > </field> > + <field name="secondarybutton" readonly="true"> Should this be secondaryButton? The all-lowercase field name is confusing when reading the JS code, and I would be surprised if it didn't cause bugs at some point in the future. I guess you picked this for consistency with 'closebutton' above, but that closebutton field seems to have only 2 existing callers both in tests: http://searchfox.org/mozilla-central/search?q=.closebutton&case=true&regexp=false&path= ::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:8 (Diff revision 16) > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > rememberValue = Use Password Manager to remember this value. > rememberPassword = Use Password Manager to remember this password. > savePasswordTitle = Confirm > -# LOCALIZATION NOTE (rememberLoginMsg, rememberLoginMsgNoUser): %S is brandShortName > +# LOCALIZATION NOTE (rememberLoginMsg, rememberLoginMsgNoUser): %1$S is brandShortName, %2$S is the nit: wrap before %2$S ::: toolkit/modules/PopupNotifications.jsm (Diff revision 16) > popupnotification.setAttribute("buttonaccesskey", n.mainAction.accessKey); > popupnotification.setAttribute("buttoncommand", "PopupNotifications._onButtonEvent(event, 'buttoncommand');"); > popupnotification.setAttribute("buttonpopupshown", "PopupNotifications._onButtonEvent(event, 'buttonpopupshown');"); > popupnotification.setAttribute("learnmoreclick", "PopupNotifications._onButtonEvent(event, 'learnmoreclick');"); > popupnotification.setAttribute("menucommand", "PopupNotifications._onMenuCommand(event);"); > - popupnotification.setAttribute("closeitemcommand", `PopupNotifications._dismiss(${TELEMETRY_STAT_DISMISSAL_NOT_NOW});event.stopPropagation();`); const TELEMETRY_STAT_DISMISSAL_NOT_NOW = 8; at the top of the file can also be removed. ::: toolkit/modules/PopupNotifications.jsm:716 (Diff revision 16) > popupnotification.removeAttribute("origin"); > } > } else > popupnotification.removeAttribute("origin"); > > + if (n.options.hideClose) Would it make sense to make this a test for if (!n.options.showClose) instead? It seems we are specifying hideClose: true everywhere with this patch. ::: toolkit/themes/shared/popupnotification.inc.css:114 (Diff revision 16) > } > > .popup-notification-dropmarker > .button-box > .button-menu-dropmarker > .dropmarker-icon { > width: 16px; > height: 16px; > - list-style-image: url(chrome://global/skin/icons/menubutton-dropmarker-white.svg); > + /* XXX: move the following svg files to toolkit? */ Yes. Probably OK to do it in a follow-up, as I don't think PopupNotifications has any non-browser/ consummer.

Comment on attachment 8782171 [details] Bug 1282768 - Convert permission notifications to use two buttons instead of the drop-down menu. https://reviewboard.mozilla.org/r/72386/#review83164 They are less verbose and more direct, and came straight from UX as you noted. > .path is potentially a huge string for data urls. Is this going to be OK in the UI, or do we need to find a way to truncate excessively long strings? This is for file urls, it doesn't seem to be possible to use IndexedDB from data urls. This throws a security error: data:html,<script>window.indexedDB.open("foo",{storage:"persistent"});</script> I can imagine file paths where this might be a problem, but we already have that in other prompts and it hasn't been a problem so far. > This string is surprising. We are changing "would you like to receive notifications" to "allow to send notifications". Send to whom? The new string is actually "Will you allow [site name] to send notifications?", which uses the same verb as the action and I assume is the main reason for the change. > I don't like this "Don't Allow" string. Negations are cognitively hard to process. Could this string say "Deny" instead? > > But I see the previous "Don't Share" string had the same issue... This pattern is pervasive across all of the prompts, making things consistent. "will you allow foo to blah? Allow/Don't Allow" > Would it make sense to make this a test for > if (!n.options.showClose) instead? > It seems we are specifying hideClose: true everywhere with this patch. It's definitely not everywhere that I added hideClose, only the notifications that are part of the MVP. For instance plugins still have the X. Also, showClose is backwards incompatible and may affect add-ons. > Yes. Probably OK to do it in a follow-up, as I don't think PopupNotifications has any non-browser/ consummer. OK, makes sense.

I'll address the rest of your comments tomorrow.

Comment on attachment 8782171 [details] Bug 1282768 - Convert permission notifications to use two buttons instead of the drop-down menu. https://reviewboard.mozilla.org/r/72386/#review83164 > Do we really want the "Remember this decision" string to be translated for each prompt? Is there a way to make this the default value that PopupNotifications.jsm would fetch if nothing has been specified? We could, but that would be going against the previous decision to make the checkbox generic in the API (bug 1291642 comment 9). PopupNotifications_show doesn't make any assumptions about the role of the checkbox and the login prompt for instance could use it for a completely different reason (show password). I agree that it would be nice to cut down on the number of strings we ship, but I think the benefits of a generalized API outweigh this shortcoming. > Why are we adding "or application window" here? I don't find the "application window" wording very clear, and the "or" makes me (as a potential user) wonder if it's the site that decides if it's the screen or an application window that's going to be shared. UX would know all of the details, but it seems to me that we want to make sure the string applies to both cases, entire screen and part of it: https://mozilla.invisionapp.com/share/AF71R266U#/screens/142999157 Also, soon the preview you are working on should remove any ambiguity about what is going to be shared :) > I find these Allow {Screen,Window,Application} Access strings confusing. > > What does accessing the screen means? Does it mean display something on it? > Accessing window could mean listing existing windows and sending messages to them. > Accessing application could be communicating data with the application, or taking control of the application. > > What's the rationale for changing these strings? I thought the change from "Share Foo" to "Allow Foo" was pretty straightforward: the question now is "Will you allow site.com to see your screen or application window?" and "Allow Access" seems like an intuitive answer. That is, the Allow string on its own might be confusing, but in the context of the question it seems pretty intuitive to me. > This l10n note missed the last 2 strings (dontAllowScreenAlways and dontAllowAudioAlways). > > The choice of the "dontAllowHTTP" surprises me here, it sounds like a user action. I would have picked something like "noPersistentDeviceAccessOverInsecureConnection". But maybe it's just me :-). If you can come up with something shorter than "noPersistentDeviceAccessOverInsecureConnection", I'm all ears :) > Should we record something for the non-persistent "block" case? > See also http://searchfox.org/mozilla-central/rev/e8dd21b1673c07307068303ab3e99360c78f4d12/security/manager/ssl/nsISecurityUITelemetry.idl#96 I'd like to keep this patch focused on the prompt conversion and avoid dealing with such new functionality here if possible. > This will bitrot with bug 1206232. Thanks for the heads up, I'll rebase before landing. > nit: every other string around here uses double quotes. > > I'm not sure it's useful to keep a reference to this bundle until the end of the process, given that we only use the brandShortName value from it. Maybe we could instead make the lazy getter return brandShortName? But maybe you did this so that it's consistent with gBrandBundle in PermissionUI.jsm (where it seems to never actually be used?). That's right, it's a pretty common pattern and should reduce cognitive overhead. I'm happy to change it if you feel strongly about it. > This will also bitrot with bug 1206232. Thanks for the heads up, I'll keep it in mind when it's time to rebase.

I've made all the other changes you requested, thanks!

The previous try build failed due to --artifact, so new try run with build for testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=254599e2a814

Comment on attachment 8782171 [details] Bug 1282768 - Convert permission notifications to use two buttons instead of the drop-down menu. I've looked at the PopupNotifications.jsm styling changes and I've tested various prompts. Things look good to me, except for the add-ons case for which bug 1201896 can provide more definition, or a better fix. In general this is fine for landing on elm if we want to build upon it.

Updated with two small fixes for test failures on try. Here is the latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=634c8494c79d25f22332b1ddf7785b43fc858bec&selectedJob=30031355 The few remaining failures are either known intermittents or also present in elm and I haven't been able to reproduce locally.

Comment on attachment 8806659 [details] Bug 1282768 - Part 2 - Move the secondary actions of doorhanger notifications to their own menu button. I've fixed a few nits and I'm working on removing some unnecessary test code for secondary actions, but for the rest this part looks good.

Comment on attachment 8806658 [details] Bug 1282768 - Part 1 - Move filters.svg and menubutton-dropmarker.svg to Toolkit. https://reviewboard.mozilla.org/r/90034/#review89602 LGTM

Comment on attachment 8806659 [details] Bug 1282768 - Part 2 - Move the secondary actions of doorhanger notifications to their own menu button. https://reviewboard.mozilla.org/r/90036/#review89656

I've added missing tests for extra secondary actions, and I've cleaned up the test code when we don't show actions in the submenu anymore.

Comment on attachment 8807545 [details] Bug 1282768 - Part 4 - Fix intermittent browser_social_activation.js. https://reviewboard.mozilla.org/r/90700/#review90380 ::: browser/base/content/test/social/browser_social_activation.js:135 (Diff revision 1) > + "popuphidden"); > + > + // We need to wait for PopupNotifications.jsm to place the element. > + let notification; > + yield BrowserTestUtils.waitForCondition( > + () => (notification = PopupNotifications.panel.childNodes[0])); Looks good, but I would feel better if we could assert that notification.id == "servicesInstall-notification".

I think that the remaining test_object_plugin_nav.html failure is due to the login prompt staying open during a previous test: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/27bef556300a907fa697603c86d6f8862cb8ad9f7bd18676e9bd54d49717bbd52f9c8a195b6114e7d871f0b66eb0ccee64fec8020c55527ee4e4481850bd724c

Comment on attachment 8806660 [details] Bug 1282768 - Part 3 - Convert permission notifications to use two buttons and a checkbox instead of multiple secondary actions. https://reviewboard.mozilla.org/r/90038/#review90768 Sorry for the long delay here :-(. The code looks good. I haven't tested the patch locally, do you want me to do it? Some of my comments about the strings in comment 44 still apply, especially: > > +webNotifications.receiveFromSite2=Will you allow %S to send notifications? > This string is surprising. We are changing "would you like to receive notifications" to "allow to send notifications". Send to whom? and > > +getUserMedia.shareScreen2.message = Will you allow %S to see your screen or application window? > Why are we adding "or application window" here? I don't find the "application window" wording very clear, and the "or" makes me (as a potential user) wonder if it's the site that decides if it's the screen or an application window that's going to be shared. ::: browser/base/content/test/webrtc/browser_devices_get_user_media.js:502 (Diff revision 4) > - for (let node of notification.childNodes) { > - if (node.localName == "menuitem") > - labels.push(node.getAttribute("label")); > - } > - is(labels.indexOf(alwaysLabel), -1, "The 'Always Allow' item isn't shown"); > + let checkbox = notification.checkbox; > + ok(!!checkbox, "checkbox is present"); > + ok(!checkbox.checked, "checkbox is not checked"); > + checkbox.click(); > + ok(checkbox.checked, "checkbox now checked"); > + ok(notification.button.disabled, "Allow button is disabled"); Can we also check here that the additional warning message is displayed? ::: browser/locales/en-US/chrome/browser/browser.properties:483 (Diff revision 4) > -getUserMedia.shareScreen.message = Would you like to share your screen with %S? > -getUserMedia.shareCameraAndMicrophone.message = Would you like to share your camera and microphone with %S? > -getUserMedia.shareCameraAndAudioCapture.message = Would you like to share your camera and this tab’s audio with %S? > -getUserMedia.shareScreenAndMicrophone.message = Would you like to share your microphone and screen with %S? > -getUserMedia.shareScreenAndAudioCapture.message = Would you like to share this tab’s audio and your screen with %S? > -getUserMedia.shareAudioCapture.message = Would you like to share this tab’s audio with %S? > +getUserMedia.shareScreen2.message = Will you allow %S to see your screen or application window? > +getUserMedia.shareCameraAndMicrophone2.message = Will you allow %S to use your camera and microphone? > +getUserMedia.shareCameraAndAudioCapture2.message = Will you allow %S to use your camera and listen to this tab’s audio? > +getUserMedia.shareScreenAndMicrophone2.message = Will you allow %S to use your microphone and see your screen or application window? > +getUserMedia.shareScreenAndAudioCapture2.message = Will you allow %S to listen to this tab’s audio and see your screen or application window? > +getUserMedia.shareAudioCapture2.message = Will you allow %S to use this tab’s audio? The audio capture strings are inconsistent, shareCameraAndAudioCapture2 and shareScreenAndAudioCapture2 say "listen to this tab’s audio", but shareAudioCapture2 says "use this tab’s audio". I prefer the "listen to" version which is more explicit. ::: browser/modules/webrtcUI.jsm:333 (Diff revision 4) > - mainAction.callback(aState, true); > - } > + } > - }); > + ]; > + > + let productName = gBrandBundle.GetStringFromName("brandShortName"); > + let id = "getUserMedia.dontAllowHTTP"; nit: This 'id' variable name seems too generic, maybe use 'warningId'? (but I know it was 'id' in the pseudo code in my previous review comment, sorry about that) ::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js (Diff revision 4) > promptMsg, > "password-notification-icon", > mainAction, > secondaryActions, > { > - displayURI: Services.io.newURI(login.hostname, null, null), Why is this no longer needed?

Comment on attachment 8806660 [details] Bug 1282768 - Part 3 - Convert permission notifications to use two buttons and a checkbox instead of multiple secondary actions. https://reviewboard.mozilla.org/r/90038/#review90788 ::: browser/locales/en-US/chrome/browser/browser.properties:518 (Diff revision 4) > -getUserMedia.never.label = Never Share > -getUserMedia.never.accesskey = N > +getUserMedia.dontAllowHTTP=Your connection to this site is not secure. %S can not allow permanent access to your devices. > +getUserMedia.dontAllowHTTPAudio=Your connection to this site is not secure. %S can not allow permanent access to your microphone. > +getUserMedia.dontAllowHTTPVideo=Your connection to this site is not secure. %S can not allow permanent access to your camera. Can we just use one string saying "devices" for this warning label? I tested it, and the type of device is already really clear from the context, so we can save the localizers some work.

(In reply to :Paolo Amadini from comment #79) > ::: browser/locales/en-US/chrome/browser/browser.properties:518 > (Diff revision 4) > > -getUserMedia.never.label = Never Share > > -getUserMedia.never.accesskey = N > > +getUserMedia.dontAllowHTTP=Your connection to this site is not secure. %S can not allow permanent access to your devices. > > +getUserMedia.dontAllowHTTPAudio=Your connection to this site is not secure. %S can not allow permanent access to your microphone. > > +getUserMedia.dontAllowHTTPVideo=Your connection to this site is not secure. %S can not allow permanent access to your camera. > > Can we just use one string saying "devices" for this warning label? I tested > it, and the type of device is already really clear from the context, so we > can save the localizers some work. Seems OK to me.

(In reply to Florian Quèze [:florian] [:flo] from comment #78) > Some of my comments about the strings in comment 44 still apply, especially: Did you see my responses in comment 46 and comment 47? I will copy my previous comments to the two specific issues you raise below. > > > +webNotifications.receiveFromSite2=Will you allow %S to send notifications? > > This string is surprising. We are changing "would you like to receive notifications" to "allow to send notifications". Send to whom? The new string is actually "Will you allow [site name] to send notifications?", which uses the same verb as the action and I assume is the main reason for the change. > > and > > > > +getUserMedia.shareScreen2.message = Will you allow %S to see your screen or application window? > > Why are we adding "or application window" here? I don't find the "application window" wording very clear, and the "or" makes me (as a potential user) wonder if it's the site that decides if it's the screen or an application window that's going to be shared. UX would know all of the details, but it seems to me that we want to make sure the string applies to both cases, entire screen and part of it: https://mozilla.invisionapp.com/share/AF71R266U#/screens/142999157 Also, soon the preview you are working on should remove any ambiguity about what is going to be shared :) > ::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js > (Diff revision 4) > > promptMsg, > > "password-notification-icon", > > mainAction, > > secondaryActions, > > { > > - displayURI: Services.io.newURI(login.hostname, null, null), > > Why is this no longer needed? The notifications no longer display a title where displayURI used to go.

(In reply to :Paolo Amadini from comment #79) > ::: browser/locales/en-US/chrome/browser/browser.properties:518 > (Diff revision 4) > > -getUserMedia.never.label = Never Share > > -getUserMedia.never.accesskey = N > > +getUserMedia.dontAllowHTTP=Your connection to this site is not secure. %S can not allow permanent access to your devices. > > +getUserMedia.dontAllowHTTPAudio=Your connection to this site is not secure. %S can not allow permanent access to your microphone. > > +getUserMedia.dontAllowHTTPVideo=Your connection to this site is not secure. %S can not allow permanent access to your camera. > > Can we just use one string saying "devices" for this warning label? I tested > it, and the type of device is already really clear from the context, so we > can save the localizers some work. Michelle Heubusch pointed out that "devices" is often too generic for the end user and in this particular case the user would be unlikely to think of microphone and camera as devices (presumably because they are inside the computer). I don't think that comment was about this specific string, but it seems to me to apply equally well here.

(In reply to Panos Astithas [:past] from comment #82) > Michelle Heubusch pointed out that "devices" is often too generic for the > end user and in this particular case the user would be unlikely to think of > microphone and camera as devices (presumably because they are inside the > computer). I don't think that comment was about this specific string, but it > seems to me to apply equally well here. That comment only tells me that we should spell out "camera and microphone" in all occasions rather than saying "devices", but it's not exactly my point here. My actual point is that if this additional warning label said "Your connection to this site is not secure. %S can not allow permanent access." it would be equally crystal clear in the context where it appears, especially because it's only shown after the user interacted with the checkbox on the notification, and at the same time the main button is disabled. The final words are just an embellishment, that's why I suggested using a generic term. We may even remove the final words completely. We should find a balance between perfect wording and multiplication of strings and work for localizers.

(In reply to Panos Astithas [:past] from comment #81) > (In reply to Florian Quèze [:florian] [:flo] from comment #78) > > Some of my comments about the strings in comment 44 still apply, especially: > > Did you see my responses in comment 46 and comment 47? I will copy my > previous comments to the two specific issues you raise below. Yes > > > > +webNotifications.receiveFromSite2=Will you allow %S to send notifications? > > > This string is surprising. We are changing "would you like to receive notifications" to "allow to send notifications". Send to whom? > > The new string is actually "Will you allow [site name] to send > notifications?", which uses the same verb as the action and I assume is the > main reason for the change. Still, "send notifications" is very ambiguous about who will see these notifications. Worded this way, it could totally be email notifications to other people. Maybe it should be "send you notifications", or "show notifications". > > > > +getUserMedia.shareScreen2.message = Will you allow %S to see your screen or application window? > > > Why are we adding "or application window" here? I don't find the "application window" wording very clear, and the "or" makes me (as a potential user) wonder if it's the site that decides if it's the screen or an application window that's going to be shared. > > UX would know all of the details, but it seems to me that we want to make > sure the string applies to both cases, entire screen and part of it: > https://mozilla.invisionapp.com/share/AF71R266U#/screens/142999157 > Also, soon the preview you are working on should remove any ambiguity about > what is going to be shared :) This mockup isn't specific enough, it shows "Screen/window" before the menulist, which is something we never do. But the button says "Share Window". It looks like this is one mockup to avoid putting in the slides 2 almost identical mockups, but there are still a few distinct cases. More specifically, there's: 1. sharing a whole screen (screen sharing) 2. sharing all the windows of an application (app sharing) 3. sharing one specific window. (window sharing) 'application window' is ambiguous, vaguely relates to 2 or 3, without matching either case exactly. In other places we used "screen" generically to cover all the cases, because you are sharing your screen or a subset of your screen. So either we should use a generic string that's not confusing, or we should use a specific string for each case.

(In reply to :Paolo Amadini from comment #83) > We should find > a balance between perfect wording and multiplication of strings and work for > localizers. If something has a particularly high cost with questionable gain, maybe. As a general rule I don't think I can agree with the above. If we feel we need to avoid the clearest, least confusing wording to minimize work for localizers, we should treat that as a bug. If localizers can't keep up or need to spend a lot of time on keeping up, we need to help them by improving documentation, improving our l10n technology stack and tools (pretty sure people are working on this), reaching out to the community to get more people involved, etc.

(In reply to Dão Gottwald [:dao] from comment #85) > If something has a particularly high cost with questionable gain, maybe. As > a general rule I don't think I can agree with the above. There is also a maintenance cost beside localization if we make our interfaces excessively specific for each case. Just as an example, if in the future we need to add a new variable element in the same label, we'll have to spend more effort in deciding whether we can get rid of the former specificity or we need to create combinations. This is the kind of decision we have to make every day when coding, and I'm definitely not saying we should have a general rule. I'm saying that it's worth considering if we really need the complexity for each specific case. The case I mentioned is one of them, and if I read correctly the case Florian mentioned in comment 84 is another.

Michelle could you chime in with some context around the decisions for the permission notification strings and consider the alternative suggestions? The relevant discussion is in comment 46, comment 48 and from comment 78 onward.

Cumulative tryserver build including bug 1201896 and bug 1316311: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4911266256e5c6018eef794ee2d7067a6fe61b7

Review ping for Michelle. Refining the strings is the last part we're waiting upon before we merge the Permissions Notification project to Nightly. If we don't get feedback on the strings, I actually suggest to land this tomorrow anyways, and update the strings later. We are at the beginning of the cycle, so it shouldn't be an issue if we have temporary strings for a few days.

THanks for the ping. I did update the copy spec document here: https://docs.google.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/edit# In short, the recommendation is: Simplify the buttons to [Don't Allow] [Allow] Where we have a option that requires a user to select a dropdown, use the copy "Will you allow [domain name] to access your:" and then use a simple label over the dropdown to complete the phrase so it will look like this: Will you allow [domain name] to access your: Microphone [dropdown menu of options] X Remember this decision [Don’t Allow] [Allow] The revised non-secure site message is: our connection to this site is not secure. To protect you, Firefox will only allow access for this session.

Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd13c8aaf95e Part 1 - Move filters.svg and menubutton-dropmarker.svg to Toolkit. r=past https://hg.mozilla.org/integration/mozilla-inbound/rev/3304424abe70 Part 2 - Move the secondary actions of doorhanger notifications to their own menu button. r=paolo https://hg.mozilla.org/integration/mozilla-inbound/rev/d229169c60ea Part 3 - Convert permission notifications to use two buttons and a checkbox instead of multiple secondary actions. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/53fc06a96f27 Part 4 - Fix intermittent browser_social_activation.js. r=past

https://hg.mozilla.org/mozilla-central/rev/bd13c8aaf95e https://hg.mozilla.org/mozilla-central/rev/3304424abe70 https://hg.mozilla.org/mozilla-central/rev/d229169c60ea https://hg.mozilla.org/mozilla-central/rev/53fc06a96f27

https://hg.mozilla.org/mozilla-central/rev/d229169c60ea#l7.157 > getUserMedia.reasonForNoPermanentAllow.screen=%S can not allow permanent access to your screen or application without asking which one to share. I assume this one is missing a "windows" (or application window).

(In reply to mheubusch from comment #90) > Where we have a option that requires a user to select a dropdown, use the > copy "Will you allow [domain name] to access your:" and then use a simple > label over the dropdown to complete the phrase so it will look like this: > Will you allow [domain name] to access your: > Microphone > [dropdown menu of options] Looks like this comment wasn't addressed.

I filed bug 1319112 for the string issues.

This seems to have resulted in a nice increase [1] in people clicking buttons on the remember password dialog (at least in Nightly). Yes, the number of people clicking on "Not now" went from basically-0 to very-much-not-0... but the number of people clicking on "Remember this" went up, too. (as did the much smaller number of people clicking "Never"). Not sure if this was the intent but, uh, good job? [1]: https://mzl.la/2gFpYqI

Verified as fixed on Firefox Nightly 54.0a1 and on Firefox Aurora 53.0a2 on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64. I verified different permissions prompts like (Audio, Video, Screen, Window and Add-ons). Here you can find all the test results: https://testrail.stage.mozaws.net/index.php?/plans/view/1948

PopupNotification module has changed and the respective documentation should be updated to reflect that Adding dev-doc-needed flag to request update in dev documentation and test site: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PopupNotifications.jsm https://developer.mozilla.org/en-US/docs/Mozilla/Using_popup_notifications https://permission.site/ Thanks!

Sorry, I meant Adding dev-doc-needed flag to request update in dev documentation: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PopupNotifications.jsm https://developer.mozilla.org/en-US/docs/Mozilla/Using_popup_notifications