amaannawab923 commented on code in PR #43756:
URL: https://github.com/apache/superset/pull/43756#discussion_r3906771094
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -87,10 +94,14 @@ export default (memo as <T>(fn: T) => T)(function
GlobalFilter<
}: GlobalFilterProps<D>) {
const count = serverPagination ? rowCount : preGlobalFilteredRows.length;
const inputRef = useRef<InputRef>(null);
+ const isComposingRef = useRef(false);
const [value, setValue] = useAsyncState(
filterValue,
(newValue: string) => {
+ if (isComposingRef.current) {
Review Comment:
Worth confirming the intended scope here. Android soft keyboards, Gboard in
particular, fire `compositionstart` and `compositionend` around ordinary Latin
word entry with suggestions enabled, not only for CJK input methods.
With this guard in place, search on those keyboards defers to word
boundaries rather than the 200ms debounce. That may well be acceptable or even
preferable, but it is a behaviour change for a much larger group than the issue
describes, so it seems worth being deliberate about rather than incidental.
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -118,6 +129,17 @@ export default (memo as <T>(fn: T) => T)(function
GlobalFilter<
isSearchFocused.set(id, false);
};
+ const handleCompositionStart = () => {
+ isComposingRef.current = true;
Review Comment:
`isComposingRef` is set here and cleared only in `handleCompositionEnd`. If
a composition is interrupted without a `compositionend`, the flag stays set for
the lifetime of the component and every later debounced `setGlobalFilter`
returns early at the guard above, so search stops working with nothing surfaced
to the user.
Blur while a candidate window is open is the easiest way to reach that
state, and it is not consistent across browsers and IMEs.
Clearing it in `handleBlur` alongside `isSearchFocused.set(id, false)` would
bound the failure to a single interaction rather than the component instance.
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx:
##########
@@ -118,6 +129,17 @@ export default (memo as <T>(fn: T) => T)(function
GlobalFilter<
isSearchFocused.set(id, false);
};
+ const handleCompositionStart = () => {
+ isComposingRef.current = true;
+ };
+
+ const handleCompositionEnd = (
+ e: React.CompositionEvent<HTMLInputElement>,
Review Comment:
Nit: this file imports its React types by name, including
`CompositionEventHandler` added just above, and does not import the `React`
namespace. Referring to `React.CompositionEvent` here relies on the global UMD
namespace instead.
`CompositionEvent<HTMLInputElement>` from the existing named import would
keep it consistent with the rest of the file.
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -2658,6 +2659,74 @@ describe('plugin-chart-table', () => {
expect(screen.queryByText('Search by')).toBeInTheDocument();
});
+ test('defers server-side search until IME composition ends', async () => {
+ jest.useFakeTimers();
+ try {
+ const setDataMask = jest.fn();
+ const props = transformProps({
+ ...testData.raw,
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ server_pagination: true,
+ include_search: true,
+ },
+ hooks: { setDataMask },
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['name'],
+ coltypes: [GenericDataType.String],
+ data: [{ name: 'Michael' }, { name: 'John' }],
+ },
+ ],
+ });
+ render(
+ ProviderWrapper({
+ children: (
+ <TableChart {...props} setDataMask={setDataMask} sticky={false} />
+ ),
+ }),
+ );
+
+ const searchInput = screen.getByRole('textbox');
+ const searchCalls = () =>
+ setDataMask.mock.calls.filter(([mask]) =>
+ Object.prototype.hasOwnProperty.call(
+ mask?.ownState ?? {},
+ 'searchText',
+ ),
+ );
+
+ fireEvent.compositionStart(searchInput);
+ fireEvent.change(searchInput, { target: { value: 'nihao' } });
+
+ await act(async () => {
+ jest.advanceTimersByTime(300);
+ });
+ await act(async () => {
+ jest.advanceTimersByTime(900);
+ });
+ expect(searchInput).toHaveValue('nihao');
+ expect(searchCalls()).toHaveLength(0);
+
+ fireEvent.change(searchInput, { target: { value: '你好' } });
+ fireEvent.compositionEnd(searchInput);
Review Comment:
The test fires the final change before `compositionEnd`. Browsers differ on
this ordering, and Chrome commonly delivers `compositionend` before the final
input event.
The implementation already handles that case by reading
`e.currentTarget.value` directly, which is the part worth protecting. A second
case that fires `compositionEnd` first, with the committed text set on the
element, would lock that in. As written the suite would still pass if the
handler were changed to depend on the preceding change event.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
