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

[Combobox] Programmatically selecting a combobox item focuses the component #5540

Closed
macandcheese opened this issue Oct 21, 2022 · 16 comments · Fixed by #5774
Closed

[Combobox] Programmatically selecting a combobox item focuses the component #5540

macandcheese opened this issue Oct 21, 2022 · 16 comments · Fixed by #5774
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. a11y Issues related to Accessibility fixes or improvements. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. Calcite (design) Issues logged by Calcite designers. p - medium Issue is non core or affecting less that 60% of people using the library

Comments

@macandcheese
Copy link
Contributor

Actual Behavior

When programmatically selecting a combobox item, focus is moved to the Combobox. This differs from how other form components work, where the acted upon component does not gain focus.

Expected Behavior

I wouldn't expect the Combobox to gain focus when programmatically setting the selected value of contained combobox items.

Reproduction Sample

https://codepen.io/mac_and_cheese/pen/rNvXGgz?editors=1000

Reproduction Steps

  1. Open codepen
  2. Click the button to set selected on the combobox item.
  3. Notice that the combobox has received focus
  4. Repeat for input and slider - those components to not receive focus

Reproduction Version

95

Relevant Info

No response

Regression?

No response

Esri team

Calcite (design)

@macandcheese macandcheese added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Oct 21, 2022
@github-actions github-actions bot added the Calcite (design) Issues logged by Calcite designers. label Oct 21, 2022
@geospatialem geospatialem added the a11y Issues related to Accessibility fixes or improvements. label Oct 21, 2022
@alisonailea alisonailea added p - medium Issue is non core or affecting less that 60% of people using the library and removed needs triage Planning workflow - pending design/dev review. labels Nov 1, 2022
@driskull
Copy link
Member

driskull commented Nov 9, 2022

@alisonailea alisonailea self-assigned this Nov 10, 2022
@alisonailea alisonailea linked a pull request Nov 17, 2022 that will close this issue
1 task
@alisonailea alisonailea added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Nov 23, 2022
alisonailea added a commit that referenced this issue Nov 23, 2022
**Related Issue:** #5540 

## Summary

added a prop to combobox-item that will allow it to inform it's parent
if it was clicked or not.
The combobox parent no longer sets focus on it's text input whenever a
combobox-item is updated. Instead it only sets text input focus when
there is a keyboard interaction or the updated combobox-item is passed
with a clicked prop equal to _true_. This allows combobox-items to be
selected programmatically without changing the document.activeElement

- [x] feature or fix has a corresponding test
- [] changes have been tested with demo page in Edge (I still need to
get a VM setup)

Co-authored-by: Matt Driscoll <mdriscoll@esri.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben Elan <belan@esri.com>
@driskull
Copy link
Member

hey @alisonailea don't close the issue, just label it as installed so that it can get verified.

@driskull driskull reopened this Nov 23, 2022
@alisonailea alisonailea added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Nov 24, 2022
@github-actions github-actions bot assigned benelan and geospatialem and unassigned alisonailea Nov 24, 2022
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem
Copy link
Member

@alisonailea Can you speak to the skipped tests in filter, input-number, and input?

The fix works well with next.642, but want to make sure the skipped tests are expected.

@alisonailea alisonailea added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Dec 12, 2022
@github-actions github-actions bot assigned geospatialem and unassigned alisonailea Dec 12, 2022
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem
Copy link
Member

A good majority of the skip tests have been addressed via #5885, but there still seems to be a skipped test in filter.

@alisonailea It looks like there's one skipped test remaining in filter.e2e.ts per the discussion in https://github.com/Esri/calcite-components/pull/5774/files#r1030924370.

Reassigning to dev for final incorporation.

@geospatialem geospatialem added 2 - in development Issues that are actively being worked on. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Dec 13, 2022
@driskull
Copy link
Member

@alisonailea can you restore the last skipped test?

@alisonailea alisonailea added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Jan 17, 2023
@github-actions github-actions bot assigned geospatialem and unassigned alisonailea Jan 17, 2023
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem
Copy link
Member

A good majority of the skip tests have been addressed via #5885, but there still seems to be a skipped test in filter.

@alisonailea It looks like there's one skipped test remaining in filter.e2e.ts per the discussion in https://github.com/Esri/calcite-components/pull/5774/files#r1030924370.

Reassigning to dev for final incorporation.

@alisonailea Can the skipped tests be restored? Reassigning to dev for final incorporation.

@geospatialem geospatialem added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jan 18, 2023
@alisonailea
Copy link
Contributor

@geospatialem I just pulled it down and removed the .skip on filter.e2e to see what would happen and it is still failing. I don't know why. I didn't touch this code and this test was failing in main before I did any work.

calcite-filter › filter behavior › updates filtered items after filtering

    expect(received).toHaveLength(expected)

    Expected length: 1
    Received length: 4
    Received array:  [{"description": "developer", "metadata": {"favoriteBand": "MetallicA", "haircolor": "red"}, "name": "Harry", "value": "harry"}, {"description": "developer", "metadata": {"favoriteBand": "Radiohead", "haircolor": "black"}, "name": "Matt", "value": "matt"}, {"description": "developer", "metadata": {"favoriteBand": "The Mars Volta", "haircolor": "black"}, "name": "Franco", "value": "franco"}, {"description": "developer", "metadata": {"favoriteBand": "Hippity Hops", "haircolor": "brown"}, "name": "Jon", "value": "jon"}]

      121 |
      122 |   function assertMatchingItems(filtered: any[], values: string[]): void {
    > 123 |     expect(filtered).toHaveLength(values.length);
          |                      ^
      124 |     values.forEach((value) => expect(filtered.find((element) => element.value === value)).toBeDefined());
      125 |   }

@jcfranco
Copy link
Member

I didn't touch this code and this test was failing in main before I did any work.

Seems like unstable tests at play. @alisonailea Can you create a follow-up issue to restore the skipped tests?

@alisonailea
Copy link
Contributor

Follow up issue created: #6311

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Jan 19, 2023
@geospatialem
Copy link
Member

The original issue has been mitigated and verified. With the above follow-up to the filter unstable test, will close out this issue. 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. a11y Issues related to Accessibility fixes or improvements. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. Calcite (design) Issues logged by Calcite designers. p - medium Issue is non core or affecting less that 60% of people using the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants