This state should be set as reaction to the user denying a permission request without checking "Always remember" and should be persisted through the tab lifetime or until another site is opened.

We discussed this and the blocked/allowed temporarily states will be set depending on the "Always remember my decision" checkbox, which doesn't have its own ticket yet. So I'm renaming this ticket to take care of adding that checkbox and figuring out the implementation of the different states that derive from it being ticked or not. See https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96290116

This patch moves us away from providing explicit actions for persistent permissions towards a unified "Always remember my decision" checkbox, as specified by UX. Panos and Paolo, would you like to take a look at this and poke holes? I'll take care of the surely failing tests once I know we're going in the right direction. From manual testing everything is working as it should. Note that we'll need to do a lot of followups such as Bug 1282768 which adds the left button, remove the "Not now" option and more cleanup work. As the whole panel is very WIP, this should obviously land in Elm.

Just in case you were wondering: This is not taking care of properly handling "temporary" permissions. I think adding the checkbox is enough complexity for a single patch. "Temporary" permissions are still always re-requested here.

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. https://reviewboard.mozilla.org/r/71970/#review69856 r- because I can't find the f+ that I want to use in MozReview. The direction looks OK to me. ::: browser/components/nsBrowserGlue.js:2497 (Diff revision 1) > * Permission is granted if action is null or ALLOW_ACTION. > * @param aNotificationId The id of the PopupNotification. > * @param aAnchorId The id for the PopupNotification anchor. > * @param aOptions Options for the PopupNotification > */ > - _showPrompt: function CPP_showPrompt(aRequest, aMessage, aPermission, aActions, > + _showPrompt: function CPP_showPrompt(aRequest, aMessage, aPermission, aAllowAction, aDenyAction, I think main/secondary work better than allow/deny, as the defaults may be sometimes the opposite (plugins are I think one such case). Also, don't forget to update the jsdoc above. ::: browser/components/nsBrowserGlue.js:2514 (Diff revision 1) > - } > + } > > - // Remember permissions. > + // Remember permissions. > - if (promptAction.action) { > + if (remember) { > - Services.perms.addFromPrincipal(requestPrincipal, aPermission, > + Services.perms.addFromPrincipal(requestPrincipal, aPermission, > - promptAction.action, promptAction.expireType); > + Ci.nsIPermissionManager.ALLOW_ACTION, aAllowAction.expireType); Oh, I see now why you chose the allow/deny names. Maybe it's OK for the call site to bear the responsibility to set them properly. ::: browser/components/nsBrowserGlue.js (Diff revision 1) > - // Only show "allow for session" in PB mode, we don't > - // support "allow for session" in non-PB mode. I believe we still need to maintain this functionality. I'm not sure if it's OK to change the button label or not ("Receive for this session" -> "Receive Notifications"), but the expireType should remain the same in PB. ::: browser/locales/en-US/chrome/browser/browser.properties:377 (Diff revision 1) > # LOCALIZATION NOTE (geolocation.shareLocation geolocation.alwaysShareLocation geolocation.neverShareLocation): > # 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.dontShareLocation=Don’t Share Location > +geolocation.dontShareLocation.accesskey=N The accesskey here should be lowercase 'n', as 'N' isn't present in the string. ::: browser/modules/webrtcUI.jsm:322 (Diff revision 1) > - secondaryActions.push({ > - label: stringBundle.getString("getUserMedia.never.label"), > - accessKey: stringBundle.getString("getUserMedia.never.accesskey"), > - callback: function () { > denyRequest(notification.browser, aRequest); > + if (aRemember) { Isn't it necessary to guard this with (!sharingScreen && !sharingAudio)?

If I remember correctly the discussions with UX, the checkbox should be there for webrtc for the http case and for the screensharing cases, but the "allow" button should be disabled when the checkbox is checked, and there was a message to display to explain why (something like "can't remember when the connection isn't secure"), I don't remember the exact wording. The reason for this was that we wanted to preserve a way for the user to say "don't bother me ever again". Also, "Always remember my decision" isn't the final wording, we had a discussion a couple weeks ago about "always" and "remember" used together being confusing. The final string was "Remember this decision".

