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

[EuiResizableContainer] Mouse drags outside of the container no longer lose dragging state #7456

Merged
merged 4 commits into from
Jan 11, 2024
Merged

[EuiResizableContainer] Mouse drags outside of the container no longer lose dragging state #7456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f1346b
[EuiResizableContainer] Switch from React events to window event list…
cee-chen Jan 10, 2024
a291648
Update resizable helper to work with native touch/mouse events
cee-chen Jan 10, 2024
10bf4ee
Update EuiFlyoutResizable to use EuiResizableContainer helper
cee-chen Jan 10, 2024
1e20797
changelog
cee-chen Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/upcoming/7456.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Enhanced `EuiResizableContainer` to preserve the drag/resize event when the user's mouse leaves the parent container and re-enters
42 changes: 24 additions & 18 deletions src/components/flyout/flyout_resizable.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,27 +62,33 @@ describe('EuiFlyoutResizable', () => {
it('mouse drag', () => {
cy.mount(<EuiFlyoutResizable onClose={onClose} size={800} />);
cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { pageX: 400 })
.trigger('mousemove', { pageX: 600 });
.trigger('mousedown', { clientX: 400 })
.trigger('mousemove', { clientX: 600 });
cy.get('.euiFlyout').should('have.css', 'inline-size', '600px');

cy.get('[data-test-subj="euiResizableButton"]').trigger('mousemove', {
pageX: 200,
clientX: 200,
});
cy.get('.euiFlyout').should('have.css', 'inline-size', '1000px');

// Should not change the flyout width if not dragging
cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mouseup')
.trigger('mousemove', { pageX: 1000 });
.trigger('mousemove', { clientX: 1000 });
cy.get('.euiFlyout').should('have.css', 'inline-size', '1000px');
});

it('mobile touch drag', () => {
cy.mount(<EuiFlyoutResizable onClose={onClose} size={800} />);
cy.get('[data-test-subj="euiResizableButton"]')
.trigger('touchstart', { targetTouches: [{ pageX: 400 }], touches: [] })
.trigger('touchmove', { targetTouches: [{ pageX: 800 }], touches: [] })
.trigger('touchstart', {
targetTouches: [{ clientX: 400 }],
touches: [],
})
.trigger('touchmove', {
targetTouches: [{ clientX: 800 }],
touches: [],
})
.trigger('touchend', { touches: [] });
cy.get('.euiFlyout').should('have.css', 'inline-size', '400px');
});
Expand All @@ -101,8 +107,8 @@ describe('EuiFlyoutResizable', () => {
it('does not allow the flyout to be resized past the window width', () => {
cy.mount(<EuiFlyoutResizable onClose={onClose} size={800} />);
cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { pageX: 400 })
.trigger('mousemove', { pageX: -100 });
.trigger('mousedown', { clientX: 400 })
.trigger('mousemove', { clientX: -100 });
cy.get('.euiFlyout').should('have.css', 'inline-size', '1180px');
});

Expand All @@ -111,8 +117,8 @@ describe('EuiFlyoutResizable', () => {
<EuiFlyoutResizable onClose={onClose} size={800} maxWidth={1000} />
);
cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { pageX: 400 })
.trigger('mousemove', { pageX: 100 });
.trigger('mousedown', { clientX: 400 })
.trigger('mousemove', { clientX: 100 });
cy.get('.euiFlyout').should('have.css', 'inline-size', '1000px');
});

Expand All @@ -121,8 +127,8 @@ describe('EuiFlyoutResizable', () => {
<EuiFlyoutResizable onClose={onClose} size={800} minWidth={100} />
);
cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { pageX: 400 })
.trigger('mousemove', { pageX: 2000 });
.trigger('mousedown', { clientX: 400 })
.trigger('mousemove', { clientX: 2000 });
cy.get('.euiFlyout').should('have.css', 'inline-size', '100px');
});

