Skip to content

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

Merged
straker merged 3 commits intodequelabs:developfrom
kevin940726:fix/select-value
Sep 13, 2021
Merged

fix(nativeSelectValue): update selected value on change#3154
straker merged 3 commits intodequelabs:developfrom
kevin940726:fix/select-value

Conversation

@kevin940726
Copy link
Copy Markdown
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
Copy Markdown
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.

});
});

describe('props', function() {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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