fix(combobox): allow numeric values and trigger change event on keybo… #4405
Conversation
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Tachometer resultsChromecombobox permalinkbasic-test
light-dom-test permalink
Firefoxcombobox permalinkbasic-test
light-dom-test permalink
|
packages/combobox/src/Combobox.ts
Outdated
| @@ -211,6 +214,7 @@ export class Combobox extends Textfield { | |||
| return; | |||
| } | |||
| this.value = this.activeDescendant.itemText; | |||
| this.handleChange(); | |||
There was a problem hiding this comment.
It looks like selectDescendant() is only used when the Enter key is pressed. I wonder if we can get the same, but slightly better, simply by this.activeDescendant.click(). In theory, this would trigger the lifecycle of the <sp-menu> and hit all of the appropriate change, etc. pathways ensuing all selections were "the same".
There was a problem hiding this comment.
this.activeDescendant.click() does not really work, since it is defined as MenuItem | ComboboxOption, but what can be done and works is similar to what's inside scrollToActiveDescendant, but instead of scroll you click.
| @@ -354,4 +354,31 @@ describe('Combobox accessibility', () => { | |||
|
|
|||
| expect(el.open).to.be.false; | |||
| }); | |||
| it('manages keyboard navigation and selection', async () => { | |||
There was a problem hiding this comment.
Can we include testing for numeric values, too?
There was a problem hiding this comment.
Sure! 😄 More than what I already added in packages/combobox/test/combobox.data.test.ts?
Or a combination of numeric values + selection testing?
There was a problem hiding this comment.
Oh, somehow I didn't see that test. I think we just need to press Enter on something there to confirm it's fully hit the getElementById() path, and then this will be good to go!
There was a problem hiding this comment.
I combined them into one test since it was 90% the same, just the data was different. 🙈
| const activeEl = this.shadowRoot.getElementById( | ||
| this.activeDescendant.value | ||
| ); | ||
| if (activeEl) { | ||
| activeEl.click(); | ||
| } |
There was a problem hiding this comment.
Does this change also trigger the code in line 216 to happen in handleInput, as well?
There was a problem hiding this comment.
Ah indeed.
It does not trigger handleInput, buuuut it does trigger handleMenuChange which does this.value = selected?.itemText || ''; which is same thing as line 216.
Westbrook
force-pushed
the
rocss/numeric-combobox-fix
branch
from
9d59470 to
8af69ea
Compare
#4405) * fix(combobox): allow numeric values and trigger change event on keyboard selection * chore: call click method to trigger change * chore: update tests --------- Co-authored-by: rmanole <rmanole@adobe.com>
…ard selection
Description
This PR aims to fix two issues when navigating in the Combobox using the keyboard.
Related issue(s)
Motivation and context
changeevent is not triggered, as it is triggered on mouse interactions.How has this been tested?
changeevent is triggered,event.target.valueis55Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.