search

pfn / passifox

Extensions to allow Chrome and Firefox (4.0+) to auto form-fill passwords from KeePass (requires KeePassHttp)
GNU General Public License v3.0
908 stars 186 forks source link

Use Zepto.js instead of jQuery where possible #670

Open mehrdadn opened 6 years ago

mehrdadn commented 6 years ago

Hi! This is an update to address the chromeIPass performance issue due to jQuery; it is intended to resolve issue #550.

It is somewhat ad-hoc and not well-written, but I think it should (?) work correctly. The idea is to use Zepto.js as a lightweight alternative to jQuery where possible, and load jQuery dynamically when input fields are actually detected. This prevents most subframes from loading jQuery, since generally few subframes have input fields.

I mainly tried to minimize the diff and avoid impacting the old code where possible (to make it easier to compare/understand), so that's why the code isn't very polished. I can fix the indentation issues, etc. if desired; it was a bit difficult to do this non-invasively since I would have had to re-indent and re-touch existing code as well.

One thing to note is that I also replaced the setInterval call with MutationObserver. I'm not sure whether the observe() call needs attributes: true instead of attribute: false. Please change this if needed. (I didn't set it lest there is a performance impact.)

It would be great if someone could also test this and ensure that:

  1. The performance is actually better.
  2. Nothing previously-working is broken (including when pages change dynamically).

Testing it is important as I've only done limited testing with it on one machine.

mehrdadn commented 6 years ago

Bump?

pfn commented 6 years ago

Thanks for the contribution, I've been heavily backed up on work, life and other projects. Sorry for the delay, I will try to address PRs in the future.

On Sat, Jan 13, 2018 at 1:07 AM mehrdadn notifications@github.com wrote:

Hi! This is an update to address the chromeIPass performance issue due to jQuery; it is intended to resolve issue #550 https://github.com/pfn/passifox/issues/550.

It is somewhat ad-hoc and not well-written, but I think it should (?) work correctly. The idea is to use Zepto.js as a lightweight alternative to jQuery where possible, and load jQuery dynamically when input fields are actually detected. This prevents most subframes from loading jQuery, since generally few subframes have input fields.

I mainly tried to minimize the diff and avoid impacting the old code where possible (to make it easier to compare/understand), so that's why the code isn't very polished. I can fix the indentation issues, etc. if desired; it was a bit difficult to do this non-invasively since I would have had to re-indent and re-touch existing code as well.

One thing to note is that I also replaced the setInterval call with MutationObserver. I'm not sure whether the observe() call needs attributes: true instead of attribute: false. Please change this if needed. (I didn't set it lest there is a performance impact.)

It would be great if someone could also test this and make sure the performance is actually better; I've only tested it on one machine.

You can view, comment on, or merge this pull request online at:

https://github.com/pfn/passifox/pull/670 Commit Summary

  • Use Zepto.js instead of jQuery where possible

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pfn/passifox/pull/670, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfQxWG6cL5ugi9kmuv_xR0gNtLbCCHSks5tKHI6gaJpZM4RdKIc .

mehrdadn commented 6 years ago

Actually... I think I've found bugs in this. :\ I'll try to update it when I get the chance (but it might be a while from now).

ForsakenHarmony commented 6 years ago

You can probably do it without either

mehrdadn commented 6 years ago

Update: I fixed the bug I had found (subframes were throwing errors). However, it's still not ideal. The problem is that chromeIPass makes the fundamentally incorrect assumption that all input fields become available together, and that they do so at a "nice" time. This isn't always the case (Javascript can add/remove fields to a form), and and when this assumption fails to hold, the password-generation "key" icon no longer auto-appears. Addressing this issue would seem to require quite a bit of surgery (essentially rewriting much of the extension), so I'm not sure I'll ever get the chance to address it.

Regarding jQuery vs. Zepto.js, Zepto.js might be avoidable with some care, but jQuery-UI is definitely required for the password-generation dialog UI; there's no getting around that, unless you write your own equivalent UI code.