From a66d64b5477b72ebd013e1378a34c3ea36a3610b Mon Sep 17 00:00:00 2001
From: rahulramesha <71900764+rahulramesha@users.noreply.github.com>
Date: Tue, 7 Mar 2023 15:42:15 +0530
Subject: [PATCH] fix: Drag to select widgets within container like widgets
ends up selecting Parent widget (#20885)
## Description
This PR fixes, drag to select widget feature inside container like
widgets. The changes include,
- Add condition to stop triggering select action when, drag to select is
still active
- Delay stopping drag to select to the end of the execution stack, to
prevent triggering selection action
- Change name of `isDragging` to `isMouseDown` to avoid confusion with
the other `isDragging` in the same file
- Trigger start dragging to select action after `mousedown` and
`mousemove` instead on every `mousedown`
Fixes #20804
Media
## Type of change
- Bug fix (non-breaking change which fixes an issue)
## How Has This Been Tested?
- Manual
Before
https://user-images.githubusercontent.com/71900764/220725344-a4a50770-1335-405f-ac32-2ec63d3c9e6f.mp4
After
https://user-images.githubusercontent.com/71900764/220725390-9d94cd31-28d2-4b21-ae62-dbb98c2678ea.mp4
### Test Plan
> Add Testsmith test cases links that relate to this PR
### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)
## Checklist:
### Dev activity
- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag
### QA activity:
- [ ] Test plan has been approved by relevant developers
- [ ] Test plan has been peer reviewed by QA
- [ ] Cypress test cases have been added and approved by either SDET or
manual QA
- [ ] Organized project review call with relevant stakeholders after
Round 1/2 of QA
- [ ] Added Test Plan Approved label after reveiwing all Cypress test
---
.../CanvasSelectionArena.test.tsx | 7 ++++
.../CanvasArenas/CanvasSelectionArena.tsx | 35 +++++++++++++------
app/client/src/selectors/widgetSelectors.ts | 3 ++
.../DividerWidget/widget/index.test.tsx | 3 ++
.../DropdownWidget/widget/index.test.tsx | 3 ++
5 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.test.tsx b/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.test.tsx
index 1faea0460f..599be8d92b 100644
--- a/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.test.tsx
+++ b/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.test.tsx
@@ -44,6 +44,11 @@ describe("Canvas selection test cases", () => {
beforeEach(() => {
spyWidgetSelection.mockClear();
+ jest.useFakeTimers();
+ });
+
+ afterEach(() => {
+ jest.useRealTimers();
});
it("Should select using canvas draw", () => {
@@ -502,6 +507,8 @@ describe("Canvas selection test cases", () => {
`canvas-selection-${MAIN_CONTAINER_WIDGET_ID}`,
);
+ jest.runOnlyPendingTimers();
+
expect(selectionCanvas.style.zIndex).toBe("");
act(() => {
fireEvent.dragStart(widgetEditor);
diff --git a/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.tsx b/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.tsx
index a7a7d37d32..47021133d7 100644
--- a/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.tsx
+++ b/app/client/src/pages/common/CanvasArenas/CanvasSelectionArena.tsx
@@ -169,7 +169,8 @@ export function CanvasSelectionArena({
});
let selectionRectangle: SelectedArenaDimensions = initRectangle();
let isMultiSelect = false;
- let isDragging = false;
+ let isMouseDown = false;
+ let shouldStartCanvasDragging = false;
const getSelectionDimensions = () => {
return {
@@ -242,7 +243,7 @@ export function CanvasSelectionArena({
const onMouseEnter = (e: any) => {
if (
slidingArenaRef.current &&
- !isDragging &&
+ !isMouseDown &&
drawOnEnterObj?.current.canDraw
) {
firstRender(e, true);
@@ -259,7 +260,7 @@ export function CanvasSelectionArena({
Math.abs(selectionRectangle.width) >
0
) {
- if (!isDragging) {
+ if (!isMouseDown) {
// cant set this in onMouseUp coz click seems to happen after onMouseUp.
selectionRectangle = initRectangle();
}
@@ -306,7 +307,11 @@ export function CanvasSelectionArena({
};
const firstRender = (e: any, fromOuterCanvas = false) => {
- if (slidingArenaRef.current && stickyCanvasRef.current && !isDragging) {
+ if (
+ slidingArenaRef.current &&
+ stickyCanvasRef.current &&
+ !isMouseDown
+ ) {
isMultiSelect = e.ctrlKey || e.metaKey || e.shiftKey;
if (fromOuterCanvas) {
const { left, top } = startPositionsForOutCanvasSelection();
@@ -320,7 +325,8 @@ export function CanvasSelectionArena({
}
selectionRectangle.width = 0;
selectionRectangle.height = 0;
- isDragging = true;
+ isMouseDown = true;
+ shouldStartCanvasDragging = true;
// bring the canvas to the top layer
stickyCanvasRef.current.style.zIndex = "2";
slidingArenaRef.current.style.zIndex = "2";
@@ -335,13 +341,12 @@ export function CanvasSelectionArena({
slidingArenaRef.current &&
(!isDraggableParent || e.ctrlKey || e.metaKey)
) {
- dispatch(setCanvasSelectionStateAction(true, widgetId));
firstRender(e);
}
};
const onMouseUp = () => {
- if (isDragging && slidingArenaRef.current && stickyCanvasRef.current) {
- isDragging = false;
+ if (isMouseDown && slidingArenaRef.current && stickyCanvasRef.current) {
+ isMouseDown = false;
canvasCtx.clearRect(
0,
0,
@@ -351,11 +356,21 @@ export function CanvasSelectionArena({
stickyCanvasRef.current.style.zIndex = "";
slidingArenaRef.current.style.zIndex = "";
slidingArenaRef.current.style.cursor = "";
- dispatch(setCanvasSelectionStateAction(false, widgetId));
+ //moving triggering action to the end of queue,
+ // to avoid selecting the widget being dragged on
+ setTimeout(() => {
+ dispatch(setCanvasSelectionStateAction(false, widgetId));
+ }, 0);
}
};
const onMouseMove = (e: any) => {
- if (isDragging && slidingArenaRef.current && stickyCanvasRef.current) {
+ if (isMouseDown && slidingArenaRef.current && stickyCanvasRef.current) {
+ // This is to make sure we start selection only after dragging start
+ // rather than mouse down
+ if (shouldStartCanvasDragging) {
+ dispatch(setCanvasSelectionStateAction(true, widgetId));
+ shouldStartCanvasDragging = false;
+ }
selectionRectangle.width =
e.offsetX -
slidingArenaRef.current.offsetLeft -
diff --git a/app/client/src/selectors/widgetSelectors.ts b/app/client/src/selectors/widgetSelectors.ts
index 59feaa7181..c3ebcc4323 100644
--- a/app/client/src/selectors/widgetSelectors.ts
+++ b/app/client/src/selectors/widgetSelectors.ts
@@ -148,6 +148,7 @@ export const shouldWidgetIgnoreClicksSelector = (widgetId: string) => {
getIsTableFilterPaneVisible,
(state: AppState) => state.ui.widgetDragResize.isResizing,
(state: AppState) => state.ui.widgetDragResize.isDragging,
+ (state: AppState) => state.ui.canvasSelection.isDraggingForSelection,
getAppMode,
getIsAutoHeightWithLimitsChanging,
(
@@ -155,12 +156,14 @@ export const shouldWidgetIgnoreClicksSelector = (widgetId: string) => {
isTableFilterPaneVisible,
isResizing,
isDragging,
+ isDraggingForSelection,
appMode,
isAutoHeightWithLimitsChanging,
) => {
const isFocused = focusedWidgetId === widgetId;
return (
+ isDraggingForSelection ||
isResizing ||
isDragging ||
appMode !== APP_MODE.EDIT ||
diff --git a/app/client/src/widgets/DividerWidget/widget/index.test.tsx b/app/client/src/widgets/DividerWidget/widget/index.test.tsx
index 4e49080257..9a0dfa61e6 100644
--- a/app/client/src/widgets/DividerWidget/widget/index.test.tsx
+++ b/app/client/src/widgets/DividerWidget/widget/index.test.tsx
@@ -40,6 +40,9 @@ describe("", () => {
autoHeightUI: {
isAutoHeightWithLimitsChanging: false,
},
+ canvasSelection: {
+ isDraggingForSelection: false,
+ },
},
entities: { canvasWidgets: {}, app: { mode: "canvas" } },
};
diff --git a/app/client/src/widgets/DropdownWidget/widget/index.test.tsx b/app/client/src/widgets/DropdownWidget/widget/index.test.tsx
index 86ac16eaed..e1d6bff04b 100644
--- a/app/client/src/widgets/DropdownWidget/widget/index.test.tsx
+++ b/app/client/src/widgets/DropdownWidget/widget/index.test.tsx
@@ -44,6 +44,9 @@ describe("", () => {
autoHeightUI: {
isAutoHeightWithLimitsChanging: false,
},
+ canvasSelection: {
+ isDraggingForSelection: false,
+ },
},
entities: { canvasWidgets: {}, app: { mode: "canvas" } },
};