[DataGrid] Fix1295 backspace #1322
Conversation
| cellRef.current!.focus(); | ||
| } | ||
| }); | ||
| }, [hasFocus]); |
There was a problem hiding this comment.
fix #1295, basically we trigger focus only when hasFocus changed
There was a problem hiding this comment.
This seems to be a symptom, not the root cause. From what I understand we should filter all the events that are coming from a portal in
diff --git a/packages/grid/_modules_/grid/components/GridRow.tsx b/packages/grid/_modules_/grid/components/GridRow.tsx
index 27135564..f1a3f0be 100644
--- a/packages/grid/_modules_/grid/components/GridRow.tsx
+++ b/packages/grid/_modules_/grid/components/GridRow.tsx
@@ -27,8 +27,14 @@ export const GridRow: React.FC<RowProps> = ({ selected, id, className, rowIndex,
const rowHeight = useGridSelector(apiRef, gridDensityRowHeightSelector);
const publish = React.useCallback(
- (eventName: string) => (event: React.MouseEvent) =>
+ (eventName: string) => (event: React.MouseEvent) => {
+ // Ignore portal
+ if (!event.currentTarget.contains(event.target)) {
+ return;
+ }
+
apiRef!.current.publishEvent(eventName, apiRef?.current.getRowParams(id), event),
+ },
[apiRef, id],
);what do you think?
There was a problem hiding this comment.
you would need to stopPropagation for all events that are fired from an element inside the cell. So it doesn't bubble to the rowClick... So if you have a renderCell with a div then it wouldn't propagate which seems wrong.
Or we could stop propagation for events coming from an input, but then if you click the dialog div it will bubble up to the row and so on
I think it should be up to users to stop propagation which gives the best flexibility.
There is a big react issue for propagation of events in portals and I suggest that we don't support portals in cells for now. Or at least that the propagation of events should be handled by users.
facebook/react#11387
I suggest to close this PR as the fix I proposed originally might not be ideal as if the cell gets rendered a 2nd time and the focus has not changed then it will loose focus, which is why there is no dependency in the useEffect
There was a problem hiding this comment.
If the cell gets rendered a 2nd time and the focus has not changed then it will loose focus, which is why there is no dependency in the useEffect
Will it lose focus because the dom node is different?
There was a problem hiding this comment.
because it redraws ie if there is an updated value
There was a problem hiding this comment.
Why would a redraw impact the focus? Maybe you are referring to what happens when we toggle between the read and edit mode. In which case, maybe we can only trigger a focus when the edit/read mode change?
I would argue they are not related. They can and should be fixed in isolation, one stone at a time. |
oliviertassinari
added
the
component: data grid
fix #1295, backspace issue only, lead to more issue with portals
This PR is based on #1319 as they are all related...