admin ui: fix checkbox toggles not working #374
Conversation
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
@nickvergessen @luka-nextcloud Pinging because the most recent PRs were handled by you two. Do you have a moment to spare to check this simple patch resolving a commonly encountered issue for end-users? |
|
I'm not affiliated with the app and just send automated pull requests. But I will try to forward the ping. |
|
Hi @vermeeren apologies for the situation, very sorry nobody reviewed the PR, this shouldn't happen and you should get feedback on your contribution. I'll get in touch with @luka-nextcloud and @blizzz to get this going 👍 |
luka-nextcloud
force-pushed
the
fix-admin-ui-checkboxes
branch
from
952d170 to
ef830a7
Compare
|
@vermeeren Thanks for your contribution. Sorry for the late review from my side. Could you please commit again with signoff so that the DCO check doesn't complain? |
Since jQuery 1.6 the attr() and prop() functions more strictly follow
W3C forms specification. Concretely the following is the current
functionality without going to deep into details:
* attr('checked') will actually check defaultChecked, which is the
initial state of the checkbox upon page load.
* prop('checked') matches the current state of the checkbox. It will
change state when the user checks or unchecks the checkbox.
https://api.jquery.com/attr/
https://api.jquery.com/prop/
Concretely current implementation is broken since probably jQuery 1.6 as
observed in devtools requests. The requests when toggling checkboxes
are always sending the on-page-load current value. So if a setting is on
all toggles of the checkbox will always turn the setting on server-side,
making it impossible to actually change the setting.
Switch to prop() function to resolve the issue.
Fixes nextcloud#121
Fixes nextcloud#166
Fixes nextcloud#305
Signed-off-by: Melvin Vermeeren <[email protected]>
vermeeren
force-pushed
the
fix-admin-ui-checkboxes
branch
from
ef830a7 to
25c2ae1
Compare
|
@luka-nextcloud Done, thanks for checking! |
|
@luka-nextcloud you might want to backport this to all affected versions |
|
/backport to stable31 |
|
/backport to stable32 |
4 participants
Since jQuery 1.6 the attr() and prop() functions more strictly follow W3C forms specification. Concretely the following is the current functionality without going to deep into details:
https://api.jquery.com/attr/
https://api.jquery.com/prop/
Concretely current implementation is broken since probably jQuery 1.6 as observed in devtools requests. The requests when toggling checkboxes are always sending the on-page-load current value. So if a setting is on all toggles of the checkbox will always turn the setting on server-side, making it impossible to actually change the setting.
Switch to prop() function to resolve the issue.
Fixes #121 Fixes #166 Fixes #305