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

fix(nativeSelectValue): update selected value on change #3154

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

kevin940726
Copy link
Contributor

nativeSelectValue doesn't return the selected value after manual selection. This PR fixes that by using the selected on <option> to get the up-to-date value.

@kevin940726 kevin940726 requested a review from a team as a code owner September 8, 2021 08:36
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. That looks better. One last thing, could you add a test to https://github.com/dequelabs/axe-core/blob/develop/test/core/base/virtual-node/virtual-node.js? I know we don't have tests for the node specific props (multiple, nodeValue, value, etc.), but I think that might be a failing on our part.

The test can be as simple "node with selected property is reflected in props" or something, just so we don't delete it one day without knowing.

@@ -97,6 +107,32 @@ describe('VirtualNode', function() {
});
});

describe('props', function() {
Copy link
Contributor

@straker straker Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to add these tests? They look like copy/paste from the previous describe('attr')

Copy link
Contributor Author

@kevin940726 kevin940726 Sep 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Deleted!

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks for fixing this.

@straker
Copy link
Contributor

straker commented Sep 13, 2021

Reviewed for security

@straker straker changed the title Fix nativeSelectValue not picking up selection fix(nativeSelectValue): update selected value on change Sep 13, 2021
@straker straker merged commit 1ee88cb into dequelabs:develop Sep 13, 2021
straker pushed a commit that referenced this pull request Oct 18, 2021
* Fix nativeSelectValue not picking up selection

* Use props instead

* Add test for selected prop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants