Remove jQuery UI from form-mini.js #14273
Conversation
|
Could you clarify what is benefit of getting rid of jquery ui specially in this place? AFAIK jquery UI is using in a lot of places, so removing it just from one place - do not changing anything. Not sure if it's reasonable to accept such pull request. Actually previous implementation was much better because there were using constants |
|
@ihor-sviziev the idea is to burn down the implementation areas of jQuery UI. Given the jQuery UI version is old and insecure. The jQuery UI version is also modified making it hard to upgrade.
Seems fair, I mean it might make sense to have a lightweight jQuery plugin which just handles keydown events etc, however I didn't spot many other areas that would use it. |
|
Minus TinyMCE and some other libraries it looks like it could be removed in another PR in the front-end. I could expand this commit to do that? The only thing I am uncertain of is the use of |
There was a problem hiding this comment.
Currently, we can't use event.key without the fallback to the old approach because we supporting IE11 and Edge browsers and there aren't fully support event.key.
https://caniuse.com/#feat=keyboardevent-key
Also, if you want you can read more about it https://medium.com/@uistephen/keyboardevent-key-for-cross-browser-key-press-check-61dbad0a067a
Damn sounds like MDN docs are wrong on the support for this sorry. Anyway I still think this would be easier to copy out the jQuery UI behaviour for keypresses into a smaller lib as the only other potential jQuery UI on the front end I have found is the calendar meaning in a follow up we could remove it as a FE dependency. Does that sound like an ok solution @VladimirZaets? |
While me personally would love to get rid of jQuery/jQuery UI completely (in favor of Angular, but it's not gonna happen :D), how is it even possible considering http://devdocs.magento.com/guides/v2.2/javascript-dev-guide/widgets/jquery-widgets-about.html? So, to reduce jQuery UI usages some common strategy should be agreed first which then should be concretized in particular action points.
Are there any attack vectors you know of actually affecting Magento?
Having backward compatibility in mind, this seems the only viable approach though. |
Not at the moment, however having very old versions is never good. I wasn't raising this as a vuln or anything, just that there might be attack vectors. I also haven't tried to look into the exploits in UI and match them to Magento.
I think at this point if the front end can have the code removed then this will help update or removal. I'm also not 100% on how you handle backwards compatibility either.
Only the following appear to use UI though:
Also "Magento out of the box does not contain jQuery UI styles", so it really does seem like these small number of references that could be burned down in a few PRs.
Yeah we have @dmanners at the office today, he said we should have a conversation about architectural changes with the relevant parties :) |
My main concern is http://devdocs.magento.com/guides/v2.0/coding-standards/code-standard-jquery-widgets.html, there are almost 300 occurrences of
Great! |
jonathanKingston
force-pushed
the
2.3-develop_remove-jquery-ui-from-form-mini
branch
from
2b161f8 to
1858391
Compare
Yeah I think we can migrate that to something simpler too. @orlangur I updated the PR to include a function from an external file, I moved another usage in a different file also. |
|
@VladimirZaets please take a look on this one. |
It's make sense. But we already have small library that transform keycodes to understandable type. So, we can use this library in current case, but we shouldnt create new one library with the similiar behaviour |
|
I'm not working on this any longer, I'm just going to close my PRs. Feel free to adopt the code after a fix-up etc. |
Description
Removal of jQuery UI from the form search as it's a dependency that isn't needed.
Contribution checklist