Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed an issue with not removing the resize handler when user clicks RMB #10416

Merged
merged 8 commits into from
Jun 20, 2023
Merged

Fixed an issue with not removing the resize handler when user clicks RMB #10416

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
40a0d5e
Fixed problem described within the issue (added extra callback to "co…
wszymanski Jun 15, 2023
3c62c52
Added e2e test showing that fix works
wszymanski Jun 15, 2023
ef41105
Added entry to the changelog
wszymanski Jun 15, 2023
3186034
Update handsontable/src/plugins/manualColumnResize/manualColumnResize.js
wszymanski Jun 16, 2023
b7ca869
Changes after CR
wszymanski Jun 16, 2023
edf886b
Minor refactor - variable name & analogous changes for ManualRowResiz…
wszymanski Jun 16, 2023
a8539ed
Removed unnecessary commited line
wszymanski Jun 16, 2023
0471a26
Merge branch 'develop' into feature/dev-issue-1312
wszymanski Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changelogs/10416.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"issuesOrigin": "private",
"title": "Fixed an issue with not removing the resize handler when user clicks RMB ",
"type": "fixed",
"issueOrPR": 10416,
"breaking": false,
"framework": "none"
}
26 changes: 26 additions & 0 deletions handsontable/src/plugins/manualColumnResize/__tests__/manualColumnResize.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,5 +1087,31 @@ describe('manualColumnResize', () => {
expect($handle.css('z-index')).toBeGreaterThan(getTopClone().css('z-index'));
});
});

it('should remove resize handler when user clicks RMB', async() => {
handsontable({
data: Handsontable.helper.createSpreadsheetData(5, 5),
colHeaders: true,
manualColumnResize: true
});

const $colHeader = getTopClone().find('thead tr:eq(0) th:eq(2)');

$colHeader.simulate('mouseover');

const $handle = spec().$container.find('.manualColumnResizer');
const resizerPosition = $handle.position();

$handle.simulate('mousedown', { clientX: resizerPosition.left });

// To watch whether color has changed.
expect(getComputedStyle($handle[0]).backgroundColor).toBe('rgb(52, 169, 219)');

$handle.simulate('contextmenu');

await sleep(0);

expect(getComputedStyle($handle[0]).backgroundColor).not.toBe('rgb(52, 169, 219)');
});
});
});
25 changes: 25 additions & 0 deletions handsontable/src/plugins/manualColumnResize/manualColumnResize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export class ManualColumnResize extends BasePlugin {
this.guide = rootDocument.createElement('DIV');
this.eventManager = new EventManager(this);
this.pressed = null;
this.skipMouseOverAction = false;
this.dblclick = 0;
this.autoresizeTimeout = null;

Expand Down Expand Up @@ -396,6 +397,11 @@ export class ManualColumnResize extends BasePlugin {
return;
}

// A "mouseover" action is triggered right after executing "contextmenu" event. It should be ignored.
if (this.skipMouseOverAction === true) {
budnix marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (this.checkIfColumnHeader(event.target)) {
const th = this.getClosestTHParent(event.target);

Expand Down Expand Up @@ -560,6 +566,24 @@ export class ManualColumnResize extends BasePlugin {
}
}

/**
* Callback for "contextmenu" event triggered on element showing move handle. It removes handle and guide elements.
budnix marked this conversation as resolved.
Show resolved Hide resolved
*/
onContextMenu() {
this.hideHandleAndGuide();
this.hot.rootElement.removeChild(this.handle);
this.hot.rootElement.removeChild(this.guide);
Comment on lines +576 to +577
Copy link
Member

@budnix budnix Jun 16, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.hot.rootElement.removeChild(this.handle);
this.hot.rootElement.removeChild(this.guide);

I think that it is ok that the handler does not disappear after triggering the context menu. The origin bug is still fixed, the handler does not follow the mouse after unclicking the context menu:
Kapture 2023-06-16 at 10 22 56

By removing these lines, we do not add new code to maintain, and the fix still seems to be working. So why not do it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see, there UI element left behind. Title of the issue says that: "The resize handler should be removed when user clicks RMB".

Copy link
Member

@budnix budnix Jun 16, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better code health, I'd remove the lines. UI, in this case, still works fine for me. What do you think @AMBudnik @krzysztofspilka?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify. Current PR fixes the issue by hiding/removing the handler right after the RMB is clicked. I believe it's a bad UI/UX design that some elements just disappear after mouse-clicking.
Kapture 2023-06-16 at 12 34 01

My proposition is not to hide/remove the handler and make it just inactive for RMB.
Kapture 2023-06-16 at 12 36 29

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine with me 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@budnix @wszymanski @AMBudnik

Thanks for discussing this! I think it all comes down to two options that we have:

  1. Leave the resize indicator as it is when the context menu is opened, as @budnix suggested - but then, for the sake of consistency, allow the end-user to resize other columns manually. It's what Google Sheets does.
google-beh.mp4

or

  1. Hide the manual-resize indicator. The mouse cursor should return to default in this case.
excel-beh.mp4

Solution no. 2 blocks the UI outside (behind) the context menu, which is exactly what the OS-context menu does. It's also more consistent with how Handsontable interacts with other plugins. For example, in v12.4.0 a mouse cursor over the dropdown menu button doesn't give you the on-hover visual feedback. The cursor style remains default.

That being said, I'm fond of solution no. 2 (hide the manual-resize indicator).

See the screen recording below for reference.

Screen.Recording.2023-06-17.at.11.58.06.mov


this.pressed = false;
this.skipMouseOverAction = true;

// There is thrown "mouseover" event right after opening a context menu. This flag inform that handle
// shouldn't be drawn just after removing it.
setImmediate(() => {
this.skipMouseOverAction = false;
});
wszymanski marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Binds the mouse events.
*
Expand All @@ -572,6 +596,7 @@ export class ManualColumnResize extends BasePlugin {
this.eventManager.addEventListener(rootElement, 'mousedown', e => this.onMouseDown(e));
this.eventManager.addEventListener(rootWindow, 'mousemove', e => this.onMouseMove(e));
this.eventManager.addEventListener(rootWindow, 'mouseup', () => this.onMouseUp());
this.eventManager.addEventListener(this.handle, 'contextmenu', () => this.onContextMenu());
}

/**
Expand Down