fix(combobox): clear custom input value on blur #7956

anveshmekala · 2023-10-06T14:38:17Z

Related Issue: #2162

Summary

Clears input value on blur if the value has no matching combobox-item when allowCustomValue property is set to false

github-actions · 2023-10-14T01:43:20Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

…on-blur

alisonailea · 2023-10-24T21:36:52Z

packages/calcite-components/src/components/combobox/combobox.tsx

@@ -298,15 +298,19 @@ export class Combobox
      return;
    }

-    this.setInactiveIfNotContained(event);
+    const composedPath = event.composedPath();
+    if (!this.open || composedPath.includes(this.el) || composedPath.includes(this.referenceEl)) {


Can you add a test that explicitly checks that the custom input value doesn't clear when the compose path includes this.el?

we do have a test for this one currently. Any item selection test covers this usecase.

jcfranco

I have some concerns regarding the changes to the disabled test helper. Other than that, this is looking good. 😎👍🖥

jcfranco · 2023-10-25T04:18:33Z

packages/calcite-components/src/components/combobox-item/combobox-item.tsx

@@ -160,7 +160,7 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
    this.selected = !this.selected;
  }

-  itemClickHandler = (event: MouseEvent): void => {
+  itemPointerHandler = (event: PointerEvent): void => {
    event.preventDefault();


Can you confirm the event still needs to be canceled here? I strongly suspect it is no longer needed because:

It was added in a very early stage of the component

In this early iteration, it was set on an <a>

Not sure why it was canceled in 2 since href was set to #, which won't redirect – maybe it was an extra step to prevent redirects (canceling click stops <a>'s default behavior)

This PR uses pointerdown (vs click) and canceling it will prevent mousedown and mouseup from firing (this was not the case when click was canceled)

The pointer event is set on <li>, which does not have any default click behavior (unlike <a>)

If not needed, it would help us avoid adding pointerEventPreventDefaultOnClick to the disabled test helper options.

The reason for using event.preventDefault in this scenario is to avoid emitting compatibility mouse

Without preventDefault mouse compatibility events will be fired thus causing blur on input which results in loosing focus.

Preventing mouse events wouldn't cause any issues since we don't have any listeners or handlers for mouse events in combobox-item and combobox.

Though <li> element does not have any default click behavior , the preventDefault is used to prevent mouse and focus related events.

jcfranco · 2023-10-25T04:24:30Z

packages/calcite-components/src/tests/commonTests.ts

+   *  Note: mousedown and mouseup events are skipped when pointerEvent has preventDefault
+   *  https://github.com/web-platform-tests/wpt/blob/master/pointerevents/compat/pointerevent_mouse-pointer-preventdefault.html#L54
+   */
+  pointerEventPreventDefaultOnClick?: boolean;


This argument is for a very particular use case and I'm not sure about its wider applicability.

Yup, it is a specific use case . Added this to the options so that we can extend this if required to any component in future. pointerDownEventDefaultPrevented might be a better name?

jcfranco · 2023-10-25T04:26:58Z

packages/calcite-components/src/components/combobox-item/combobox-item.tsx

@@ -160,7 +160,7 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
    this.selected = !this.selected;
  }

-  itemClickHandler = (event: MouseEvent): void => {
+  itemPointerHandler = (event: PointerEvent): void => {


Can you rename this to better match the particular event it's handling?

jcfranco · 2023-10-25T04:29:42Z

packages/calcite-components/src/components/combobox/combobox.e2e.ts

@@ -1097,7 +1097,7 @@ describe("calcite-combobox", () => {
      await page.waitForChanges();
      await page.keyboard.press("Tab");
      await page.waitForChanges();
-      expect(await combobox.getProperty("value")).toBe("");
+      expect(await inputEl.getProperty("value")).toBe("");


Great catch, but can we submit these renames separately? There are no other changes in this file related to the PR, so it's out of scope.

These changes are related i think. This helps in asserting the input clear on blur functionality via keyboard & Mouse. Though these tests were added earlier as part of #7721 , they couldn't identify regression when #7954 is installed . Made changes to this test so that they can detect any regressions from now on.

anveshmekala · 2023-10-27T19:19:59Z

Closed in favor #8070

fix(combobox): clear custom input value on blur

b7d16ed

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Oct 6, 2023

github-actions bot assigned anveshmekala Oct 6, 2023

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 14, 2023

replace click handler with pointer in combobox-item

fd2e550

anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Oct 16, 2023

anveshmekala added 5 commits October 16, 2023 12:18

clear input on blur

7f82bbe

fix resetting filtered items list

94b5a32

refactor

9e9b63f

refactor disabled test util to accommadate preventDefault

24ab22d

Merge branch 'main' into anveshmekala/2162-combobox-clear-user-input-…

cb8ced9

…on-blur

anveshmekala added the low risk Issues with low risk for consideration in low risk milestones label Oct 24, 2023

Merge branch 'main' into anveshmekala/2162-combobox-clear-user-input-…

22365e1

…on-blur

anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 24, 2023

anveshmekala marked this pull request as ready for review October 24, 2023 21:06

anveshmekala requested a review from a team as a code owner October 24, 2023 21:06

alisonailea requested changes Oct 24, 2023

View reviewed changes

jcfranco requested changes Oct 25, 2023

View reviewed changes

refactor tests

d766d8e

anveshmekala marked this pull request as draft October 27, 2023 16:22

anveshmekala closed this Oct 27, 2023

anveshmekala deleted the anveshmekala/2162-combobox-clear-user-input-on-blur branch February 28, 2024 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(combobox): clear custom input value on blur #7956

fix(combobox): clear custom input value on blur #7956

anveshmekala commented Oct 6, 2023 •

edited

Loading

github-actions bot commented Oct 14, 2023

alisonailea Oct 24, 2023

anveshmekala Oct 24, 2023

jcfranco left a comment

jcfranco Oct 25, 2023

anveshmekala Oct 25, 2023

jcfranco Oct 25, 2023

anveshmekala Oct 25, 2023

jcfranco Oct 25, 2023

jcfranco Oct 25, 2023

anveshmekala Oct 25, 2023

anveshmekala commented Oct 27, 2023 •

edited

Loading

fix(combobox): clear custom input value on blur #7956

fix(combobox): clear custom input value on blur #7956

Conversation

anveshmekala commented Oct 6, 2023 • edited Loading

Summary

github-actions bot commented Oct 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anveshmekala commented Oct 27, 2023 • edited Loading

anveshmekala commented Oct 6, 2023 •

edited

Loading

anveshmekala commented Oct 27, 2023 •

edited

Loading