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

Fixes issue #5603 #5604

Merged
merged 6 commits into from
Feb 25, 2022
Merged

Conversation

reejuBhattacharya
Copy link
Contributor

Resolves #5603

####Screenshots of the change:

The issue, as illustrated below, is whenever we cycled through the radio button selections in the code, the checked=true option is not removed after each subsequent change. Therefore, after one cycle, all options have checked=true and the program malfunctions.

before

The expected behavior:

after

PR Checklist

  • npm run lint passes

Acknowledgement: Credit goes to @sflanker for solving the problem in the issue itself, I simply implemented it selectively in the source code.

@reejuBhattacharya reejuBhattacharya changed the title Issue radiobutton Fixes issue #5603 Feb 16, 2022
@sflanker
Copy link
Contributor

It turns out that browser actually track the checked state for radio button inputs using a JavaScript property separately/in addition to the checked="true" attribute 🤦‍♂️. So even with the fix here things will be broken if the user interactively changes the selection. Update:

        // When clearing the state
        self._getOptionsArray().forEach(option => {
          option.checked = false;
          option.removeAttribute("checked");
        });
            // When setting the selection
            option.setAttribute("checked", true);
            option.checked = true;

@reejuBhattacharya
Copy link
Contributor Author

Oh ok...So, as I understand it, we will have to update the JavaScript using option.checked and simultaneously update the HTML using the setAttribute and removeAttribute functions.....

Ok, making the changes as you mentioned pronto. Thanks for the update!

@limzykenneth
Copy link
Member

From what I can see the issue seems to be fixed now with this PR. @sflanker Would you like to double check this before I merge?

@sflanker
Copy link
Contributor

The change looks good to me.

@limzykenneth limzykenneth merged commit e46fa4d into processing:main Feb 25, 2022
@limzykenneth
Copy link
Member

Looks good. Thanks!

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