Thanks for the feedback Panos, I'll update the patch and try to run some tests. :) (In reply to Florian Quèze [:florian] [:flo] (PTO until August 29th) from comment #6) > If I remember correctly the discussions with UX, the checkbox should be > there for webrtc for the http case and for the screensharing cases, but the > "allow" button should be disabled when the checkbox is checked, and there > was a message to display to explain why (something like "can't remember when > the connection isn't secure"), I don't remember the exact wording. The > reason for this was that we wanted to preserve a way for the user to say > "don't bother me ever again". Yes I remember this as well, I think saw the designs for that recently, but I wanted to leave it as a follow-up bug. > > Also, "Always remember my decision" isn't the final wording, we had a > discussion a couple weeks ago about "always" and "remember" used together > being confusing. The final string was "Remember this decision". Ah, good to know, thanks. I'll change it.

(In reply to Panos Astithas [:past] from comment #5) > Comment on attachment 8781561 [details] > Bug 1291642 - Add an "Always remember my decision" checkbox to permission > doorhangers. > > Oh, I see now why you chose the allow/deny names. Maybe it's OK for the call > site to bear the responsibility to set them properly. Yeah I think it's appropriate in nsBrowserGlue since that function is an abstraction over allow/deny only. In other places I'm not using that terminology. > > ::: browser/components/nsBrowserGlue.js > (Diff revision 1) > > - // Only show "allow for session" in PB mode, we don't > > - // support "allow for session" in non-PB mode. > > I believe we still need to maintain this functionality. I'm not sure if it's > OK to change the button label or not ("Receive for this session" -> "Receive > Notifications"), but the expireType should remain the same in PB. > Wouldn't it be more according to spec if we made the "remember" checkbox trigger session expiry in PB mode for notifications? This way we'd avoid adding a third button and staying consistent with using the checkbox. We can also check that with UX to be sure. According to the design docs the correct text is "Allow notifications". > > ::: browser/modules/webrtcUI.jsm:322 > (Diff revision 1) > > - secondaryActions.push({ > > - label: stringBundle.getString("getUserMedia.never.label"), > > - accessKey: stringBundle.getString("getUserMedia.never.accesskey"), > > - callback: function () { > > denyRequest(notification.browser, aRequest); > > + if (aRemember) { > > Isn't it necessary to guard this with (!sharingScreen && !sharingAudio)? Technically the checkbox should not be shown under that condition as we guard against this in the options object already. I can insert the condition if you like but it shouldn't be necessary.

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. For now I'm only commenting about the PopupNotifications.jsm changes. Maybe the text of the checkbox should be provided by the caller, like we do for the text of the buttons. This seems in line with the current design of the module as a request dialog not necessarily tied to permissions, and if we do this then in bug 1270416 we might use the checkbox as a confirmation for registering a protocol handler, instead of implementing custom UI.

Yes, you're right. I'm actually extending the capabilities of the alwaysRemember options right now since there are other things in the spec that we need to be able to do, e.g. pre-checking the checkbox by default or showing an error message on HTTP. Thanks for the feedback! :)

Paolo, I know you're already busy on reviews, sorry for adding another one :) I realize some of this stuff looks a bit hacky until we merge this bug with Bug 1282768 and have more opportunities for cleaning up. Note that no tests were changed so far, I'll fix the tests and add some more in a separate commit. I expect a ton of failures, so I'll just watch the try push before I attempt to find them locally. I'm also a bit unsure about the colors in popupnotification.inc.css, is there a way to use color variables here? Thanks!