Expand Down Expand Up @@ -155,8 +161,8 @@ describe('EuiFlyoutResizable', () => {
cy.get('.euiFlyout').should('have.css', 'inline-size', '850px');

cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { pageX: 850, ...options })
.trigger('mousemove', { pageX: 400, ...options });
.trigger('mousedown', { clientX: 850, ...options })
.trigger('mousemove', { clientX: 400, ...options });
cy.get('.euiFlyout').should('have.css', 'inline-size', '400px');
};
});
Expand All @@ -168,8 +174,8 @@ describe('EuiFlyoutResizable', () => {
cy.get('body').should('have.css', 'padding-inline-end', '800px');

cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { pageX: 400 })
.trigger('mousemove', { pageX: 1000 });
.trigger('mousedown', { clientX: 400 })
.trigger('mousemove', { clientX: 1000 });

cy.get('.euiFlyout').should('have.css', 'inline-size', '200px');
cy.get('body').should('have.css', 'padding-inline-end', '200px');
Expand All @@ -187,8 +193,8 @@ describe('EuiFlyoutResizable', () => {
cy.get('body').should('have.css', 'padding-inline-start', '800px');

cy.get('[data-test-subj="euiResizableButton"]')
.trigger('mousedown', { pageX: 800 })
.trigger('mousemove', { pageX: 200 });
.trigger('mousedown', { clientX: 800 })
.trigger('mousemove', { clientX: 200 });

cy.get('.euiFlyout').should('have.css', 'inline-size', '200px');
cy.get('body').should('have.css', 'padding-inline-start', '200px');
Expand Down
15 changes: 3 additions & 12 deletions src/components/flyout/flyout_resizable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import React, {

import { keys, useCombinedRefs, useEuiTheme } from '../../services';
import { EuiResizableButton } from '../resizable_container';
import { getPosition } from '../resizable_container/helpers';

import { EuiFlyout, EuiFlyoutProps } from './flyout';
import { euiFlyoutResizableButtonStyles } from './flyout_resizable.styles';
Expand Down Expand Up @@ -80,7 +81,7 @@ export const EuiFlyoutResizable = forwardRef(

const onMouseMove = useCallback(
(e: MouseEvent | TouchEvent) => {
const mouseOffset = getMouseOrTouchX(e) - initialMouseX.current;
const mouseOffset = getPosition(e, true) - initialMouseX.current;
const changedFlyoutWidth =
initialWidth.current + mouseOffset * direction;

Expand All @@ -100,7 +101,7 @@ export const EuiFlyoutResizable = forwardRef(

const onMouseDown = useCallback(
(e: React.MouseEvent | React.TouchEvent) => {
initialMouseX.current = getMouseOrTouchX(e);
initialMouseX.current = getPosition(e, true);
initialWidth.current = flyoutRef?.offsetWidth ?? 0;

// Window event listeners instead of React events are used
Expand Down Expand Up @@ -155,13 +156,3 @@ export const EuiFlyoutResizable = forwardRef(
}
);
EuiFlyoutResizable.displayName = 'EuiFlyoutResizable';

const getMouseOrTouchX = (
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, our docs still have a copy of this method in use. Is getPosition exported from our package in a way that we can update our docs to use this approach as well?

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2024-01-11 at 2 16 23 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo, thanks for catching this! It looks like we don't export any resizable utils, but I'm now wondering if we should. What do you think? Should we add a new public export for this, probably under src/services? Maybe src/services/drag, or src/services/touch_or_mouse? 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if its worth it or not. I had a suspicion people were copy/pasting this example verbatim and it might clean that up -- but I don't see any examples in Kibana where it looks like someone would have done so. I don't think we need to solve this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair! Will leave this uncollapsed for now in case we decide to come back to this later on.

e: TouchEvent | MouseEvent | React.MouseEvent | React.TouchEvent
): number => {
// Some Typescript fooling is needed here
const x = (e as TouchEvent).targetTouches
? (e as TouchEvent).targetTouches[0].pageX
: (e as MouseEvent).pageX;
return x;
};
24 changes: 10 additions & 14 deletions src/components/resizable_container/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,10 @@ interface Params {
onPanelWidthChange?: ({}: { [key: string]: number }) => any;
}

function isMouseEvent(
event: ReactMouseEvent | ReactTouchEvent
): event is ReactMouseEvent {
return typeof event === 'object' && 'pageX' in event && 'pageY' in event;
}
export const isTouchEvent = (
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't previously exported. Did you mean to add this as an export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured I might as well, although I'm happy to revert if needed! I originally grabbed this util for EuiFlyoutResizable to replace some similar/duplicated code, before realizing I could actually use the parent exported getPosition() util entirely.

That being said, I could definitely see myself using this util again in other places in EUI in the future, if needed 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine either way, I just wanted to double check.

event: MouseEvent | ReactMouseEvent | TouchEvent | ReactTouchEvent
): event is TouchEvent | ReactTouchEvent =>
typeof event === 'object' && 'targetTouches' in event;

export const pxToPercent = (proportion: number, whole: number) => {
if (whole < 1 || proportion < 0) return 0;
Expand Down Expand Up @@ -81,16 +80,13 @@ export const getPanelMinSize = (
};

export const getPosition = (
event: ReactMouseEvent | ReactTouchEvent,
event: ReactMouseEvent | MouseEvent | ReactTouchEvent | TouchEvent,
isHorizontal: boolean
) => {
const clientX = isMouseEvent(event)
? event.clientX
: event.touches[0].clientX;
const clientY = isMouseEvent(event)
? event.clientY
: event.touches[0].clientY;
return isHorizontal ? clientX : clientY;
): number => {
const direction = isHorizontal ? 'clientX' : 'clientY';
return isTouchEvent(event)
? event.targetTouches[0][direction]
: event[direction];
};

const getSiblingPanel = (
Expand Down
24 changes: 16 additions & 8 deletions src/components/resizable_container/resizable_container.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
*/

import React from 'react';
import { act } from 'react-dom/test-utils';
import { mount } from 'enzyme';

import { findTestSubject, requiredProps } from '../../test';
import { shouldRenderCustomStyles } from '../../test/internal';
import { render } from '../../test/rtl';
Expand Down Expand Up @@ -217,8 +219,7 @@ describe('EuiResizableContainer', () => {
};

test('onResizeStart and onResizeEnd are called for pointer events', () => {
const { container, button, onResizeStart, onResizeEnd } =
mountWithCallbacks();
const { button, onResizeStart, onResizeEnd } = mountWithCallbacks();
button.simulate('mousedown', {
pageX: 0,
pageY: 0,
Expand All @@ -227,7 +228,9 @@ describe('EuiResizableContainer', () => {
});
expect(onResizeStart).toHaveBeenCalledTimes(1);
expect(onResizeStart).toHaveBeenLastCalledWith('pointer');
container.simulate('mouseup');
act(() => {
window.dispatchEvent(new Event('mouseup'));
});
expect(onResizeEnd).toHaveBeenCalledTimes(1);
button.simulate('mousedown', {
pageX: 0,
Expand All @@ -237,7 +240,9 @@ describe('EuiResizableContainer', () => {
});
expect(onResizeStart).toHaveBeenCalledTimes(2);
expect(onResizeStart).toHaveBeenLastCalledWith('pointer');
container.simulate('mouseleave');
act(() => {
window.dispatchEvent(new Event('mouseup'));
});
expect(onResizeEnd).toHaveBeenCalledTimes(2);
button.simulate('touchstart', {
touches: [
Expand All @@ -249,7 +254,9 @@ describe('EuiResizableContainer', () => {
});
expect(onResizeStart).toHaveBeenCalledTimes(3);
expect(onResizeStart).toHaveBeenLastCalledWith('pointer');
container.simulate('touchend');
act(() => {
window.dispatchEvent(new Event('touchend'));
});
expect(onResizeEnd).toHaveBeenCalledTimes(3);
});

Expand Down Expand Up @@ -312,8 +319,7 @@ describe('EuiResizableContainer', () => {
});

test('onResizeEnd is called before starting a new resize if a keyboard resize is triggered while a pointer resize is in progress', () => {
const { container, button, onResizeStart, onResizeEnd } =
mountWithCallbacks();
const { button, onResizeStart, onResizeEnd } = mountWithCallbacks();
button.simulate('mousedown', {
pageX: 0,
pageY: 0,
Expand All @@ -326,7 +332,9 @@ describe('EuiResizableContainer', () => {
expect(onResizeEnd).toHaveBeenCalledTimes(1);
expect(onResizeStart).toHaveBeenCalledTimes(2);
expect(onResizeStart).toHaveBeenLastCalledWith('key');
container.simulate('mouseup');
act(() => {
window.dispatchEvent(new Event('mouseup'));
});
expect(onResizeEnd).toHaveBeenCalledTimes(1);
button.simulate('keyup', { key: keys.ARROW_RIGHT });
expect(onResizeEnd).toHaveBeenCalledTimes(2);
Expand Down
67 changes: 25 additions & 42 deletions src/components/resizable_container/resizable_container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,32 +165,32 @@ export const EuiResizableContainer: FunctionComponent<
const position = getPosition(event, isHorizontal);
resizeStart('pointer');
actions.dragStart({ position, prevPanelId, nextPanelId });
},
[actions, isHorizontal, resizeStart]
);

const onMouseMove = useCallback(
(event: React.MouseEvent | React.TouchEvent) => {
if (
!reducerState.prevPanelId ||
!reducerState.nextPanelId ||
!reducerState.isDragging
)
return;
const position = getPosition(event, isHorizontal);
actions.dragMove({
position,
prevPanelId: reducerState.prevPanelId,
nextPanelId: reducerState.nextPanelId,
});
// Window event listeners instead of React events are used to continue
// detecting movement even if the user's mouse leaves the container

const onMouseMove = (event: MouseEvent | TouchEvent) => {
const position = getPosition(event, isHorizontal);
actions.dragMove({ position, prevPanelId, nextPanelId });
};

const onMouseUp = () => {
if (resizeContext.current.trigger === 'pointer') {
resizeEnd();
}
actions.reset();

window.removeEventListener('mousemove', onMouseMove);
window.removeEventListener('mouseup', onMouseUp);
window.removeEventListener('touchmove', onMouseMove);
window.removeEventListener('touchend', onMouseUp);
};
window.addEventListener('mousemove', onMouseMove);
window.addEventListener('mouseup', onMouseUp);
window.addEventListener('touchmove', onMouseMove);
window.addEventListener('touchend', onMouseUp);
},
[
actions,
isHorizontal,
reducerState.prevPanelId,
reducerState.nextPanelId,
reducerState.isDragging,
]
[actions, isHorizontal, resizeStart, resizeEnd]
);

const getKeyMoveDirection = useCallback(
Expand Down Expand Up @@ -246,13 +246,6 @@ export const EuiResizableContainer: FunctionComponent<
[getKeyMoveDirection, resizeEnd]
);

const onMouseUp = useCallback(() => {
if (resizeContext.current.trigger === 'pointer') {
resizeEnd();
}
actions.reset();
}, [actions, resizeEnd]);

const onBlur = useCallback(() => {
if (resizeContext.current.trigger === 'key') {
resizeEnd();
Expand Down Expand Up @@ -322,17 +315,7 @@ export const EuiResizableContainer: FunctionComponent<
resizers: reducerState.resizers,
}}
>
<div
css={cssStyles}
className={classes}
ref={containerRef}
onMouseMove={reducerState.isDragging ? onMouseMove : undefined}
onMouseUp={onMouseUp}
onMouseLeave={onMouseUp}
onTouchMove={onMouseMove}
onTouchEnd={onMouseUp}
{...rest}
>
<div css={cssStyles} className={classes} ref={containerRef} {...rest}>
{render()}
</div>
</EuiResizableContainerContextProvider>
Expand Down
Loading