Repository: Sage/carbon (ts)

Issue:
Multi Select onChange is called twice when a list item is selected, also when object set as value it cannot be edited
### Current behaviour

When an item is selected from the Multi Select option list, two `onChange` callbacks are made.

Also, when an object is set as a value the options cannot be edited.

### Expected behaviour

Only one `onChange` callback should be made when an option is selected.

When an object is set as a value, it should be possible to delete or add more options.

### CodeSandbox or Storybook URL

https://carbon.sage.com/?path=/docs/select-multiselect--with-object-as-value

### JIRA Ticket (Sage Only)

FE-5332

### Suggested Solution

The `onChange` callback twice issue looks to be because `onChange` and `updateValue` are both called in the `onSelectOption` function.

The object as value issue looks to be caused by the `onSelectOption` function too, so both issues seem related to this.

### Carbon Version

110.1.1

### Design Tokens Version

2.40.0

### What browsers are you seeing the problem on?

Firefox, Chrome, Safari, Microsoft Edge

### What Operating System are you seeing the problem on?

MacOS

### Anything else we should know?

_No response_

### Confidentiality

- [x] I confirm there is no confidential or commercially sensitive information included.

Pull request:
fix(multi-select): fix onchange callback being called twice on option select - FE-5332
### Proposed behaviour


Fixes #5418

### Checklist

<!-- Each PR should include the following -->

- [x] Commits follow our style guide
- [x] Related issues linked in commit messages if required
- [x] Screenshots are included in the PR if useful
- [x] All themes are supported if required
- [x] Unit tests added or updated if required
- [x] Cypress automation tests added or updated if required
- [x] Storybook added or updated if required
- [x] Translations added or updated (including creating or amending translation keys table in storybook) if required
- [x] Typescript `d.ts` file added or updated if required
- [x] Related docs have been updated if required

#### QA

- [x] Tested in CodeSandbox/storybook
- [x] Add new Cypress test coverage if required
- [x] Carbon implementation matches Design System/designs
- [x] UI Tests GitHub check reviewed if required

### Additional context

<!-- Add any other context or links about the pull request here. -->

### Testing instructions
https://codesandbox.io/s/carbon-quickstart-forked-zxih4h
In the provided sandbox select a new value in the MultiSelect component and check console to see how many times the `onChange handler fired` text is logged

Patch:
diff --git a/src/components/select/multi-select/multi-select.component.js b/src/components/select/multi-select/multi-select.component.js
index d939d1933..c0534616b 100644
--- a/src/components/select/multi-select/multi-select.component.js
+++ b/src/components/select/multi-select/multi-select.component.js
@@ -442,10 +442,6 @@ const MultiSelect = React.forwardRef(
         const isAlreadySelected =
           actualValue.findIndex((val) => isExpectedValue(val, newValue)) !== -1;
 
-        if (!isAlreadySelected && isControlled.current && onChange) {
-          onChange(createCustomEvent([...actualValue, newValue]));
-        }
-
         textboxRef.focus();
         isMouseDownReported.current = false;
 
@@ -457,7 +453,7 @@ const MultiSelect = React.forwardRef(
           return [...previousValue, newValue];
         });
       },
