In bug 802326 we removed the "No Video" and "No Audio" menu items from the gUM permission prompts. The "No Window"/screen/application menuitem remains, because we can't pick a default window to share for the user. This is inconsistent, and when the user clicks the big "Share Selected Items" button without selecting a window first, we return a SecurityError to the website, despite the user thinking the permission has been granted. I think a less broken UI would be to replace the "No Window" item with a disabled "(select window)" item, and to disable the "Share Selected Items" button until something is selected. I'm not sure if this is actually worth implementing now though, as the permission prompts will be redesigned soon.

Hi Florian. This issue appears to have survived the redesign, and still seems to be a problem. This came up as a priority concern with a large site recently, who reported that it confuses their users. I agree with the quick remedy you propose. Anything we can do to prioritize a fix here? Thanks.

Summary of the IRC conversation jib and I had yesterday: - changing "No Window" to "(select window)" is a trivial string change, so easy to do, but we can't uplift it. - Disabling the button until something is selected likely requires a few lines of code, but shouldn't be really difficult; we already disable that button when the "remember this decision" checkbox is checked. Thinking about this some more, we'll want to have the button disabling behavior covered by a test, and that's probably what's going to take the most effort. - The people who are familiar with this code and can patch it easily include myself, johannh and I think also jkt. jkt, do you happen to have a few cycles to take this? (johannh and myself are buried in photon work these days) I'm happy to review patches if needed :-).

Discussed with jkt on irc, who will take it.

Is this in the right direction :florian I can add tests and fix others etc after: https://reviewboard.mozilla.org/r/168010/diff/1#index_header