(In reply to Johann Hofmann [:johannh] from comment #13) > Note that no tests were changed so far, I'll fix the tests and add some more > in a separate commit. I expect a ton of failures, so I'll just watch the try > push before I attempt to find them locally. I'll be able to review this in the next few days, but after taking a quick look I think you can split this in two changesets, maybe two bugs. The first would be the PopupNotifications.jsm API addition, with its tests. Ideally this doesn't affect any other permission test because the checkbox wouldn't be shown if the call sites aren't changed. We can land this in mozilla-central when ready. The second would be the actual call site change, with updates to the permission tests where they exist. For the PopupNotifications.jsm change, I'm undecided if the logic for enabling the action button and showing the red label should be in the call site instead, with the API giving access to those controls and their events. We'll likely need slightly different logic for the registerProtocolHandler case, for example. > I'm also a bit unsure about the colors in popupnotification.inc.css, is > there a way to use color variables here? Doesn't the opacity work for the buttons? For the red label, we might need to use variables so that we can redefine the value for high contrast themes.

(In reply to :Paolo Amadini from comment #14) > (In reply to Johann Hofmann [:johannh] from comment #13) > > Note that no tests were changed so far, I'll fix the tests and add some more > > in a separate commit. I expect a ton of failures, so I'll just watch the try > > push before I attempt to find them locally. > > I'll be able to review this in the next few days, but after taking a quick > look I think you can split this in two changesets, maybe two bugs. The first > would be the PopupNotifications.jsm API addition, with its tests. Ideally > this doesn't affect any other permission test because the checkbox wouldn't > be shown if the call sites aren't changed. We can land this in > mozilla-central when ready. > > The second would be the actual call site change, with updates to the > permission tests where they exist. > Splitting in two patches sounds like a good idea! > For the PopupNotifications.jsm change, I'm undecided if the logic for > enabling the action button and showing the red label should be in the call > site instead, with the API giving access to those controls and their events. > We'll likely need slightly different logic for the registerProtocolHandler > case, for example. The only solution for this I could come up with would be to expose callbacks and access to xul elements to callers. In the most common case ("remember my decision") that would mean a lot of additional state juggling for the ordinary callers. This just sounds like potential to get really messy. What kind of logic do you need here? > > > I'm also a bit unsure about the colors in popupnotification.inc.css, is > > there a way to use color variables here? > > Doesn't the opacity work for the buttons? The designs specify a "grey out" behavior and I tried to get close to that. https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/106406109

(In reply to Johann Hofmann [:johannh] from comment #15) > What kind of logic do you need here? Possibly, disable the main action when the checkbox is disabled. I agree we may just implement that as another option in the module if we need it later. > > Doesn't the opacity work for the buttons? > > The designs specify a "grey out" behavior and I tried to get close to that. > https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/106406109 We probably need to use platform colors in some way, maybe "graytext" and a transparent background that changes the "-moz-field" color slightly. See bug 1296861 that fixed the colors for the remove buttons in the Control Center.

Paolo, sorry for flooding. We can load balance these review requests as soon as folks are back from their vacation. Most of the patches aren't that big, and part 4 is not guaranteed to actually fix all tests, I'll do another try now.

Turns out I broke more tests than fix, fun times.

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. https://reviewboard.mozilla.org/r/71970/#review72008 Don't worry for the many reviews, while I couldn't look at everything in detail I already have a few suggestions. ::: toolkit/content/widgets/notification.xml:496 (Diff revision 3) > + <xul:checkbox > + anonid="checkbox" > + label="&rememberDecision;" > + xbl:inherits="hidden=hidecheckbox,checked=checkboxchecked,label=checkboxlabel,oncommand=oncheckboxcommand" > + checked="false"/> This would match the existing conventions better: checkboxhidden, checkboxchecked, checkboxlabel, checkboxcommand Remove the checked attribute from the element. Indent like other nearby code. ::: toolkit/content/widgets/notification.xml:501 (Diff revision 3) > + <xul:checkbox > + anonid="checkbox" > + label="&rememberDecision;" > + xbl:inherits="hidden=hidecheckbox,checked=checkboxchecked,label=checkboxlabel,oncommand=oncheckboxcommand" > + checked="false"/> > + <xul:description class="disabled-info" xbl:inherits="hidden=hidedisabledinfo,xbl:text=disabledinfo"/> <xul:description class="popup-notification-warning" xbl:inherits="hidden=warninghidden,xbl:text=warninglabel"> ::: toolkit/locales/en-US/chrome/global/notification.dtd:13 (Diff revision 3) > > <!ENTITY checkForUpdates "Check for updates…"> > > <!ENTITY learnMore "Learn more…"> > + > +<!ENTITY rememberDecision "Remember this decision."> I think we don't use the full stop for checkboxes. ::: toolkit/modules/PopupNotifications.jsm:342 (Diff revision 3) > + * show: Whether or not to the checkbox. Defaults to false. > + * label: > + * (optional) Text to override the default label. > + * Defaults to "Remember this decision". Not sure if "show" is needed, at least it should default to true if the object is present? I see from the test you've added it for the case where we don't have other arguments, but maybe we could support "checkbox: true" instead. Actually, we could sidestep the above and make the "label" mandatory. We don't have default permission-related labels for buttons, so why for the checkbox? A helper in the permissions code could provide the common value, and it would be more logical to find it there. ::: toolkit/modules/PopupNotifications.jsm:349 (Diff revision 3) > + * disableMainAction: > + * (optional) Whether the mainAction is disabled with > + * the checkbox checked. Defaults to false. > + * disableInfo: > + * (optional) Text to be shown if the disableMainAction > + * option is set and the checkbox is enabled. I think this would be clearer: whenChecked: { disableMainAction: true, warningLabel: "..." }, whenUnchecked: { ... }, We don't need to implement whenUnchecked now but I guess it would simplify your implementation. ::: toolkit/modules/PopupNotifications.jsm:762 (Diff revision 3) > + // Need to find the associated notification object, which is a bit tricky > + // since it isn't associated with the button directly - this is kind of > + // gross and very dependent on the structure of the popupnotification > + // binding's content. > + let target = event.originalTarget; > + let notificationEl; > + let parent = target; > + while (parent && (parent = target.ownerDocument.getBindingParent(parent))) > + notificationEl = parent; Helper needed. ::: toolkit/themes/shared/popupnotification.inc.css (Diff revision 3) > -.popup-notification-button-wrapper { > - background-color: #0996f8; > -} This must be on the wrapper to style the separator properly, especially on Windows and Mac where it can be transparent. I was thinking, however, that we should base the patch and tests on central and land it sooner? This would not require styling changes as we use platform buttons there. ::: toolkit/themes/shared/popupnotification.inc.css:61 (Diff revision 3) > /* This is to override the linux !important */ > -moz-appearance: none !important; > min-width: 0; > } > > +.popup-notification-button[disabled] ~ toolbarseparator, Maybe we should lock the 7px margin instead? ::: toolkit/themes/shared/popupnotification.inc.css:121 (Diff revision 3) > + pointer-events: none; > +} > + > +.disabled-info { > + margin-top: 4px; > + color: #aa1b08; I'm not sure how to handle the highlight color properly. This hardcoded color is fine for default themes, but in other cases we might want to use a platform color for highlighted text or "-moz-fieldText". I'd say to leave the coloring out of the mozilla-central patch and discuss this separately. I don't know if what we did for the insecure connection text is being updated for high contrast support.

Comment on attachment 8784405 [details] Bug 1291642 - Part 2 - Add tests for the checkbox option in PopupNotifications. https://reviewboard.mozilla.org/r/73878/#review72024 Tests need updating but look good for the rest.

> I'm not sure how to handle the highlight color properly. I just discussed this with Dao and he suggested to hardcode on Mac, do a -moz-windows-default-theme media query on Windows and maybe just leave the red away on Linux. If that sounds ok to you I'll go ahead with it.

Comment on attachment 8784797 [details] Bug 1291642 - Part 3 - Update permission notifications to use checkbox in PopupNotifications. https://reviewboard.mozilla.org/r/74112/#review72026 This update of the calling sites overlaps with the bug Panos is working on, maybe we should only update the calling sites once? In fact, this intermediate state with the checkbox but only one button doesn't make a lot of sense. We could have had an intermediate state with no checkbox but two buttons, but it may be better to do everything at once now that we have the checkbox implemented.

Comment on attachment 8784798 [details] Bug 1291642 - Part 4 - Fix tests for permission notifications now using the checkbox. https://reviewboard.mozilla.org/r/74114/#review72028

(In reply to Johann Hofmann [:johannh] from comment #27) > I just discussed this with Dao and he suggested to hardcode on Mac, do a > -moz-windows-default-theme media query on Windows and maybe just leave the > red away on Linux. If that sounds ok to you I'll go ahead with it. Sounds good.

Matt, just a heads up that we're adding this checkbox feature to PopupNotifications.jsm. I've reviewed the changes and Johann is now updating the patch. I'm confident the new version is what we need but let me know if you have any comment.

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. https://reviewboard.mozilla.org/r/71970/#review72352 ::: toolkit/modules/PopupNotifications.jsm:276 (Diff revision 3) > * - callback (function): a callback to be invoked when the button is > - * pressed. > + * pressed, is passed the value of the checkbox as argument. Nit: "as argument" is missing a word or words. I kinda wonder if a boolean argument is the best idea or if an object with a boolean property would be a better API? I guess I'm not sure what else we would add to the argument list but having a main action callback get a boolean argument about a different UI widget feels a bit unusual to me. An object represenating the state of the notification feels more natural to me. ::: toolkit/modules/PopupNotifications.jsm:346 (Diff revision 3) > + * checked: > + * (optional) Whether the checkbox should be checked. > + * Defaults to false. > + * disableMainAction: > + * (optional) Whether the mainAction is disabled with > + * the checkbox checked. Defaults to false. > + * disableInfo: > + * (optional) Text to be shown if the disableMainAction > + * option is set and the checkbox is enabled. Are you maintaining the state of the checkbox's checkedness between minimizing and re-showing? I think you should but I don't recall if you get that for free with how the panel is implemented or not. I realize this won't be a problem if you can't minimize the panel but that is also an option on the API so it's possible for a dismissable panel to request a checkbox. Please make sure there is a test for this. ::: toolkit/modules/PopupNotifications.jsm:347 (Diff revision 3) > + * show: Whether or not to the checkbox. Defaults to false. > + * label: > + * (optional) Text to override the default label. > + * Defaults to "Remember this decision". > + * checked: > + * (optional) Whether the checkbox should be checked. Nit: s/be checked/be checked by default/

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. https://reviewboard.mozilla.org/r/71970/#review72008 > Not sure if "show" is needed, at least it should default to true if the object is present? I see from the test you've added it for the case where we don't have other arguments, but maybe we could support "checkbox: true" instead. > > Actually, we could sidestep the above and make the "label" mandatory. We don't have default permission-related labels for buttons, so why for the checkbox? A helper in the permissions code could provide the common value, and it would be more logical to find it there. I agree that we should get rid of `show` and make the `label` mandatory with no fallback. Before I saw this suggestion I was going to say the following about the notification.xml field: > IMO this field should be more descriptively named to describe its function not the UI widget used. For example, "rememberCheckbox" would be better and the anonid should also be updated. > > "checkbox" would be fine for a generic checkbox where the consumer provides the string but that's not what this is. > Helper needed. I agree, it seems like you copied this code from `_onButtonEvent` so factor the common part into a helper.

Comment on attachment 8784405 [details] Bug 1291642 - Part 2 - Add tests for the checkbox option in PopupNotifications. https://reviewboard.mozilla.org/r/73878/#review72356 ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:205 (Diff revision 1) > + // Test that passing the checkbox field shows the checkbox. > + { id: "Test#8", > + run: function* () { Please put these in a new file like browser_popupNotification_checkbox.js especially since there is so little boilerplate for these tests. It makes it easier to find tests and reduces the impact when a test file gets skipped due to intermittents. ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:206 (Diff revision 1) > goNext(); > }); > } > - } > + }, > + // Test that passing the checkbox field shows the checkbox. > + { id: "Test#8", Nit: You don't have to use meaningless numbers for the `id`. Examples: "checkbox" "checkbox_advanced" "checkbox_disableMainAction" etc. ::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:246 (Diff revision 1) > + // Test checkbox disabling the main action. > + { id: "Test#10", As I said in the other commit, please add a test to make sure that the state is remember upon minimizing and re-showing

Comment on attachment 8784797 [details] Bug 1291642 - Part 3 - Update permission notifications to use checkbox in PopupNotifications. https://reviewboard.mozilla.org/r/74112/#review72358 I didn't look at this patch closely yet since it's late and there's a lot going on. ::: browser/components/nsBrowserGlue.js (Diff revision 1) > - if (PrivateBrowsingUtils.isWindowPrivate(chromeWin) && > - promptAction.expireType != Ci.nsIPermissionManager.EXPIRE_SESSION && > - promptAction.action) { > - continue; > - } Why are you moving the PB remember logic out of the `_showPrompt` helper to each consumer? I think you should still have code here that removes/nulls `aOptions.checkbox` if we're in a private window so each consumer doesn't have to do that. ::: browser/components/nsBrowserGlue.js:2637 (Diff revision 1) > + options.checkbox.label = > + gBrowserBundle.GetStringFromName("webNotifications.rememberForSession"); This seems unusual to me. I think the checkbox should be hidden or disabled in PB since the main action is for the session already regardless of the checkbox state

(In reply to Matthew N. [:MattN] from comment #32) > Comment on attachment 8781561 [details] > Bug 1291642 - Part 1 - Add an optional checkbox to PopupNotifications. > > https://reviewboard.mozilla.org/r/71970/#review72352 > I kinda wonder if a boolean argument is the best idea or if an object with a > boolean property would be a better API? I guess I'm not sure what else we > would add to the argument list but having a main action callback get a > boolean argument about a different UI widget feels a bit unusual to me. An > object represenating the state of the notification feels more natural to me. I had the same thoughts actually. I'll just pass an object with a checkboxChecked field or something, that might make for a cleaner API. > ::: toolkit/modules/PopupNotifications.jsm:346 > (Diff revision 3) > > + * checked: > > + * (optional) Whether the checkbox should be checked. > > + * Defaults to false. > > + * disableMainAction: > > + * (optional) Whether the mainAction is disabled with > > + * the checkbox checked. Defaults to false. > > + * disableInfo: > > + * (optional) Text to be shown if the disableMainAction > > + * option is set and the checkbox is enabled. > > Are you maintaining the state of the checkbox's checkedness between > minimizing and re-showing? I think you should but I don't recall if you get > that for free with how the panel is implemented or not. Nope, never thought of that. Thanks for bringing it up. > I didn't look at this patch closely yet since it's late and there's a lot going on. Actually I think I'll move these two patches into Bug 1282768, so you don't have to look too closely right now. :)

(In reply to Johann Hofmann [:johannh] from comment #36) > (In reply to Matthew N. [:MattN] from comment #32) > > I didn't look at this patch closely yet since it's late and there's a lot going on. > > Actually I think I'll move these two patches into Bug 1282768, so you don't > have to look too closely right now. :) I was going to suggest moving the last two patches to another bug too :)

Renaming this to clarify our focus on the API patch.

The only comment that was not addressed (afaik) is this one: > Comment on attachment 8781561 [details] > Bug 1291642 - Part 1 - Add an optional checkbox to PopupNotifications. > > I think this would be clearer: > > whenChecked: { disableMainAction: true, > warningLabel: "..." }, > whenUnchecked: { ... }, > > We don't need to implement whenUnchecked now but I guess it would simplify > your implementation. > I'm not sure if this syntax makes more sense. To me it sounds semantically weird to have "whenChecked" (which sounds more like a callback) be a configuration object. It sounds a bit like we'll just pile up whatever behavior we want into these objects as boolean flags, bloating the API. Theoretically you could produce undefined behavior by implementing both "whenUnchecked" and "whenChecked. I know nobody would do that but designing an API that doesn't prevent this doesn't sound optimal and hints at potential problems in the future. I'm starting to think that just taking a callback that allows the caller to manually disable things might be smoother. What do you think?

(In reply to Johann Hofmann [:johannh] from comment #41) > weird to have "whenChecked" (which sounds more like a callback) be a > configuration object. We could call this stateWhenChecked or something. > Theoretically you could produce undefined behavior by implementing both > "whenUnchecked" and "whenChecked". I know nobody would do that but designing > an API that doesn't prevent this doesn't sound optimal and hints at > potential problems in the future. I don't understand this, what do you mean by undefined behavior? > I'm starting to think that just taking a callback that allows the caller to > manually disable things might be smoother. Well, as I've said this is definitely another option to consider and it's definitely more flexible, though it leaves open the question of what is the right interface to make the state changes. While a callback is probably better, I don't find the declarative way to be that bad either for what we need to do now.

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. https://reviewboard.mozilla.org/r/71970/#review73544 ::: toolkit/modules/PopupNotifications.jsm:60 (Diff revision 4) > return null; > } > > +function getNotificationFromElement (aElement) { > + // Need to find the associated notification object, which is a bit tricky > + // since it isn't associated with the button directly - this is kind of nit: button -> element in the comment, no space after function name, and maybe the code can be rewritten more clearly? ::: toolkit/modules/PopupNotifications.jsm:736 (Diff revision 4) > } > } > > + let checkbox = n.options.checkbox; > + if (checkbox && checkbox.label) { > + let checked = n._checkboxChecked || !!checkbox.checked; You probably need to check if you actually set "_checkboxChecked" previously. You may also initialize the state of "_checkboxChecked" in advance on creation, and just use it here. ::: toolkit/modules/PopupNotifications.jsm:741 (Diff revision 4) > + if (checkbox.disableMainAction) { > + popupnotification.setAttribute("checkboxcommand", `PopupNotifications._onCheckboxCommand(event);`); > + if (checkbox.warningLabel) { > + popupnotification.setAttribute("warninglabel", checkbox.warningLabel); > + } > + this._toggleMainAction(popupnotification, checked); > + } else { > + popupnotification.removeAttribute("checkboxcommand"); > + } > + } else { > + popupnotification.setAttribute("checkboxhidden", true); > + } > + > this.panel.appendChild(popupnotification); > > // The popupnotification may be hidden if we got it from the chrome > // document rather than creating it ad hoc. > popupnotification.hidden = false; > }, this); > }, > > + _toggleMainAction(notification, disabled) { > + if (disabled) { > + notification.setAttribute("mainactiondisabled", true); > + if (notification.getAttribute("warninglabel")) { > + notification.setAttribute("warninghidden", false); > + } > + } else { > + notification.removeAttribute("mainactiondisabled"); > + notification.setAttribute("warninghidden", true); > + } > + }, Thinking about it, you might actually want a notification.setUIState({ mainActionDisabled, warningLabel }) method. You can call it conditionally with the stateWhenChecked and stateWhenUnchecked objects here and in the _onCheckboxCommand event. Alternatively you may invoke a callback provided by the consumer, and it would call setUIState on the notification. You just need to provide it with the notification object and the checkbox state. You may put the latter on the notification object, actually.

Comment on attachment 8784405 [details] Bug 1291642 - Part 2 - Add tests for the checkbox option in PopupNotifications. https://reviewboard.mozilla.org/r/73878/#review73550

Yeah, I've been pondering this a lot and stateWhenChecked (maybe rename to checkedState?) is the best I can think of right now. I'll implement that, sorry for holding up.

(In reply to :Paolo Amadini from comment #43) > maybe the code can be rewritten more clearly? It's pretty concise but looks readable to me, anything in particular that you find unclear? > ::: toolkit/modules/PopupNotifications.jsm:736 > (Diff revision 4) > > } > > } > > > > + let checkbox = n.options.checkbox; > > + if (checkbox && checkbox.label) { > > + let checked = n._checkboxChecked || !!checkbox.checked; > > You probably need to check if you actually set "_checkboxChecked" > previously. You may also initialize the state of "_checkboxChecked" in > advance on creation, and just use it here. > We could initialize it to null to declare that it exists, but the || operator filters falsey values for us so if it's not defined it'll just use !!checkbox.checked, which looks enough for me. > ::: toolkit/modules/PopupNotifications.jsm:741 > (Diff revision 4) > > + if (checkbox.disableMainAction) { > > + popupnotification.setAttribute("checkboxcommand", `PopupNotifications._onCheckboxCommand(event);`); > > + if (checkbox.warningLabel) { > > + popupnotification.setAttribute("warninglabel", checkbox.warningLabel); > > + } > > + this._toggleMainAction(popupnotification, checked); > > + } else { > > + popupnotification.removeAttribute("checkboxcommand"); > > + } > > + } else { > > + popupnotification.setAttribute("checkboxhidden", true); > > + } > > + > > this.panel.appendChild(popupnotification); > > > > // The popupnotification may be hidden if we got it from the chrome > > // document rather than creating it ad hoc. > > popupnotification.hidden = false; > > }, this); > > }, > > > > + _toggleMainAction(notification, disabled) { > > + if (disabled) { > > + notification.setAttribute("mainactiondisabled", true); > > + if (notification.getAttribute("warninglabel")) { > > + notification.setAttribute("warninghidden", false); > > + } > > + } else { > > + notification.removeAttribute("mainactiondisabled"); > > + notification.setAttribute("warninghidden", true); > > + } > > + }, > > Thinking about it, you might actually want a notification.setUIState({ > mainActionDisabled, warningLabel }) method. You can call it conditionally > with the stateWhenChecked and stateWhenUnchecked objects here and in the > _onCheckboxCommand event. > > Alternatively you may invoke a callback provided by the consumer, and it > would call setUIState on the notification. You just need to provide it with > the notification object and the checkbox state. You may put the latter on > the notification object, actually. I really like the simplicity of calling setUIState with the object that was provided by the consumer!

(In reply to Johann Hofmann [:johannh] from comment #46) > (In reply to :Paolo Amadini from comment #43) > > You probably need to check if you actually set "_checkboxChecked" > > previously. You may also initialize the state of "_checkboxChecked" in > > advance on creation, and just use it here. > > > > We could initialize it to null to declare that it exists, but the || > operator filters falsey values for us so if it's not defined it'll just use > !!checkbox.checked, which looks enough for me. Ok thinking about it you're right we have to cover the case where checkbox.checked was defaulted to `true` but the user unchecked it. Good catch, thanks!

Needed to make setUIState -> _setNotificationUIState, because the element is not initialized when first calling that method :( If you know a way around this I'm all ears :)

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. https://reviewboard.mozilla.org/r/71970/#review73952 ::: toolkit/modules/PopupNotifications.jsm:350 (Diff revision 5) > + * checkbox: An object that allows you to control the behavior of > + * the "Remember this decision" checkbox with these fields: > + * label: > + * (required) Text to override the default label. Should update ths part of the comment. ::: toolkit/modules/PopupNotifications.jsm:745 (Diff revision 5) > + let checked = checkbox.checked; > + if (n._checkboxChecked != null) { > + checked = n._checkboxChecked; > + } let checked = n._checkboxChecked !== null ? n._checkboxChecked : !!checkbox.checked; ::: toolkit/modules/PopupNotifications.jsm:750 (Diff revision 5) > + let checked = checkbox.checked; > + if (n._checkboxChecked != null) { > + checked = n._checkboxChecked; > + } > + > + popupnotification.setAttribute("checkboxhidden", false); nit: "false" and "true" can be strings. ::: toolkit/modules/PopupNotifications.jsm:754 (Diff revision 5) > + > + popupnotification.setAttribute("checkboxhidden", false); > + popupnotification.setAttribute("checkboxchecked", checked); > + popupnotification.setAttribute("checkboxlabel", checkbox.label); > + > + popupnotification.setAttribute("checkboxcommand", `PopupNotifications._onCheckboxCommand(event);`); Can use normal quotes. ::: toolkit/modules/PopupNotifications.jsm:1228 (Diff revision 5) > } > > notification._recordTelemetryStat(TELEMETRY_STAT_ACTION_1); > > try { > - notification.mainAction.callback.call(); > + notification.mainAction.callback.call(null, { nit: undefined

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. Matt may have some final comments before this lands.

Comment on attachment 8784405 [details] Bug 1291642 - Part 2 - Add tests for the checkbox option in PopupNotifications. https://reviewboard.mozilla.org/r/73878/#review73954 ::: browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js:29 (Diff revision 3) > + // Test that passing the checkbox field shows the checkbox. > + { id: "show_checkbox", > + run: function () { > + this.notifyObj = new BasicNotification(this.id); > + this.notifyObj.options.checkbox = { > + label: "This is a checkbox" nit: I generally put a comma at the end of all multi-line initializers, it's more consistent when you need to add lines later. I don't think this is followed consistently in the code base though. ::: browser/base/content/test/popupNotifications/browser_popupNotification_checkbox.js:61 (Diff revision 3) > + triggerMainCommand(popup); > + }, > + onHidden: function () { } > + }, > + > + // Test checkbox disabling the main action. Worth having a similar case where the checkbox is initially unchecked, we have uncheckedState with disableMainAction. We should test that the action is enabled when the checkbox is checked.

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. Paolo, can you check the changes in webrtcUI.jsm? Nothing exciting, this code will change soon anyway. Thanks!

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. Ah, weird way of using the callbacks in the WebRTC code, the fix makes sense. Good that it was caught by a regression test.

(In reply to :Paolo Amadini from comment #52) > Matt may have some final comments before this lands. From a quick skim of the changes since revision 5 I think this is fine.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c37f66cfffcaff8c09f61fe6417605d1a24bf8ab Bug 1291642 - Part 1 - Add an optional checkbox to PopupNotifications. r=paolo https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1eefbee0df4c5f7b0d43be16e36a228f05f1cd Bug 1291642 - Part 2 - Add tests for the checkbox option in PopupNotifications. r=paolo

https://hg.mozilla.org/mozilla-central/rev/c37f66cfffca https://hg.mozilla.org/mozilla-central/rev/3a1eefbee0df

+@media (-moz-windows-default-theme) { + color: #aa1b08; +} This is quite broken...

Wow, thanks for catching this, I'll make a quick followup patch.

Comment on attachment 8781561 [details] Bug 1291642 followup - Add a CSS selector for warning labels on windows. Nice how this automatically hides the original patch. ReviewBoard really annoys me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/22edc38ba187a99bc0d14430809ad0a14a63edac Bug 1291642 followup - Add a CSS selector for warning labels on windows. r=dao

(In reply to Dão Gottwald [:dao] from comment #65) > +@media (-moz-windows-default-theme) { > + color: #aa1b08; > +} > > This is quite broken... Is this something a linter could/should catch?

Hi Johann, Could you provide some details on how to test this? I don't see the checkbox when triggering the permissions on http://permission.site/ in latest Nightly.

(In reply to Florian Quèze [:florian] [:flo] from comment #70) > (In reply to Dão Gottwald [:dao] from comment #65) > > +@media (-moz-windows-default-theme) { > > + color: #aa1b08; > > +} > > > > This is quite broken... > > Is this something a linter could/should catch? This should be caught by the browser_parsable_css.js mochitest, but unfortunately it currently verifies only the browser/ files and this is in toolkit. See bug 1221383.

(In reply to Paul Silaghi, QA [:pauly] from comment #72) > Hi Johann, > Could you provide some details on how to test this? > I don't see the checkbox when triggering the permissions on > http://permission.site/ in latest Nightly. Hi Paul, this is an API-only change, so no need to test this. The verify flag was set when we had a different idea of the scope of this bug. It would be great if you could pay special attention to the checkbox once Bug 1282768 lands in Nightly. :) Thanks! Sorry for the confusion...

(In reply to Johann Hofmann [:johannh] from comment #74) > It would be great if you could pay special attention to the checkbox once Bug 1282768 > lands in Nightly. :) Sure. Thanks

[bugday-20170118] Bug Verified