fix(Search): fix event passed to onChange when clearing input (#12133)

[gi]

• fix(Search): when clearing, pass a pseudo input change event instead of
  the mouse click event with which the internal event handler is called.

Closes #12133

When the close button is clicked, the internal event handler is called with the mouse click event.

Previously, the onChange prop was called with this event or a modified version. However, that event callback normally expects an input change event.

This fixes the issue by creating and sending a pseudo input change event to the onChange prop.

Changelog

Changed

• Search: onChange is called with proper data shape when clearing input

Testing / Reviewing

• Verified in Storybook
• Added unit tests.

[github-actions]

DCO Assistant Lite bot All contributors have signed the DCO.

[netlify]

✅ Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	5365473
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-components-react/deploys/633c5dd58ef1e70008493d59
😎 Deploy Preview	https://deploy-preview-12140--carbon-components-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[gi]

I have read the DCO document and I hereby sign the DCO.

[netlify]

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	5365473
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/633c5dd52ef6160009aa033b
😎 Deploy Preview	https://deploy-preview-12140--carbon-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[tay1orjones]

Thanks for noticing this and putting up a PR! This is a quite old snippet of code - 5+ years at this point. I think the original intent of this code was probably to replace the existing event.target.value with an empty string.

 function clearInput(event) {
   if (!value) {
     inputRef.current.value = '';
     onChange(event);
   } else {
-    const clearedEvt = Object.assign({}, event.target, {
+    const clearedEvt = Object.assign({}, event, {
       target: {
         value: '',
       },
     });
     onChange(clearedEvt);
   }

   onClear();
   setHasContent(false);
   focus(inputRef);
 }

This would be the smallest change to rectify the root issue and I think the least likely to introduce a breaking change. I'd prefer not to change the signature and call structure if possible, or at least as little as possible, as these could inadvertently break folks who have relied on this since v8/v9.

[tay1orjones]

Should this be in a beforeEach or beforeAll?

[gi]

It could be a beforeAll, but that would only be doing the same thing as this but with less clarity: the functions would need to be declared as let variables and then assigned in the before routine.

This code solves for an issue that I see a lot with front-end tests and which I see with the tests in this suite: if you render the element under test in a beforeEach routine, then it makes any other setup in nested contexts very difficult. You end up with multiple renders of the element.

In fact, I believe that some of the tests in this suite are finding the wrong element rendered.

If you move the mounting or rendering into each test/it, then you avoid this issue.

To avoid duplicating the possibly complex mount/render declaration, I have adopted this paradigm which works quite well in my experience.

Each nested context/describe can build on the props:

• default: props reset
  • with size large: props.large = true
  • with size small: props.small = true
• with onChange: props.onChange = jest.fn();
• etc.

Each test simply becomes

render(props);
expect(...);

[gi]

@tay1orjones The actual culprit is the first if block:

if (!value) {
  inputRef.current.value = '';
  onChange(event);
}

This clears the input's value, but it sends the click event from the button, which does not contain the target.value:

• event: mouse click event
• event.target: button element
• event.target.value: undefined

Additionally, the else clause was sending an "event" such that the event data was really the target button element and the "target" was this manufactured object:

else {
  const clearedEvt = Object.assign({}, event.target, {
    target: {
      value: '',
    },
  });
  onChange(clearedEvt);
}

• clearedEvt: button element not an event
• clearedEvt.target: { target: { value: '' } }

In both cases from this clearInput handler, the onChange callback does not receive the same type of event or target which it otherwise receives from the normal change events when the user inputs text.

Alternatively, this could be:

    if (!value) {
      inputRef.current.value = '';
    }

    const inputTarget = Object.assign({}, inputRef.current, { value: '' });
    const clearedEvt = { target: inputTarget, type: 'change' };

[tay1orjones]

That makes sense, thanks!