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.

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

Add/implement pc.EVENT_MOUSEOUT and pc.EVENT_MOUSEENTER #4920

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
14 changes: 14 additions & 0 deletions src/platform/input/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,20 @@ export const EVENT_MOUSEUP = 'mouseup';
*/
export const EVENT_MOUSEWHEEL = 'mousewheel';

/**
* Name of event fired when the mouse moves out or enters another DOM element.
Copy link
Contributor

@yaustar yaustar May 11, 2023

Choose a reason for hiding this comment

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

Suggested change
* Name of event fired when the mouse moves out or enters another DOM element.
* Name of event fired when the mouse moves out or enters another DOM element. This can fire on entering a PlayCanvas browser window not in focus so you may want to check with {@link https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus document.hasFocus()}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it really "defocused" or "unfocused"?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe 'background' is a better term?

Copy link
Contributor

@yaustar yaustar May 11, 2023

Choose a reason for hiding this comment

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

Updated suggested change with something with less jargon.

Edit: I just saw the commit, that looks fine too :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just changed it via normal commit, GitHub is having some strange issues:

image

(for my sake it sounds good enough, but you can make a suggestion if needed)

*
* @type {string}
*/
export const EVENT_MOUSEOUT = 'mouseout';

/**
* Name of event fired when the mouse moves out or enters another DOM element.
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly confused here since the description is identical to the first sentence of the description for EVENT_MOUSEOUT.

*
* @type {string}
*/
export const EVENT_MOUSEENTER = 'mouseenter';

/**
* Name of event fired when a new touch occurs. For example, a finger is placed on the device.
*
Expand Down
2 changes: 1 addition & 1 deletion src/platform/input/mouse-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class MouseEvent {
* @type {number}
*/
this.y = coords.y;
} else if (isMousePointerLocked()) {
} else if (isMousePointerLocked() || event.type === 'mouseout') {
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

On the same note, what values are coords.x/y on a mouse out event?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why is this necessary?

coords = mouse._getTargetCoords(event); returns null and then the constructor just ends here:

} else {
return;
}

Resulting in the entire event being "uninitialized".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the same note, what values are coords.x/y on a mouse out event?

@yaustar There are two cases:

  1. When mouse is inside the canvas while entering another DOM element, mouseout is fired and then its the normal x/y
  2. When mouse leaves the window, _getTargetCoords returns null and x/y sticks to 0 (because _getTargetCoords returns some proper values).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see 🤔 This seems fine to me

this.x = 0;
this.y = 0;
} else {
Expand Down
36 changes: 33 additions & 3 deletions src/platform/input/mouse.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { platform } from '../../core/platform.js';
import { EventHandler } from '../../core/event-handler.js';

import { EVENT_MOUSEDOWN, EVENT_MOUSEMOVE, EVENT_MOUSEUP, EVENT_MOUSEWHEEL } from './constants.js';
import { EVENT_MOUSEDOWN, EVENT_MOUSEENTER, EVENT_MOUSEMOVE, EVENT_MOUSEOUT, EVENT_MOUSEUP, EVENT_MOUSEWHEEL } from './constants.js';
import { isMousePointerLocked, MouseEvent } from './mouse-event.js';

/**
Expand All @@ -16,6 +16,12 @@ import { isMousePointerLocked, MouseEvent } from './mouse-event.js';
* @augments EventHandler
*/
class Mouse extends EventHandler {
/**
* @type {Element|null}
* @private
*/
_target;

/**
* Create a new Mouse instance.
*
Expand All @@ -36,6 +42,8 @@ class Mouse extends EventHandler {
this._downHandler = this._handleDown.bind(this);
this._moveHandler = this._handleMove.bind(this);
this._wheelHandler = this._handleWheel.bind(this);
this._enterHandler = this._handleEnter.bind(this);
this._outHandler = this._handleOut.bind(this);
this._contextMenuHandler = (event) => {
event.preventDefault();
};
Expand Down Expand Up @@ -86,7 +94,7 @@ class Mouse extends EventHandler {
/**
* Attach mouse events to an Element.
*
* @param {Element} element - The DOM element to attach the mouse to.
* @param {Element} [element] - The DOM element to attach the mouse to.
*/
attach(element) {
this._target = element;
Expand All @@ -99,6 +107,10 @@ class Mouse extends EventHandler {
window.addEventListener('mousedown', this._downHandler, opts);
window.addEventListener('mousemove', this._moveHandler, opts);
window.addEventListener('wheel', this._wheelHandler, opts);
if (element) {
element.addEventListener('mouseenter', this._enterHandler, opts);
element.addEventListener('mouseout', this._outHandler, opts);
}
}

/**
Expand All @@ -107,13 +119,17 @@ class Mouse extends EventHandler {
detach() {
if (!this._attached) return;
this._attached = false;
this._target = null;

const opts = platform.passiveEvents ? { passive: false } : false;
window.removeEventListener('mouseup', this._upHandler, opts);
window.removeEventListener('mousedown', this._downHandler, opts);
window.removeEventListener('mousemove', this._moveHandler, opts);
window.removeEventListener('wheel', this._wheelHandler, opts);
if (this._target) {
this._target.removeEventListener('mousein', this._enterHandler, opts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be mouseenter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, nice catch, absolutely and thank you!

this._target.removeEventListener('mouseout', this._outHandler, opts);
this._target = null;
}
}

/**
Expand Down Expand Up @@ -289,6 +305,20 @@ class Mouse extends EventHandler {
this.fire(EVENT_MOUSEWHEEL, e);
}

_handleEnter(event) {
const e = new MouseEvent(this, event);
if (!e.event) return;

this.fire(EVENT_MOUSEENTER, e);
}

_handleOut(event) {
const e = new MouseEvent(this, event);
if (!e.event) return;

this.fire(EVENT_MOUSEOUT, e);
}

_getTargetCoords(event) {
const rect = this._target.getBoundingClientRect();
const left = Math.floor(rect.left);
Expand Down