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

Clean up any persistent borders when updated/uninstalled #61

Open
matatk opened this issue May 9, 2017 · 2 comments
Open

Clean up any persistent borders when updated/uninstalled #61

matatk opened this issue May 9, 2017 · 2 comments

Comments

@matatk
Copy link
Owner

matatk commented May 9, 2017

If the user reloads (or updates) the extension when persistent borders are active, then a stray border can be left on the page (until its landmark is highlighted again).

Whilst this probably won't adversely affect many users, it is best practice to clean up when necessary. The onSuspend event fires when an Event (not standard Background) page is going to be suspended, but it doesn't look like there's an event for when the whole extension is to be uninstalled—though a StackOverflow answer seems to imply it would be called on uninstall.

Either way, Firefox and Edge don't implement this yet, and Landmarks uses a Background page, not an Event page, plus it seems best to wait and see if a more apt event is added in future. The worst that can happen currently is that a duplicate border is sometimes drawn, but will be removed as the user navigates through the landmark again.

@matatk matatk self-assigned this May 9, 2017
@matatk matatk added this to the 2.2.0 milestone Nov 7, 2017
@matatk
Copy link
Owner Author

matatk commented Jan 9, 2018

Apparently, onSuspend is not the right way to go; there are other much more appropriate methods. Including using events to clean up any previous content scripts when a new one is loaded, which looks like a very nice method, and would also help with #131.

If the events-based solution works on Firefox, then it could be used as a general solution for both this issue and #131. If not, then it's probably best to solve them separately (for now, until some sort of standard event for "extension is about to be disabled/uninstalled" is provided).

@matatk
Copy link
Owner Author

matatk commented Jan 10, 2018

Think it's best to leave this for now; the behaviour between Firefox and Chrome is too different to solve generally, because what's needed is a generic callback/event to handle cleanup on extensions being disabled, but this doesn't exist in WebExtensions, and the port.onDisconnect() workaround won't work in Firefox (Firefox seems to have a cleaner approach to content scripts generally).

Haven't tried the events-based solution, but it isn't relevant here anyway, because we can't assume that a reload is taking place—we need to know when it's being unloaded regardless of whether it is being disabled, uninstalled or ultimately reloaded.

For reference:

@matatk matatk modified the milestones: 2.3.0, next May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant