Missing checkbox on SeaMonkey doorhanger notification breaks geolocation API

RESOLVED FIXED in seamonkey2.48

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: isaacschemm, Assigned: isaacschemm)

Tracking

({regression})

SeaMonkey 2.48 Branch
seamonkey2.48
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.48 fixed, seamonkey2.49esr fixed, seamonkey2.50 wontfix, seamonkey2.51 affected, seamonkey2.52 affected)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1323494
(Assignee)

Comment 1

2 years ago
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.
Attachment #8872094 - Flags: review?(frgrahl)
(Assignee)

Comment 2

2 years ago
Added a comment to the patch file.
Attachment #8872094 - Attachment is obsolete: true
Attachment #8872094 - Flags: review?(frgrahl)
Attachment #8872096 - Flags: review?(frgrahl)
Assignee: nobody → isaacschemm
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
Version: SeaMonkey 2.49 Branch → SeaMonkey 2.48 Branch
Blocks: SM2.48
Posted image Capture.PNG
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.
Attachment #8872096 - Flags: ui-review?(iann_bugzilla)
Attachment #8872096 - Flags: review?(frgrahl)
Attachment #8872096 - Flags: feedback+
(Assignee)

Comment 5

2 years ago
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?
(Assignee)

Comment 6

2 years ago
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.
Attachment #8872096 - Attachment is obsolete: true
Attachment #8872096 - Flags: ui-review?(iann_bugzilla)
Attachment #8874840 - Flags: ui-review?(iann_bugzilla)
Attachment #8874840 - Flags: review?(frgrahl)
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?
Attachment #8874840 - Flags: review?(frgrahl) → review+
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Comment 9

2 years ago
Posted image offline.PNG
"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.
(Assignee)

Comment 10

2 years ago
The only difference between this patch and the previous one is that two of the buttons have been switched (see previous comment.)
Attachment #8874840 - Attachment is obsolete: true
Attachment #8874840 - Flags: ui-review?(iann_bugzilla)
Attachment #8878660 - Flags: ui-review?(iann_bugzilla)
Attachment #8878660 - Flags: review?(frgrahl)
Comment on attachment 8878660 [details] [diff] [review]
1368277-geolocation-notification-bar-v3.patch

Works as expected. r+
Attachment #8878660 - Flags: review?(frgrahl) → review+

Comment 12

2 years ago
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?
Attachment #8878660 - Flags: ui-review?(iann_bugzilla) → ui-review+
(Assignee)

Comment 13

2 years ago
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.

Comment 14

2 years ago
(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.49.1 later. For current releses this should be fixed Bug 1323494
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.48
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.
You need to log in before you can comment on or make changes to this bug.