[Bug]: TableToolbarSearch is calling onChangeProp with incorrect parameters when there is a default value

[laramlewis]

Package

carbon-components-react

Browser

No response

Package version

v7.59.8

React version

v16.14.0

Description

In TableToolbarSearch.js, it is calling onChangeProp with two strings instead of an event as expected when a default value is specified.

  useEffect(
    () => {
      if (defaultValue) {
        onChangeProp?.('', defaultValue);
      }
    },
    //eslint-disable-next-line react-hooks/exhaustive-deps
    []
  );

As a result the onChange handler is passed '' instead of an event with a target value.

Reproduction/example

N/A

Steps to reproduce

• Render a TableToolbarSearch component with a defaultValue and an onChange handler.
• The onChange handler will be called with an emptyString instead of an event.

Suggested Severity

Severity 3 = User can complete task, and/or has a workaround within the user experience of a given component.

Application/PAL

IBM Knowledge Catalog (formally Watson Knowledge Catalog)

Code of Conduct

• I agree to follow this project's Code of Conduct
• I checked the current issues for duplicate problems

[tay1orjones]

Thanks for reporting this! Here's the relevant source you're referring to:

carbon/packages/react/src/components/DataTable/TableToolbarSearch.tsx

Lines 179 to 187 in 0d75092

	useEffect(
	() => {
	if (defaultValue) {
	onChangeProp?.('', defaultValue);
	}
	},
	//eslint-disable-next-line react-hooks/exhaustive-deps
	[]
	);

Since the dependency array is empty, this is only ran once, after the initial render. What event would you expect onChange to be called with in this case? I agree in principle that the format of what's passed through the callback should ideally be the same as the event populated from the actual input's onChange, but I'm not sure what we'd change it to be. What are your thoughts?

[laramlewis]

@tay1orjones - Thank you for your quick response. I discussed this with my colleague and we have the following thoughts:

• Creating a custom event is doable: https://developer.mozilla.org/en-US/docs/Web/Events/Creating_and_triggering_events

• The problem with creating a ‘fake’ event is that users can’t tell if it’s coming from the ‘mount’ or actual user changes.
  unless you also give that event an attribute to identify it’s different from the change event.

• Another thought my colleague had is to make it consistent to TextInput where onChange is not called on ‘mount’ but only when it’s actually changed.
  https://github.com/carbon-design-system/carbon/blob/0d75092bfc865937cdb77b78e8e4d9[…]8cfcd33f3/packages/react/src/components/TextInput/TextInput.tsx

Since the current implementation is broken and calls onChange with an empty string instead of an event, consumers are probably not relying on this handler being called when TableToolbarSearch is mounted with a default value.

[wkeese]

Another thought my colleague had is to make it consistent to TextInput where onChange is not called on ‘mount’ but only when it’s actually changed.

I agree, that's also consistent with how onchange events work on native HTML elements.

Since the current implementation is broken and calls onChange with an empty string instead of an event, consumers are probably not relying on this handler being called when TableToolbarSearch is mounted with a default value.

Unfortunately, I'm guessing the opposite, that some code uses the initial onChange() call to trigger a search on the default value. And that code would break if we just dropped the initial onChange() call.

In other words, I agree with dropping the initial onChange() call but it seems like you would need to wait until Carbon 12. Likewise for replacing the null-like event object with a custom event.

[kubijo]

Just to add another five cents of context into this, please make sure to back port to the 7.* version line. We are now on 7.59.19 because of the incomplete state of typescript annotations (and the massive undertaking it will take to migrate).