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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Clear the hidden input elements on the clear action. #14326

Merged
merged 1 commit into from Apr 2, 2018

Conversation

cvializ
Copy link
Contributor

@cvializ cvializ commented Mar 30, 2018

Fixes #14308

amp-selector creates hidden inputs so that it behaves like a radiobutton or checkbox group in a form. When selecting normally these are created and destroyed as the user selects and unselects options. Using the clear AMP action to unselect all the options would not remove these hidden inputs and the selector would incorrectly submit out-of-sync data in a form. This PR removes the hidden inputs on the clear AMP action and tests that no hidden inputs remain afterward.

@cathyxz
Copy link
Contributor

cathyxz commented Apr 2, 2018

This looks good. So apparently the setInputs_ function is used to set the selection each time a user selects an option. It removes all the options one by one and recreates them. That seems to be a bit of overkill. git blames to #5975, but unfortunately I can't decipher why it was implemented this way. Maybe it's possible to make this more efficient.

@cvializ
Copy link
Contributor Author

cvializ commented Apr 2, 2018

Thanks for the review! I opened a Discussion/Question issue to track possible improvements for the hidden inputs. I'll merge this now to fix the behavior.

@cvializ cvializ merged commit f5b155b into ampproject:master Apr 2, 2018
This was referenced Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants