Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only register the notification click handler once #267

Merged

Conversation

mrmr1993
Copy link
Contributor

@mrmr1993 mrmr1993 commented Mar 1, 2022

This PR replaces the handler for browser.notification.onClicked.addListener with a named function, which avoids re-registering the handler on every call to addHandlers.

Note that addEventListener called repeatedly with the same arguments will not register the same event listener multiple times (see the EventTarget.addEventListener spec for more info). This means that using the same named function for each call to addHandlers will only call that function once, where the previous code added a new anonymous function for each call.

This fixes #234, where clicking a notification would result in multiple tabs being opened, depending on the number of times that addHandlers was called.

This replaces the anonymous inline function used as the handler for
`browser.notification.onClicked.addListener` with a named function, to
avoid re-registering the same handler on every call to `addHandlers`.

Note that `addEventListener` called repeatedly with the same arguments
will not register the same event listener multiple times (see the
`EventTarget.addEventListener` spec[0] for more info). This means that
using the same named function for each call to `addHandlers` will only
call that function once, where the previous code added a new anonymous
function for each call.

This fixes sindresorhus#234, where clicking a notification would result in multiple
tabs being opened, depending on the number of times that `addHandlers`
was called

[0]:
https://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html#Events-EventTarget-addEventListener
@sindresorhus
Copy link
Owner

Linting is failing

Copy link
Collaborator

@notlmn notlmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for the fix (except for some nits).

source/background.js Outdated Show resolved Hide resolved
source/background.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@notlmn notlmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks again!

@notlmn notlmn merged commit 8b254f3 into sindresorhus:main Mar 2, 2022
@mrmr1993 mrmr1993 deleted the feature/single-register-notification-click branch March 2, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open 10 tabs when clicking notification in Mac OS Big Sur / Firefox
3 participants