-      [createCustomEvent, onChange, textboxRef, actualValue, updateValue]
+      [textboxRef, actualValue, updateValue]
     );
 
     function onSelectListClose() {
diff --git a/src/components/select/multi-select/multi-select.spec.js b/src/components/select/multi-select/multi-select.spec.js
index 66866f45c..21519f8e3 100644
--- a/src/components/select/multi-select/multi-select.spec.js
+++ b/src/components/select/multi-select/multi-select.spec.js
@@ -851,7 +851,7 @@ describe("MultiSelect", () => {
     };
 
     describe("and an option is selected", () => {
-      it("then the onChange prop should be called with expected value", () => {
+      it("then the onChange prop should be called once with expected value", () => {
         const onChangeFn = jest.fn();
         const wrapper = renderSelect({ value: ["opt1"], onChange: onChangeFn });
 
@@ -866,6 +866,7 @@ describe("MultiSelect", () => {
         act(() => {
           wrapper.find(SelectList).prop("onSelect")(clickOptionObject);
         });
+        expect(onChangeFn).toHaveBeenCalledTimes(1);
         expect(onChangeFn).toHaveBeenCalledWith(expectedObject);
       });
     });


Test patch:
diff --git a/src/components/select/multi-select/multi-select-test.stories.tsx b/src/components/select/multi-select/multi-select-test.stories.tsx
index b8f696e97..876134a66 100644
--- a/src/components/select/multi-select/multi-select-test.stories.tsx
+++ b/src/components/select/multi-select/multi-select-test.stories.tsx
@@ -19,7 +19,7 @@ export const Default = () => {
   const handleActivityChange = (event: React.ChangeEvent<HTMLInputElement>) => {
     if (event.target.value.length <= MAX_SELECTIONS_ALLOWED) {
       setSelectedPills((event.target.value as unknown) as string[]);
-      partialAction("onChange");
+      partialAction("onChange")();
     }
   };
   return (
diff --git a/src/components/select/multi-select/multi-select.test.js b/src/components/select/multi-select/multi-select.test.js
index cbd200e35..2f854277c 100644
--- a/src/components/select/multi-select/multi-select.test.js
+++ b/src/components/select/multi-select/multi-select.test.js
@@ -43,7 +43,7 @@ import {
   verifyRequiredAsteriskForLabel,
 } from "../../../../cypress/support/component-helper/common-steps";
 
-import { keyCode, positionOfElement } from "../../../../cypress/support/helper";
+import { positionOfElement } from "../../../../cypress/support/helper";
 import {
   SIZE,
   CHARACTERS,
@@ -770,7 +770,7 @@ context("Tests for Multi Select component", () => {
 
       dropdownButton().click();
       selectList().should("be.visible");
-      selectInput().tab();
+      selectInput().realPress("Tab");
       selectInput().should("have.attr", "aria-expanded", "false");
       selectList().should("not.be.visible");
     });
@@ -780,7 +780,7 @@ context("Tests for Multi Select component", () => {
 
       dropdownButton().click();
       selectList().should("be.visible");
-      selectInput().trigger("keydown", { ...keyCode("Esc") });
+      selectInput().realPress("Escape");
       selectInput().should("have.attr", "aria-expanded", "false");
       selectList().should("not.be.visible");
     });
@@ -796,8 +796,8 @@ context("Tests for Multi Select component", () => {
     });
 
     it.each([
-      ["open", "downarrow"],
-      ["open", "uparrow"],
+      ["open", "ArrowDown"],
+      ["open", "ArrowUp"],
       ["open", "Home"],
       ["open", "End"],
       ["not open", "Enter"],
@@ -807,7 +807,7 @@ context("Tests for Multi Select component", () => {
         CypressMountWithProviders(<MultiSelectComponent />);
 
         commonDataElementInputPreview().focus();
-        selectInput().trigger("keydown", { ...keyCode(key), force: true });
+        selectInput().realPress(key);
         if (state === "open") {
           selectList().should("be.visible");
         } else {
@@ -950,8 +950,7 @@ context("Tests for Multi Select component", () => {
       selectList().should("be.visible");
     });
 
-    // FE-5332 logged for bug in master preventing editing of pills when object set as value
-    it.skip("should open correct list and select one when an object is already set as a value", () => {
+    it("should open correct list and select one when an object is already set as a value", () => {
       CypressMountWithProviders(<MultiSelectObjectAsValueComponent />);
 
       multiSelectPill().should("have.attr", "title", option1);
@@ -1183,7 +1182,7 @@ context("Tests for Multi Select component", () => {
       );
 
       multiSelectPill().should("have.attr", "title", "White");
-      commonDataElementInputPreview().type("{backspace}");
+      commonDataElementInputPreview().focus().realPress("Backspace");
       multiSelectPill().should("not.exist");
     });
 
@@ -1264,7 +1263,7 @@ context("Tests for Multi Select component", () => {
         });
     });
 
-    it("should call onChange event when a list option is selected", () => {
+    it("should call onChange event once when a list option is selected", () => {
       CypressMountWithProviders(<MultiSelectComponent onChange={callback} />);
 
       const position = "first";
@@ -1275,13 +1274,13 @@ context("Tests for Multi Select component", () => {
         .click()
         .then(() => {
           // eslint-disable-next-line no-unused-expressions
-          expect(callback).to.have.been.calledWith({
+          expect(callback).to.have.been.calledOnceWith({
             target: { value: option },
           });
         });
     });
 
-    it.each([["downarrow"], ["uparrow"]])(
+    it.each(["ArrowLeft", "ArrowRight", "ArrowUp", "ArrowDown", "Enter"])(
       "should call onKeyDown event when %s key is pressed",
       (key) => {
         CypressMountWithProviders(
@@ -1290,7 +1289,7 @@ context("Tests for Multi Select component", () => {
 
         commonDataElementInputPreview()
           .focus()
-          .trigger("keydown", { ...keyCode(key), force: true })
+          .realPress(key)
           .then(() => {
             // eslint-disable-next-line no-unused-expressions
             expect(callback).to.have.been.calledOnce;