Hey would you be able to have a look at my initial patch I'm going to work on it today to try and get it in before merge date. If you could advise if it is in the right direction that would be super helpful. Thanks

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review174398 ::: browser/locales/en-US/chrome/browser/browser.properties:659 (Diff revision 1) > getUserMedia.selectScreen.accesskey=S > getUserMedia.selectApplication.label=Application to share: > getUserMedia.selectApplication.accesskey=A > getUserMedia.noApplication.label = No Application > getUserMedia.noScreen.label = No Screen > getUserMedia.noWindow.label = No Window are these strings still used? ::: browser/modules/webrtcUI.jsm:446 (Diff revision 1) > + notificationElement.setAttribute("mainactiondisabled", "true"); > + } else { > + notificationElement.removeAttribute("mainactiondisabled"); > + } > + } > + if (aTopic == "shown") { Could this be done during the "showing" event? If not, please add a comment explaining why. ::: browser/modules/webrtcUI.jsm:447 (Diff revision 1) > + } else { > + notificationElement.removeAttribute("mainactiondisabled"); > + } > + } > + if (aTopic == "shown") { > + checkDisabledWindowMenuItem(); When showing the panel, I think we can just add the mainactiondisabled attribute if 'screenSharing' is true without further check of the menulist's selected item. ::: browser/modules/webrtcUI.jsm:577 (Diff revision 1) > - // "No <type>" is the default because we can't pick a > + // "Select <type>" is the default because we can't pick a > // 'default' window to share. > - addDeviceToList(menupopup, > - stringBundle.getString("getUserMedia.no" + typeName + ".label"), > + const deviceItem = addDeviceToList(menupopup, > + stringBundle.getString("getUserMedia.pick" + typeName + ".label"), > "-1"); > + deviceItem.setAttribute("disabled", true); Could the addDeviceToList function instead add the disabled attribute when the deviceIndex is -1? It's usually preferable to set all attributes before inserting the node in the dom. ::: browser/modules/webrtcUI.jsm:707 (Diff revision 1) > let windowMenupopup = doc.getElementById("webRTC-selectWindow-menupopup"); > let micMenupopup = doc.getElementById("webRTC-selectMicrophone-menupopup"); > - if (sharingScreen) > + if (sharingScreen) { > listScreenShareDevices(windowMenupopup, videoDevices); > - else > + > + windowMenupopup.addEventListener("command", checkDisabledWindowMenuItem); You are adding this event listener each time the panel is shown (eg. each time we switch back to the tab), but never removing it. So your event listener will: 1. run multiple times 2. probably leak. Could we instead insert the checkDisabledWindowMenuItem code inside the existing command listener http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/browser/modules/webrtcUI.jsm#602 ?

(In reply to Jonathan Kingston [:jkt] from comment #6) > Is this in the right direction :florian I can add tests and fix others etc > after: https://reviewboard.mozilla.org/r/168010/diff/1#index_header Comment 8 was more a review than an answer to your question here, but yes I think it's the right direction. I'm leaving the needinfo on johannh because he may have some interesting input on the use of the mainactiondisabled attribute from outside PopupNotifications.jsm.

Indeed, you have two problems here: - As Florian mentioned, you should set the attribute during the "showing" event to avoid the button flickering from blue to grey. But you can't do that as simple as this, because it will get overriden by https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/toolkit/modules/PopupNotifications.jsm#872 afterwards. - This function: https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/toolkit/modules/PopupNotifications.jsm#903 will override your manual mainactiondisabled whenever the user toggles the checkbox. I can't come up with a great solution to this off-hand, I'll need to think about it. Feel free to tinker around more and/or chat me up on IRC.

This might not make the cut for the merge date unfortunately. I have been having trouble getting enough time to work on it. I will be on PTO on Monday also. My current WIP patch creates an event handler that listens for changes within the popup and should end up checking both the checkbox and the select for their state. The issue I have been having is grabbing the right elements within the popup, however I think the logic is sound and a little better than my initial solution.

This is ready for another look I think, there is a TODO and 3 eslint fixes I need to do that I will when I get home. Thanks!

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review184492 ::: browser/modules/webrtcUI.jsm:437 (Diff revision 5) > if (aTopic == "swapping") > return true; > > let doc = this.browser.ownerDocument; > > + function checkDisabledWindowMenuItem() { Please move this down in the file so that it's after the early returns. I would place it right before the "function listDevices(menupopup, devices)" line. ::: browser/modules/webrtcUI.jsm:439 (Diff revision 5) > > let doc = this.browser.ownerDocument; > > + function checkDisabledWindowMenuItem() { > + const list = doc.getElementById("webRTC-selectWindow-menulist"); > + const item = list.selectedItem; nit: These 2 'const' should be 'let'. ::: browser/modules/webrtcUI.jsm:616 (Diff revision 5) > // Always re-select the "No <type>" item. > doc.getElementById("webRTC-selectWindow-menulist").removeAttribute("value"); > doc.getElementById("webRTC-all-windows-shared").hidden = true; > + > menupopup._commandEventListener = event => { > + if (sharingScreen) { why do you need this test for code within the listScreenShareDevices function? ::: browser/modules/webrtcUI.jsm:695 (Diff revision 5) > menuitem.setAttribute("label", deviceName); > menuitem.setAttribute("tooltiptext", deviceName); > if (type) > menuitem.setAttribute("devicetype", type); > + > + if (deviceIndex === "-1") nit: == is enough ::: toolkit/modules/PopupNotifications.jsm:61 (Diff revision 5) > > +function getNotificationFromElementWithAnon(aElement) { > + let parent = aElement; > + let notificationEl; > + while (parent > + && (parent = aElement.ownerDocument.getBindingParent(parent) || parent.parentNode)) { nit: && operator before the line break ::: toolkit/modules/PopupNotifications.jsm:62 (Diff revision 5) > +function getNotificationFromElementWithAnon(aElement) { > + let parent = aElement; > + let notificationEl; > + while (parent > + && (parent = aElement.ownerDocument.getBindingParent(parent) || parent.parentNode)) { > + if (parent.localName === "popupnotification") nit: == is enough ::: toolkit/modules/PopupNotifications.jsm:63 (Diff revision 5) > + let parent = aElement; > + let notificationEl; > + while (parent > + && (parent = aElement.ownerDocument.getBindingParent(parent) || parent.parentNode)) { > + if (parent.localName === "popupnotification") > + notificationEl = parent; should this be 'return parent;'? We can't have nested popupnotification nodes, right? ::: toolkit/modules/PopupNotifications.jsm:904 (Diff revision 5) > }, this); > }, > > _setNotificationUIState(notification, state = {}) { > - if (state.disableMainAction) { > + if (state.disableMainAction > + || !this._satisfiesDeviceRequirement(notification)) { || on the previous line ::: toolkit/modules/PopupNotifications.jsm:917 (Diff revision 5) > } else { > notification.setAttribute("warninghidden", "true"); > } > }, > > - _onCheckboxCommand(event) { > + _satisfiesDeviceRequirement(notificationEl) { PopupNotifications.jsm is a generic module, it shouldn't contain WebRTC specific names ('device', 'screen'). ::: toolkit/modules/PopupNotifications.jsm:1428 (Diff revision 5) > }, this); > }, > > + _onUpdateStateEvent(event) { > + let target = event.originalTarget; > + let notificationEl = getNotificationFromElementWithAnon(target); If this is called only once, can you just inline it? ( with 'break' instead of 'return')

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review184492 > should this be 'return parent;'? We can't have nested popupnotification nodes, right? For binding parent no as it might be multiple layers right? > If this is called only once, can you just inline it? ( with 'break' instead of 'return') I was hoping we could use the same method for all cases and not be a separate method so it accounts for all use cases however will inline for now. (not sure that a break is ok here though)

(In reply to Jonathan Kingston [:jkt] from comment #19) > > should this be 'return parent;'? We can't have nested popupnotification nodes, right? > > For binding parent no as it might be multiple layers right? Can you give an example, I don't understand what you mean here. What I meant in my previous comment is that once you have found a node that has "popupnotification" as its localName, you can stop the loop. Is this not correct? > > If this is called only once, can you just inline it? ( with 'break' instead of 'return') > > I was hoping we could use the same method for all cases and not be a > separate method so it accounts for all use cases however will inline for > now. (not sure that a break is ok here though) I meant "break" because of the previous comment where I wanted to stop the loop.

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review184940 ::: toolkit/modules/PopupNotifications.jsm:907 (Diff revisions 5 - 6) > notification.setAttribute("warninghidden", "true"); > } > }, > > - _satisfiesDeviceRequirement(notificationEl) { > - let screenSelectionRequired = notificationEl.notification.options.screenSelectionRequired; > + _satisfiesSecondaryRequirement(notificationEl) { > + let screenSelectionRequired = notificationEl.notification.options.secondaryRequirement; SecondaryRequirement is very generic, and this variable still contains "screen". The "deviceunselected" attribute also has a reference to webrtc. Here's a naming proposal: SecondaryRequirement -> selectionRequired _satisfiesSecondaryRequirement -> _hasValidSelection "deviceunselected" -> "invalidselection" This whole method can be simplified: return !notificationEl.notification.options.secondaryRequirement || !notificationEl.hasAttribute("deviceunselected"); ::: toolkit/modules/PopupNotifications.jsm:1421 (Diff revisions 5 - 6) > let target = event.originalTarget; > - let notificationEl = getNotificationFromElementWithAnon(target); > + let parent = target; > + let notificationEl; > + // Find notification like getNotificationFromElement but some nodes are non-anon > + while (parent && > + (parent = target.ownerDocument.getBindingParent(parent) || parent.parentNode)) { This loop will go up to the root of the DOM tree. I think we should stop it as soon as we have found a node with localName == "popupnotification".

> What I meant in my previous comment is that once you have found a node that has "popupnotification" as its localName, you can stop the loop. Is this not correct? Sorry wasn't thinking about the if statement acting as a guard here and just trying to replicate the previous code. I think we are fine as you mention. I think I have it working just fine, going to push to try and review at the same time due to time.

I was getting try failures on linux for timeouts however I'm not seeing those locally which is odd. If either you or :johannh have a cycle to get this reviewed again that would be great as there is a high priority site wanting this.

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review185184 The code looks good to me (and local testing seems to show it behaves correctly), but I was going to say that this needs tests. And try was there to remind you :-). http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js needs changes: - to check that the 'Select <type>' item is disabled. - to check that the main action button is disabled when opening the panel before making a selection - to check that the main action button is no longer disabled once a selection has been made. - clicking the main action button without having made a selection to cause a permission error no longer works, so you'll need to replace several with PopupNotifications.panel.firstChild.button.click() activateSecondaryAction(kActionDeny). - the "getUserMedia screen: clicking through without selecting a screen denies" part of the test can likely just be removed, it no longer makes sense. And we should check the "disabled" and "invalidselection" attributes throughout the rest of the test file. It may also be a good idea to add a test for the toolkit/modules/PopupNotifications.jsm new feature. There are existing tests for this file in browser/base/content/test/popupNotifications ::: toolkit/modules/PopupNotifications.jsm:907 (Diff revision 7) > notification.setAttribute("warninghidden", "true"); > } > }, > > - _onCheckboxCommand(event) { > - let notificationEl = getNotificationFromElement(event.originalTarget); > + _hasValidSelection(notificationEl) { > + return !notificationEl.notification.options.selectionRequired || This new option should be documented in the comment of PopupNotifications.show ::: toolkit/modules/PopupNotifications.jsm:1411 (Diff revision 7) > }, this); > }, > > + _onUpdateStateEvent(event) { > + let target = event.originalTarget; > + let parent = target; I don't think you really need this parent variable, it seems you could s/parent/target/ througout the method and get the same behavior. ::: toolkit/modules/PopupNotifications.jsm:1421 (Diff revision 7) > + if (parent.localName == "popupnotification") { > + notificationEl = parent; > + break; > + } > + } > + if (notificationEl) { Do you really need this null check? I can't think of any case where this would be null. If you do need it, please return when it's null, instead of indenting the rest of the function.

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review185200 Sorry for being a bit slow on this. I don't have much to add to Florian's comments. This looks and works good for the most part. :) ::: toolkit/modules/PopupNotifications.jsm:1409 (Diff revision 7) > this._fireCallback(notificationObj, NOTIFICATION_EVENT_DISMISSED); > } > }, this); > }, > > + _onUpdateStateEvent(event) { I really don't understand this function name. Please add a comment and/or a better name for this. What's wrong with _onCommand? ::: toolkit/modules/PopupNotifications.jsm:1415 (Diff revision 7) > + let target = event.originalTarget; > + let parent = target; > + let notificationEl; > + // Find notification like getNotificationFromElement but some nodes are non-anon > + while (parent && > + (parent = target.ownerDocument.getBindingParent(parent) || parent.parentNode)) { I realize that this is just a variant of the loop I wrote myself, but the while condition got a lot harder to understand now. Can you put the second part inside the while loop please?

Fixed all comments, are we able to uplift/land this still? Thanks both

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review186146 Looks reasonable, but I should probably have a very quick look at the next version. ::: toolkit/modules/PopupNotifications.jsm:1413 (Diff revisions 7 - 8) > } > }, this); > }, > > - _onUpdateStateEvent(event) { > - let target = event.originalTarget; > + _onCommand(event) { > + let parent = event.originalTarget; Now that you are doing the localName check before the .parentNode, I think you can use .target instead of .originalTarget Also, you don't really need the 'parent' variable, you could just use 'notificationEl' directly everywhere. ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:34 (Diff revision 8) > > is(PopupNotifications.getNotification("webRTC-shareDevices").anchorID, > "webRTC-shareScreen-notification-icon", "anchored to device icon"); > checkDeviceSelectors(false, false, true); > let iconclass = > PopupNotifications.panel.firstChild.getAttribute("iconclass"); nit: your 'notification' variable would be useful here too. ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:47 (Diff revision 8) > > + let notification = PopupNotifications.panel.firstChild; > let noScreenItem = menulist.getItemAtIndex(0); > - ok(noScreenItem.hasAttribute("selected"), "the 'No Screen' item is selected"); > + ok(noScreenItem.hasAttribute("selected"), "the 'Select Screen' item is selected"); > is(menulist.value, -1, "no screen is selected by default"); > - is(menulist.selectedItem, noScreenItem, "'No Screen' is the selected item"); > + ok(noScreenItem.disabled, "Item is disabled"); "'Select Screen' item is disabled" ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:50 (Diff revision 8) > - ok(noScreenItem.hasAttribute("selected"), "the 'No Screen' item is selected"); > + ok(noScreenItem.hasAttribute("selected"), "the 'Select Screen' item is selected"); > is(menulist.value, -1, "no screen is selected by default"); > - is(menulist.selectedItem, noScreenItem, "'No Screen' is the selected item"); > + ok(noScreenItem.disabled, "Item is disabled"); > + ok(notification.button.disabled, "Allow button is disabled"); > + ok(notification.hasAttribute("invalidselection"), "Notification is marked as invalid"); > + is(menulist.selectedItem, noScreenItem, "'Select Screen' is the selected item"); This line should go between ok(noScreenItem.hasAttribute("selected")... and is(menulist.value, -1... Note: these 3 comments about the test apply for each of the 3 parts of the test. ::: toolkit/modules/PopupNotifications.jsm:910 (Diff revision 8) > notification.setAttribute("warninghidden", "true"); > } > }, > > - _onCheckboxCommand(event) { > - let notificationEl = getNotificationFromElement(event.originalTarget); > + _hasValidSelection(notificationEl) { > + return !notificationEl.notification.options.selectionRequired || If the only point of the 'selectionRequired" option is to save us an hasAttribute call, I think we could as well remove it and inline the .hasAttribute call instead of having the _hasValidSelection method. If you remove the option, don't forget to also remove the documentation in the comment, and the related line in the test.

(In reply to Jonathan Kingston [:jkt] from comment #28) > Fixed all comments, are we able to uplift/land this still? > Thanks both We should be able to land this either today or tomorrow and make it into 57, afaiu. Nothing to add to florian's comments :)

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review186196 ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:35 (Diff revisions 8 - 9) > is(PopupNotifications.getNotification("webRTC-shareDevices").anchorID, > "webRTC-shareScreen-notification-icon", "anchored to device icon"); > checkDeviceSelectors(false, false, true); > + let notification = PopupNotifications.panel.firstChild; > let iconclass = > - PopupNotifications.panel.firstChild.getAttribute("iconclass"); > + notification.getAttribute("iconclass"); nit: remove useless line break. ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:48 (Diff revisions 8 - 9) > - let notification = PopupNotifications.panel.firstChild; > let noScreenItem = menulist.getItemAtIndex(0); > ok(noScreenItem.hasAttribute("selected"), "the 'Select Screen' item is selected"); > + is(menulist.selectedItem, noScreenItem, "'Select Screen' is the selected item"); > is(menulist.value, -1, "no screen is selected by default"); > - ok(noScreenItem.disabled, "Item is disabled"); > + ok(noScreenItem.disabled, "'No Screen' item is disabled"); 'Select Screen' ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:132 (Diff revisions 8 - 9) > > is(PopupNotifications.getNotification("webRTC-shareDevices").anchorID, > "webRTC-shareScreen-notification-icon", "anchored to device icon"); > checkDeviceSelectors(false, false, true); > let iconclass = > PopupNotifications.panel.firstChild.getAttribute("iconclass"); This is your notification variable again. ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:146 (Diff revisions 8 - 9) > let noWindowItem = menulist.getItemAtIndex(0); > ok(noWindowItem.hasAttribute("selected"), "the 'Select Window' item is selected"); > - is(menulist.value, -1, "no window is selected by default"); > is(menulist.selectedItem, noWindowItem, "'Select Window' is the selected item"); > - ok(noWindowItem.disabled, "Item is disabled"); > + is(menulist.value, -1, "no window is selected by default"); > + ok(noWindowItem.disabled, "'No Window' item is disabled"); Select Window ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:242 (Diff revisions 8 - 9) > > is(PopupNotifications.getNotification("webRTC-shareDevices").anchorID, > "webRTC-shareScreen-notification-icon", "anchored to device icon"); > checkDeviceSelectors(false, false, true); > let iconclass = > PopupNotifications.panel.firstChild.getAttribute("iconclass"); Here too ::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:256 (Diff revisions 8 - 9) > let noApplicationItem = menulist.getItemAtIndex(0); > ok(noApplicationItem.hasAttribute("selected"), "the 'Select Application' item is selected"); > - is(menulist.value, -1, "no app is selected by default"); > is(menulist.selectedItem, noApplicationItem, "'Select Application' is the selected item"); > - ok(noApplicationItem.disabled, "Item is disabled"); > + is(menulist.value, -1, "no app is selected by default"); > + ok(noApplicationItem.disabled, "'No Application' item is disabled"); Select Application ::: toolkit/modules/PopupNotifications.jsm:1411 (Diff revisions 8 - 9) > - while (parent) { > - if (parent.localName == "popupnotification") { > + while (notificationEl) { > + if (notificationEl.localName == "popupnotification") { > - notificationEl = parent; > break; > } > - parent = target.ownerDocument.getBindingParent(parent) || parent.parentNode; > + notificationEl = notificationEl.ownerDocument.getBindingParent(parent) || notificationEl.parentNode; wrap after '=' ::: toolkit/modules/PopupNotifications.jsm:1411 (Diff revisions 8 - 9) > - while (parent) { > - if (parent.localName == "popupnotification") { > + while (notificationEl) { > + if (notificationEl.localName == "popupnotification") { > - notificationEl = parent; > break; > } > - parent = target.ownerDocument.getBindingParent(parent) || parent.parentNode; > + notificationEl = notificationEl.ownerDocument.getBindingParent(parent) || notificationEl.parentNode; What is 'parent' here?

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review186204

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review186212 Looks good to me now, but you told me on IRC that some tests are failing, so let's figure this out tomorrow.

So I didn't see from the comments that options.checkbox wasn't mandatory so I was assuming that the code would always work. In the latest change I reinstated the checkboxCommand as before as manipulating conditionals around that not existing made the onCommand much more complex for minimal gain. The onCommand is now simpler and just ignores button calls which was the other test failure as it seems that the code was juggling around state when the notification variable wasn't always present or other elements it seems. The onCommand also ignores checkboxes which is handled by checkboxCommand, this doesn't feel like the greatest solution in that we are ignoring elements based on what other code might have tied events to it, other solutions I could come up with are: - make all the selects have their own onSelectCommand or similar, this likely might cause it's own issues and require lots of code change. - In onCheckboxCommand prevent default, again this couples the two events somewhat together also. The current solution felt like the least icky and minimal code changes on what was already reviewed.

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review186496 ::: toolkit/modules/PopupNotifications.jsm:1422 (Diff revisions 11 - 12) > + this._setNotificationUIState(notificationEl, notification.options.checkbox.uncheckedState); > + } > + }, > + > _onCommand(event) { > - // Ignore events from buttons > + // Ignore events from buttons and checkboxes This comment doesn't explain anything. Why do we need to ignore these? ::: toolkit/modules/PopupNotifications.jsm:1423 (Diff revisions 11 - 12) > + } > + }, > + > _onCommand(event) { > - // Ignore events from buttons > - if (event.originalTarget.localName == "button") { > + // Ignore events from buttons and checkboxes > + const targetLocalName = event.originalTarget.localName; let

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review186610 Looks good now. ::: toolkit/modules/PopupNotifications.jsm:1424 (Diff revisions 13 - 14) > + event.stopPropagation(); > }, > > _onCommand(event) { > - // Ignore events from buttons and checkboxes > - const targetLocalName = event.originalTarget.localName; > + // Ignore events from buttons > + // as they are submitting and so don't need checks nit: this comment can fit on a single line. ::: toolkit/modules/PopupNotifications.jsm:1425 (Diff revisions 13 - 14) > }, > > _onCommand(event) { > - // Ignore events from buttons and checkboxes > - const targetLocalName = event.originalTarget.localName; > - if (targetLocalName == "button" || > + // Ignore events from buttons > + // as they are submitting and so don't need checks > + let targetLocalName = event.originalTarget.localName; Nit: remove that variable now that it's only used once. ::: toolkit/modules/PopupNotifications.jsm:1438 (Diff revisions 13 - 14) > break; > } > notificationEl = > notificationEl.ownerDocument.getBindingParent(notificationEl) || notificationEl.parentNode; > } > - > + if (notificationEl) { Please remove this if it's not needed (private IRC discussion seems to indicate it was added when attempting to debug tests but maybe wasn't actually needed).

Comment on attachment 8896716 [details] Bug 1272304 - Add disabled state to screen sharing permission https://reviewboard.mozilla.org/r/168010/#review186664

Pushed by florian@queze.net: https://hg.mozilla.org/integration/autoland/rev/c799d0ca5969 Add disabled state to screen sharing permission r=florian

Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=ba69e294fc80f23d54fc68050f1b7d63d341aa9c&newProject=mozilla-central&newRev=c0d1f9eb2a4090ab8a06ef3560ab5d0e06c5bb1f&filter=screen