User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49.1 Build ID: 20170527140225 Steps to reproduce: Tested geolocation API at https://developer.mozilla.org/en-US/docs/Web/API/Geolocation/Using_geolocation Actual results: After approving request, the notification disappears, but an error appears in Browser Console (PopupNotifications.jsm:1292 - notificationEl.checkbox is null) and the approval does not go through. Expected results: The notification should have been approved

This patch ports the old-fashioned notification bar (used for geolocation requests in the sidebar) to the main SeaMonkey window. It should tide things over until bug 1323494 is resolved. This should be applied to 2.49 ESR if possible.

Added a comment to the patch file.

The checkbox was added in bug 1291642 but it is optional. Top half af the picture SeaMonkey with the patch and notification bar. Lower half Firefox 52 with the doorhanger.

Comment on attachment 8872096 [details] [diff] [review] 1323494-use-notification-bar-for-geolocation.patch Halfway there :) The Firefox notification prompt for geolocation does not use the checkbox. With the patch I am loosing the ability to permanently forbid the geolocation prompting from the website. Never Share is no longer an option. The missing checkbox might also affect other notifications e.g. storage and not only geolocation but one step at a time (see /suite/common/src/nsSuiteGlue.js line 1450: case "desktop-notification"). Switching from a doorhanger to a notification bar would be ok for me but this needs a blessing from a higher up. I would suggest hiding the checkbox and add a Never Share button back. Please also test it in private browsing mode. IanN, would switching to a notification bar ok for you too? Not hat Isaac does it an it then gets shot down. f+ for now.

It seems to me that if you check "remember for this website" and hit "not for this request", it will deny all future geolocation requests for the domain, which should (if I'm not mistaken) mirror what the Never Share button does. (This can be reset in Data Manager on the Permissions tab.) You're right about Firefox 52 ESR not using the checkbox. In Firefox, if I don't put a breakpoint in it works, but if I put a breakpoint at line 1291 of PopupNotifications.jsm (using the Browser Toolbox) it fails with "notificationEl.checkbox is undefined". Possibly some sort of race condition?

This patch removes the checkbox from the notification bar and applies it to both the main window and the sidebar. For regular browsing, there are 3 buttons: always allow, don't allow for this request, and never allow. For private browsing, there are 2 buttons: allow for this request, and don't allow for this request.

Comment on attachment 8874840 [details] [diff] [review] 1323494-geolocation-notification-bar-3buttons.patch r+ and Big thanks. Tested with a 2.48 from the current release tree. en-US and de. The buttons were properly localized so I do not expect any problems here. Any chance you can revisit it for 249 and add the doorhanger back?

This is PopupNotifications.jsm, line 1291: try { notification.mainAction.callback.call(undefined, { checkboxChecked: notificationEl.checkbox.checked }); } catch (error) { Cu.reportError(error); } At this point, notificationEl.checkbox is undefined. If you put a breakpoint there (Firefox 52 ESR), it will throw an error. However, I think Firefox is optimizing something behind the scenes, because the callback method it's calling, in Firefox's case, is at PermissionsUI.jsm, line 300 - an arrow function that does not take any parameters - and if there's no breakpoints, it works fine. Unfortunately, I can't get SeaMonkey to work the same way. I tried changing the callback functions in suite/common/bindings/notification.xml from "function (aNotification)" to just "() =>", and I still get the same "notificationEl.checkbox" error in the browser console. This could be resolved by changing a line of code in mozilla-esr52. Perhaps it's worth reporting a bug there so they are aware of the issue, since this could wind up biting Firefox devs in the future.

"Never for this site" should come before "not now" for consistency with the offline storage prompt (see attached.) I'll make a new version of the patch later.

The only difference between this patch and the previous one is that two of the buttons have been switched (see previous comment.)

Comment on attachment 8878660 [details] [diff] [review] 1368277-geolocation-notification-bar-v3.patch Works as expected. r+

Comment on attachment 8878660 [details] [diff] [review] 1368277-geolocation-notification-bar-v3.patch r=me but, for a followup bug, looks like there is some common code between methods, is it possible to refactor?

Do you mean between the sidebar and main window? It should be possible, but ideally we could use the Firefox notifications for the main window, and I don't know if those would work in the sidebar.

(In reply to Isaac Schemm from comment #13) > Do you mean between the sidebar and main window? It should be possible, but > ideally we could use the Firefox notifications for the main window, and I > don't know if those would work in the sidebar. I was thinking some sort of shared helper, but not sure exactly how that would work yet. I would land this patch as is and worry about any other changes in a follow-up.

2.48 release branch: https://hg.mozilla.org/releases/comm-release/rev/ab1049e98f0ec11c740c434d52b5508dd1f94018

2.49.1 later. For current releses this should be fixed Bug 1323494

Fixed for comm-esr52 a+ from IanN for comm-esr52 over irc. https://hg.mozilla.org/releases/comm-esr52/rev/8daa8550f784a490b1e99e9020765e91efe103f3 We are now done here and will fix Bug 1323494 for the current releases. Thanks Isaac for the interim fix.