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

Don't close forms that are not empty ! #12

Open
GreggVan opened this issue Feb 23, 2021 · 7 comments
Open

Don't close forms that are not empty ! #12

GreggVan opened this issue Feb 23, 2021 · 7 comments
Labels
enhancement New feature or request

Comments

@GreggVan
Copy link

If a page has a form on it that is not empty - do not close it (throwing away all the data !)

thanks

@brcontainer
Copy link
Owner

@GreggVan it was not clear, but I will try to reproduce the problem. Grateful.

@marnovo
Copy link

marnovo commented Mar 3, 2021

+1 to the feature idea, but I'd like to add some extra info that may help on getting an implementation with less side effects.

A problem dear to many extensions as they evolve is that when they get features which rely on "checks" or "enhancements" over page elements, which then is done by tons of extensions on every page you have:

  1. The extension may require additional access to page content, which not everyone would like to provide, and ideally this only is required for/breaks this extra functionality.
  2. When doing such page checks and enhancements, the extensions may drive slowness and memory consumption, compounded: by interfacing/referencing page elements, the extension may be end up blocking garbage collection and releasing elements from memory, DOM nodes… and is not necessarily solved with plain reloading of the page. One example of what this could look like: DOM Nodes Not Being Released - Possible Memory Leak angular/angular#36590
  3. Pinned tabs, complex web apps and Google docs are particularly prone to it as they have lots of dynamic elements, inputs and stay open for a while.
  4. Offending extensions are not uncommon and may even be widely known and from commercial products and large companies. I found Google itself and password managers to be some of the them.
  5. It is not trivial to prevent an offending extension not to run over a certain page: browsers themselves usually provide native site access whitelisting via URLs, but not blacklisting. So it falls into the extension developer to debug and fix the issue (not trivial), implement some sensible blacklisting, or very custom and convoluted "managed devices" policy implementations.
  6. I found many extensions driving such behavior out when "debugging" memory leaks & CPU usage, and it is hard to nail down which extensions do it and to which pages, but once you do find them out and limit access to all pages, or even disable them completely, it's night and day.

Now, for a potential solution to implement the feature and avoid those pitfalls: preventing tab closure based on the widely supported browser native handling of document unload and hide events. In short, when hiding and/or unloading a document (tab closing, reloading, etc.) the browser goes through a loop where these can be paused or even cancelled; certain actions may be triggered, as auto-saving or user prompts "Changes you made may not be saved". So it's more coherent for users with native browser behavior, and may not need to read page elements or implement some input/form parsing.

Some references:

@GreggVan
Copy link
Author

GreggVan commented Mar 3, 2021 via email

@brcontainer brcontainer added the enhancement New feature or request label Mar 3, 2021
@brcontainer
Copy link
Owner

I'm already doing experiments, but I believe that some fake forms (those that are not real forms, are solved via JavaScript only) will probably not be simple. Anyway, this will probably be a "beta" feature and I will make it available as soon as possible.

@dezza
Copy link

dezza commented Sep 9, 2022

This is essential. Maybe you can take inspiration from duplicate-tabs-closer which does this, but has other issues which I can't remember off my mind.. but thats why I uninstalled it in the first place (unmaintained anyways).

Permissions to me, is a non-issue. In Firefox I use tons of advanced extensions with extensive permissions, I look at the source if in doubt. Same thing with phones, permissions are often interpreted as bad by everyone, but they have their legitimate uses.

From looking at your source it seems like you're rebuilding or sorting the whole tab collection? No? Is this why its reloading?

I'm not a javascript developer by far.. but I would love this functionality (I might attempt a PR).

@brcontainer
Copy link
Owner

brcontainer commented Apr 6, 2023

@GreggVan @dezza @marnovo

Forgive my absence, these last few years have been complicated for everyone.

I recently returned to work on the addon, and I thought of a way that won't depend on hacks, which is actually very simple, using content_scripts and Event.isTrusted, the script sends a signal to the addon (it's a pretty basic example, I'll improve it, it's just illustrative):

var userFill = false;

function detectUserFill(e) {
    if (userFill || !e.isTrusted) return;

    userFill = true;

    chrome.runtime.sendMessage(null, 'form:filled', {});
}

addEventListener('input', detectUserFill, { capture: true });

And through the "sender" I get the tabId and add it to a temporary exception list in background.js:

var expcetionList = [];

chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {
    if (request === 'form:filled') expcetionList.push(sender.tab.id);
});

That way we won't need to inject "beforeunload" into the pages.

Unfortunately it exists in a Firefox bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1810910), forcing me to postpone the migration to Manifest V3. The bug doesn't occur in MV2, so I made a polyfill that will keep the compatibility between V2 and V3, when the bug is fixed I will migrate.

Anyway it is still allowed to publish as MV2, so until next week I will have finished the tasks (#1) and will publish them in the "stores".

@GreggVan
Copy link
Author

GreggVan commented Apr 6, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants