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

DOM shouldn't be changed by the extension #9

Closed
dylankenneally opened this issue Nov 21, 2022 · 9 comments · Fixed by #32
Closed

DOM shouldn't be changed by the extension #9

dylankenneally opened this issue Nov 21, 2022 · 9 comments · Fixed by #32

Comments

@dylankenneally
Copy link

Issue

When I click on an email field, the extension injects items into the DOM & thus fiddles with a site's rendered output. Whilst the injected items are helpful & form a big part of the extension's functionality, it's inappropriate for the DOM to be updated by the extension in this way, as it can lead to odd effects happening for the site being visited & shouldn't happen.

Furthermore, it presumes that users will want a new hidden email address for each email field, but this is not the case.

Steps to replicate

  1. Installed & enabled the extension
  2. Visit any site/page with an email form field, e.g. https://github.com/settings/emails, and note the rendered output:
    • Add email address
  3. Click the email field and note the rendered output
    • Add email address

Expected/desired outcome

  • Add the access point for the functionality to the context menu for the email field instead
  • image

Platform details

Hardware: MacBook Pro (Retina, 15-inch, Mid 2015)
OS: macOS Monterey, v12.6
Browser: Google Chrome, Version 107.0.5304.110 (Official Build) (x86_64)
Extension: iCloud Hide My Email, v1.0.0

Side notes

  • Thank you for the extension, it's very helpful
  • If you agree with the issue & expected outcome, I'd be happy to take the issue on myself.
@dedoussis
Copy link
Owner

dedoussis commented Nov 26, 2022

Hey @dylankenneally! Thanks for this feedback and for taking the time to put such a detailed issue together.

I agree with your take. At the moment the extension modifies the DOM in a pretty aggressive manner.

Originally, I took the inspiration for this functionality from the 1Password extension. 1Password also modifies the DOM, but in a much more discrete way. It injects an iframe as the last child of the body element which loads a static html file that is baked into the extension. Unfortunately, I was lacking the time to implement such a sophisticated DOM modification, leading to the current suboptimal result.

I do think that your context menu idea is excellent though and I'd love to see it introduced in this extension. However, rather than sunsetting the current DOM mutating functionality I'd rather keep both and let the user pick their favourite (or even both) through the existing Options page of the extension.

Feel free to take this yourself. Send any questions you may have my way (I'll try to be more swift with my responses).

@dylankenneally
Copy link
Author

Letting users pick which UI scheme they want sounds like a good way to go. It's probably a good idea to leave the existing UI as as the default choice too - otherwise existing users that get auto-updated in Chrome could be left scratching their heads.

I'm happy to take this on, I'll send a PR over when it's ready.

@he3als
Copy link

he3als commented May 2, 2023

Any updates on this? No rush, just curious

@dylankenneally
Copy link
Author

I've had zero time to look at it @he3als - it's on my list, but that list isn't getting any shorter.

If someone else wants to take it on, that's cool, as I can't say when this will make it to the top of my todo list.

@dedoussis
Copy link
Owner

dedoussis commented May 29, 2023

Started working on this: #32

Will finish it at some point next week.

@dedoussis dedoussis linked a pull request May 29, 2023 that will close this issue
@dedoussis dedoussis reopened this Jun 18, 2023
@dedoussis
Copy link
Owner

This change ended up being more complex than what I was expecting. Have merged the PR.

I won't publish it to the web store right away though. I want to spend a few days using the new version on my machine to make sure that I haven't introduced any regression. You can get a sneak preview of the new functionality from the screenshots in the updated README (main branch).

Will close this as soon as I publish the new version to the chrome and firefox webstores.

@dedoussis
Copy link
Owner

Have submitted the new version to both of the Chrome and Firefox stores. The Firefox one has already been accepted. Might take a few days to be accepted in Chrome. Thanks everyone for your feedback on this one; closing.

@dedoussis-stripe
Copy link

Has also been published in the chrome web store. Let me know if you spot any issues.

@dylankenneally
Copy link
Author

@dedoussis - got the latest update, it works a treat

Really appreciate the work you've put in to this, next time I'm in London (my old home town), I'll have to buy you a beer to say thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants