Skip to content

refactor: extract and unify dialog bounds handling logic #9486

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

Closed
wants to merge 5 commits into from
Closed

refactor: extract and unify dialog bounds handling logic #9486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bdcd05b
always set bounds when dragging or resizing
jouni Jun 16, 2025
f361491
refactor: extract dialog bounds logic into separate class
web-padawan Jun 17, 2025
1676341
refactor: use single method to get event coordinates
web-padawan Jun 17, 2025
c76f5e1
refactor: do not store dialog unnecessarily
web-padawan Jun 17, 2025
83f89a0
fix: restore logic for only setting top and left on drag
web-padawan Jun 17, 2025
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
25 changes: 11 additions & 14 deletions packages/dialog/src/vaadin-dialog-draggable-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
*/
import { isTouch } from '@vaadin/component-base/src/browser-utils.js';
import { eventInWindow, getMouseOrFirstTouchEvent } from './vaadin-dialog-utils.js';
import { DialogManager, eventInWindow, getMouseOrFirstTouchEvent } from './vaadin-dialog-utils.js';

/**
* @polymerMixin
Expand Down Expand Up @@ -47,8 +47,10 @@ export const DialogDraggableMixin = (superClass) =>
/** @protected */
ready() {
super.ready();
this._originalBounds = {};
this._originalMouseCoords = {};

// Get or create an instance of manager
this.__manager = DialogManager.create(this);
Copy link
Contributor

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 this an improvement in terms of structure and readability. Some (very small) parts of the drag and resize logic are now "hidden" in the manager class, while the majority of it stays in the mixins. The class itself doesn't encapsulate any logic on its own, but just contains some utilities and some state. Its purpose is not clear from the name and it's also hard to come up with a better one. The weak map was also confusing me at first.

I think I prefer the duplication in this case, makes it easier to understand what's going on. As an alternative, a random idea would be to merge the two mixins. Or extract some utility functions that avoid duplicating some of the calculations at least. But I don't see a lot of value here in general, unless I'm missing some context where we would need to introduce more duplication for a new feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, actually I was also thinking about merging mixins. It's probably ok to keep the code the way it is.

I still think we should revert some changes from the base styles PR and update tests accordingly.


this._startDrag = this._startDrag.bind(this);
this._drag = this._drag.bind(this);
this._stopDrag = this._stopDrag.bind(this);
Expand Down Expand Up @@ -86,17 +88,12 @@ export const DialogDraggableMixin = (superClass) =>
if (!isDraggable) {
e.preventDefault();
}
this._originalBounds = this.$.overlay.getBounds();
const event = getMouseOrFirstTouchEvent(e);
this._originalMouseCoords = { top: event.pageY, left: event.pageX };
window.addEventListener('mouseup', this._stopDrag);
window.addEventListener('touchend', this._stopDrag);
window.addEventListener('mousemove', this._drag);
window.addEventListener('touchmove', this._drag);
if (this.$.overlay.$.overlay.style.position !== 'absolute') {
const { top, left } = this._originalBounds;
this.$.overlay.setBounds({ top, left });
}

this.__manager.handleEvent(e);
}
}
}
Expand All @@ -105,10 +102,10 @@ export const DialogDraggableMixin = (superClass) =>
_drag(e) {
const event = getMouseOrFirstTouchEvent(e);
if (eventInWindow(event)) {
const top = this._originalBounds.top + (event.pageY - this._originalMouseCoords.top);
const left = this._originalBounds.left + (event.pageX - this._originalMouseCoords.left);
this.top = top;
this.left = left;
const { top, left } = this.__manager.bounds;
const { x, y } = this.__manager.getEventXY(event);
this.top = top + y;
this.left = left + x;
}
}

Expand Down
32 changes: 16 additions & 16 deletions packages/dialog/src/vaadin-dialog-resizable-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
* Copyright (c) 2017 - 2025 Vaadin Ltd.
* This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
*/
import { eventInWindow, getMouseOrFirstTouchEvent } from './vaadin-dialog-utils.js';
import { DialogManager, eventInWindow, getMouseOrFirstTouchEvent } from './vaadin-dialog-utils.js';

/**
* @polymerMixin
*/
Expand All @@ -26,8 +27,10 @@ export const DialogResizableMixin = (superClass) =>
/** @protected */
ready() {
super.ready();
this._originalBounds = {};
this._originalMouseCoords = {};

// Get or create an instance of manager
this.__manager = DialogManager.create(this);

this._resizeListeners = { start: {}, resize: {}, stop: {} };
this._addResizeListeners();
}
Expand Down Expand Up @@ -65,18 +68,12 @@ export const DialogResizableMixin = (superClass) =>
if (e.button === 0 || e.touches) {
e.preventDefault();

this._originalBounds = this.$.overlay.getBounds();
const event = getMouseOrFirstTouchEvent(e);
this._originalMouseCoords = { top: event.pageY, left: event.pageX };
window.addEventListener('mousemove', this._resizeListeners.resize[direction]);
window.addEventListener('touchmove', this._resizeListeners.resize[direction]);
window.addEventListener('mouseup', this._resizeListeners.stop[direction]);
window.addEventListener('touchend', this._resizeListeners.stop[direction]);
if (this.$.overlay.$.overlay.style.position !== 'absolute' || this.width || this.height) {
this.$.overlay.setBounds(this._originalBounds);
}

this.$.overlay.setAttribute('has-bounds-set', '');
this.__manager.handleEvent(e, true);
}
}

Expand All @@ -89,34 +86,37 @@ export const DialogResizableMixin = (superClass) =>
const event = getMouseOrFirstTouchEvent(e);
if (eventInWindow(event)) {
const minimumSize = 40;
const { height: boundsHeight, width: boundsWidth, top: boundsTop, left: boundsLeft } = this.__manager.bounds;
const { x: eventX, y: eventY } = this.__manager.getEventXY(event);

resizer.split('').forEach((direction) => {
switch (direction) {
case 'n': {
const height = this._originalBounds.height - (event.pageY - this._originalMouseCoords.top);
const top = this._originalBounds.top + (event.pageY - this._originalMouseCoords.top);
const height = boundsHeight - eventY;
const top = boundsTop + eventY;
if (height > minimumSize) {
this.top = top;
this.height = height;
}
break;
}
case 'e': {
const width = this._originalBounds.width + (event.pageX - this._originalMouseCoords.left);
const width = boundsWidth + eventX;
if (width > minimumSize) {
this.width = width;
}
break;
}
case 's': {
const height = this._originalBounds.height + (event.pageY - this._originalMouseCoords.top);
const height = boundsHeight + eventY;
if (height > minimumSize) {
this.height = height;
}
break;
}
case 'w': {
const width = this._originalBounds.width - (event.pageX - this._originalMouseCoords.left);
const left = this._originalBounds.left + (event.pageX - this._originalMouseCoords.left);
const width = boundsWidth - eventX;
const left = boundsLeft + eventX;
if (width > minimumSize) {
this.left = left;
this.width = width;
Expand Down
13 changes: 13 additions & 0 deletions packages/dialog/src/vaadin-dialog-utils.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,16 @@ export { getMouseOrFirstTouchEvent };
declare function eventInWindow(e: MouseEvent | TouchEvent): boolean;

export { eventInWindow };

/**
* Internal class handling dialog bounds and coordinates.
*/
export declare class DialogManager {
static create(dialog: HTMLElement): DialogManager;

readonly bounds: { top: number; left: number; width: number; height: number };

handleEvent(event: MouseEvent | Touch, setBounds?: boolean): void;

getEventXY(event: MouseEvent | Touch): { x: number; y: number };
}
50 changes: 50 additions & 0 deletions packages/dialog/src/vaadin-dialog-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,53 @@ export function getMouseOrFirstTouchEvent(e) {
export function eventInWindow(e) {
return e.clientX >= 0 && e.clientX <= window.innerWidth && e.clientY >= 0 && e.clientY <= window.innerHeight;
}

const instancesMap = new WeakMap();

/**
* Internal class handling dialog bounds and coordinates.
*/
export class DialogManager {
static create(dialog) {
if (instancesMap.has(dialog)) {
return instancesMap.get(dialog);
}

const instance = new DialogManager(dialog);
instancesMap.set(dialog, instance);
return instance;
}

constructor(dialog) {
this.overlay = dialog.$.overlay;
this._originalBounds = {};
this._originalMouseCoords = {};
}

handleEvent(e, setBounds = false) {
const event = getMouseOrFirstTouchEvent(e);
this._originalBounds = this.overlay.getBounds();
this._originalMouseCoords = { top: event.pageY, left: event.pageX };

if (setBounds) {
// On resize, we should set width and height
this.overlay.setBounds(this._originalBounds);
this.overlay.setAttribute('has-bounds-set', '');
} else {
// On drag, we should only set top and left
const { top, left } = this._originalBounds;
this.overlay.setBounds({ top, left });
}
}

get bounds() {
return this._originalBounds;
}

getEventXY(event) {
return {
x: event.pageX - this._originalMouseCoords.left,
y: event.pageY - this._originalMouseCoords.top,
};
}
}
16 changes: 0 additions & 16 deletions packages/dialog/test/draggable-resizable.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,14 +305,6 @@ describe('resizable', () => {
expect(Math.floor(resizedBounds.width)).to.be.eql(Math.floor(bounds.width + dx));
});

it('should not set bounds again after position is set to absolute', () => {
const spy = sinon.spy(dialog.$.overlay, 'setBounds');
dispatchMouseEvent(overlayPart.querySelector('.n'), 'mousedown');
dialog.$.overlay.$.overlay.style.position = 'absolute';
dispatchMouseEvent(overlayPart.querySelector('.n'), 'mousedown');
expect(spy.calledOnce).to.be.true;
});

it('should dispatch resize event with correct details', () => {
const onResize = sinon.spy();
dialog.addEventListener('resize', onResize);
Expand Down Expand Up @@ -566,14 +558,6 @@ describe('draggable', () => {
expect(Math.floor(draggedBounds.height)).to.be.eql(Math.floor(bounds.height));
});

it('should not update overlay bounds with position: absolute', () => {
const spy = sinon.spy(dialog.$.overlay, 'setBounds');
dispatchMouseEvent(content, 'mousedown');
dialog.$.overlay.$.overlay.style.position = 'absolute';
dispatchMouseEvent(content, 'mousedown');
expect(spy.calledOnce).to.be.true;
});

it('should not reset scroll position on dragstart', async () => {
dialog.modeless = true;
button.style.marginBottom = '200px';
Expand Down