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

kungfooman · 2022-12-10T10:52:12Z

Fixes: #739

This event is missing, so devs always had to add custom event handling in their scripts, like here:

Lines 378 to 388 in b07a438

    
           var onMouseOut = function (e) { 
        
               self.onMouseOut(e); 
        
           }; 
        
           this.app.mouse.on(pc.EVENT_MOUSEDOWN, this.onMouseDown, this); 
        
           this.app.mouse.on(pc.EVENT_MOUSEUP, this.onMouseUp, this); 
        
           this.app.mouse.on(pc.EVENT_MOUSEMOVE, this.onMouseMove, this); 
        
           this.app.mouse.on(pc.EVENT_MOUSEWHEEL, this.onMouseWheel, this); 
        
           // Listen to when the mouse travels out of the window 
        
           window.addEventListener('mouseout', onMouseOut, false);

This will also simplify event handling later on based on #4910, in short ScriptType#listen provides:

Less code
Automatically removing events on disable/destroy
Automatically adding them back on enable
Harder to get it wrong

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

slimbuck · 2022-12-13T10:18:15Z

src/platform/input/mouse-event.js

@@ -50,7 +50,7 @@ class MouseEvent {
             * @type {number}
             */
            this.y = coords.y;
-        } else if (isMousePointerLocked()) {
+        } else if (isMousePointerLocked() || event.type === 'mouseout') {


why is this necessary?

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

why is this necessary?

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

engine/src/platform/input/mouse-event.js

Lines 56 to 58 in 7c582ab

} else {

return;

}

Resulting in the entire event being "uninitialized".

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

@yaustar There are two cases:

When mouse is inside the canvas while entering another DOM element, mouseout is fired and then its the normal x/y

When mouse leaves the window, _getTargetCoords returns null and x/y sticks to 0 (because _getTargetCoords returns some proper values).

Ah, I see 🤔 This seems fine to me

src/platform/input/constants.js

yaustar · 2022-12-13T12:51:11Z

I also noticed that mouse out event fires in unexpected situations such as:

When I click drag and move out of the window, I get the event on the move out and also on button release

Kapture.2022-12-13.at.12.45.54.mp4

When the PlayCanvas window is not in focus and moving between the active browser window and the PlayCanvas one

Kapture.2022-12-13.at.12.49.39.mp4

…t talking about pc Elements Co-authored-by: Steven <yaustar@users.noreply.github.com>

kungfooman · 2022-12-13T14:04:58Z

@yaustar Thank you for checking, it seems to be default behaviour, I get the same in Firefox and Chrome.

The easiest way to test it is any empty page and then just:

window.onmouseout = e => console.log(e.target.tagName);

I don't know why browsers do it, but do you think PC events should try to filter such cases?

yaustar · 2022-12-13T14:06:54Z

I don't know why browsers do it, but do you think PC events should try to filter such cases?

I'm in two minds about this. On one hand, it makes sense to mirror the native events like we do everywhere else. On the other, it would be nice to have mouse in and mouse out instead of both being on the same event?

kungfooman · 2022-12-13T14:29:51Z

On the other, it would be nice to have mouse in and mouse out instead of both being on the same event?

Yea, I share your concerns here, seems like this works best:

document.body.onmouseout = console.log;

It isn't confused between BODY/HTML, but what I don't like is that it's accessing body which can be undefined (coming from a <script> in <head> e.g.)

yaustar · 2022-12-13T14:33:18Z

document.body.onmouseout = console.log;

Yeah, let's not do that as the mouse DOM element can be any DOM element too on attach.

Wait, is target element not used in the mouse input? 👀 We just listen the window regardless?

kungfooman · 2022-12-13T14:42:00Z

Wait, is target element not used in the mouse input? 👀 We just listen the window regardless?

Right, _target is just used for some context menu handling and position calculations, the rest attaches events onto window:

engine/src/platform/input/mouse.js

Lines 91 to 102 in c3d5ff7

    
           attach(element) { 
        
               this._target = element; 
        
               if (this._attached) return; 
        
               this._attached = true; 
        
               const opts = platform.passiveEvents ? { passive: false } : false; 
        
               window.addEventListener('mouseup', this._upHandler, opts); 
        
               window.addEventListener('mousedown', this._downHandler, opts); 
        
               window.addEventListener('mousemove', this._moveHandler, opts); 
        
               window.addEventListener('wheel', this._wheelHandler, opts); 
        
           }

yaustar · 2022-12-13T14:42:51Z

Can we use it to know if we are 'mousing out' or 'in'?

kungfooman · 2022-12-13T15:00:52Z

Can we use it to know if we are 'mousing out' or 'in'?

You mean to have e.g. a small 100x100px canvas and then trigger mouseout when this small area is left and not the window?

It should be possible, just that it would deviate from how e.g. the OrbitCamera script used to use it (if we speak about the same thing here).

I tested window.onmouseenter but that doesn't fires at all for me. Having a pc.EVENT_MOUSEIN should still be quite easy using the mouse move events tho (or _target of pc.app.mouse)

yaustar · 2022-12-13T16:31:28Z

It should be possible, just that it would deviate from how e.g. the OrbitCamera script used to use it (if we speak about the same thing here).

That's fine, I wrote that years ago when I wasn't really thinking that closely where I wanted mouse out 😅

kungfooman · 2022-12-13T16:56:37Z

Yea, it seems more sensible to have it on Mouse#_target as it will behave less surprisingly.

(I can rewrite it tomorrow if no one comes up with a counter-argument 😅)

yaustar · 2022-12-22T10:42:43Z

@kungfooman Is this ready for a re-review?

kungfooman · 2022-12-22T11:32:46Z

@kungfooman Is this ready for a re-review?

Yep, I would like that, quick intro:

pc.app.mouse.on(pc.EVENT_MOUSEENTER, console.log);
pc.app.mouse.on(pc.EVENT_MOUSEOUT  , console.log);

or:

pc.app.mouse.on(pc.EVENT_MOUSEENTER, _ => console.log("mouse enter", _));
pc.app.mouse.on(pc.EVENT_MOUSEOUT  , _ => console.log("mouse out"  , _));

yaustar · 2022-12-23T14:13:08Z

If the window is not in focus, when the mouse enters, it fires both the ENTER and OUT events (see end of the video). Was this intended?

Kapture.2022-12-23.at.14.12.21.mp4

kungfooman · 2022-12-24T10:40:11Z

If the window is not in focus, when the mouse enters, it fires both the ENTER and OUT events (see end of the video). Was this intended?

Thank you for testing! It might be a Chrome bug on Mac, I can't reproduce on Linux/Chrome:

yaustar · 2023-01-05T12:58:32Z

Definitely Chrome specific but it's difficult to ship this when it 'doesn't work' with the major browser user share 😅

Edit: Wondering if we should only fire the event if the window is in focus?

LeXXik · 2023-01-05T13:20:06Z

Yeah, I also remember having issues with onmouseout in the past. At least on Windows. Have you tried onmouseleave?

kungfooman · 2023-01-05T15:13:12Z

I reported mouse issues in the past and it usually takes some time until issues are fixed, but imo that is the way to go - just someone needs to create a Chrome issue and keep up with it.

Issue 356090: mouseout on element triggered on mouseup after entering the element with the mouse button pressed

https://bugs.chromium.org/p/chromium/issues/detail?id=356090

Sounds like they had it before, apparently "fixed", but not quite it seems.

For my sake it works good enough besides this have-another-window-over-canvas and I'm not trying to find Chrome specific hacks for that case.

Edit: Wondering if we should only fire the event if the window is in focus?

Just for testing:

setInterval(()=>console.log(document.hasFocus()), 1000)

But I don't see how that would work out nicely, as soon you click outside the tab you won't have any "entering" events anymore?

yaustar · 2023-01-05T15:15:58Z

But I don't see how that would work out nicely, as soon you click outside the tab you won't have any "entering" events anymore?

There aren't any mousemove events on the app if the tab isn't in focus either so I would argue it's consistent behaviour?

kungfooman · 2023-01-05T15:26:37Z

There aren't any mousemove events on the app if the tab isn't in focus either so I would argue it's consistent behaviour?

The mousemove events are always fired, focused or not:

pc.app.mouse.on("mousemove", console.log);

E.g. for devtools in https://playcanvas.com/viewer if you want to test. Or do you mean something different?

yaustar · 2023-01-06T10:49:45Z

Hmm, I get different behaviour on Chrome/Mac

Kapture.2023-01-06.at.10.48.54.mp4

kungfooman · 2023-01-06T11:58:33Z

Funny how different the same browser is on different platforms:

Peek.2023-01-06.12-49.mp4

IIRC Chrome on Windows also sends mousemove even though the window is unfocused, so Mac is the oddball here.

Technically it imo makes more sense to not filter events too early, since once they are filtered, there is no way back. And for a dev it would be simple enough to just if (!document.hasFocus()) return; if they want that?

yaustar · 2023-01-06T13:23:37Z

Technically it imo makes more sense to not filter events too early, since once they are filtered, there is no way back. And for a dev it would be simple enough to just if (!document.hasFocus()) return; if they want that?

Part of me feels like the engine should be as consistent as possible in the events it fires. If a developer wants the raw events, then there's nothing stopping them registering events on the DOM

kungfooman · 2023-01-06T15:17:02Z

Part of me feels like the engine should be as consistent as possible in the events it fires.

So mousemove shall also be filtered now to make everything work like on a Chrome Mac?

yaustar · 2023-01-06T15:29:18Z

So mousemove shall also be filtered now to make everything work like on a Chrome Mac?

Ideally, it should be consistent otherwise we would end up with bug reports in the engine.

The fact that mouse move is different on a per OS basis is frustrating and truthfully, I don't know what the best way forward here is.

We could ship this as is where the behaviour is consistent across OS's browser when the tab is in focus by the user. I think that would be okay given noone has reported about mouse move being an issue when the browser is not in focus.

yaustar · 2023-05-11T11:39:59Z

Thank you for testing! It might be a Chrome bug on Mac, I can't reproduce on Linux/Chrome:

It works fine if the active window is not another Chrome window. That's probably why you can't reproduce it as you are checking against VSCode

yaustar · 2023-05-11T11:53:10Z

src/platform/input/constants.js

@@ -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.


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()}.

Is it really "defocused" or "unfocused"?

maybe 'background' is a better term?

Updated suggested change with something with less jargon.

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

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

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

yaustar

Approved with comment

yaustar

Approved with comment

willeastcott · 2023-05-11T12:49:16Z

src/platform/input/mouse.js


        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);


Shouldn't this be mouseenter?

Wow, nice catch, absolutely and thank you!

Co-authored-by: Steven <yaustar@users.noreply.github.com>

kungfooman · 2023-05-11T14:16:09Z

It works fine if the active window is not another Chrome window.

Did you test that on Linux? I tried with another tab of Chrome and I couldn't trigger the bug either. Also tried it with other apps like KolourPaint and it worked nicely.

So far I believe we could just point out it only happens on Chrome/Mac, which should be fixed in Chrome/Mac's OS code (until someone brings this issue to some Mac developer attention).

yaustar · 2023-05-11T15:04:15Z

Did you test that on Linux? I tried with another tab of Chrome and I couldn't trigger the bug either. Also tried it with other apps like KolourPaint and it worked nicely.

On have access to Mac unfortunately 😅 Either way, I don't think it's a blocker for the PR

willeastcott · 2023-05-15T14:09:50Z

src/platform/input/constants.js

+export const EVENT_MOUSEOUT = 'mouseout';
+
+/**
+ * Name of event fired when the mouse moves out or enters another DOM element.


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

src/platform/input/constants.js

mvaligursky · 2023-07-07T14:45:19Z

@kungfooman - any thoughts on the last comments here?

Co-authored-by: Will Eastcott <will@playcanvas.com>

kungfooman · 2023-07-07T15:44:29Z

Yea, it's kinda confusing, because mouseout triggers when entering other elements:

However, good catch @willeastcott, because the EVENT_MOUSEENTER description should be more specific.

I will rewrite it to make it clearer, also happy about suggestions.

kungfooman · 2023-07-07T16:08:22Z

Looking like this in VSCode:

Add/implement pc.EVENT_MOUSEOUT

7c582ab

yaustar self-assigned this Dec 12, 2022

mvaligursky added the area: input Input related issue label Dec 13, 2022

mvaligursky requested review from mvaligursky, slimbuck and yaustar December 13, 2022 09:51

slimbuck reviewed Dec 13, 2022

View reviewed changes

yaustar reviewed Dec 13, 2022

View reviewed changes

src/platform/input/constants.js Outdated Show resolved Hide resolved

Update src/platform/input/constants.js: making it clear that we aren'…

d8adc32

…t talking about pc Elements Co-authored-by: Steven <yaustar@users.noreply.github.com>

Add EVENT_MOUSEENTER and rework to use Mouse#_target

10d3655

kungfooman changed the title ~~Add/implement pc.EVENT_MOUSEOUT~~ Add/implement pc.EVENT_MOUSEOUT and pc.EVENT_MOUSEENTER Dec 14, 2022

kungfooman added 2 commits December 14, 2022 17:27

Fix types

a2c8c28

Fix lint

3faf2ba

yaustar added requires: discussion It isn't clear what needs to be done feature request labels Jan 6, 2023

yaustar reviewed May 11, 2023

View reviewed changes

yaustar approved these changes May 11, 2023

View reviewed changes

willeastcott reviewed May 11, 2023

View reviewed changes

kungfooman and others added 3 commits May 11, 2023 15:13

Mouse#detach: fix mousein -> mouseenter ("opposite" of Mouse#attach)

6386212

Update src/platform/input/constants.js

3ba5cc3

Co-authored-by: Steven <yaustar@users.noreply.github.com>

@yaustar's suggestion with < 100 chars, GitHub is 500'ing

2395405

GSterbrant added the open source contribution Issues related to contributions from people outside the PlayCanvas team label May 12, 2023

willeastcott reviewed May 15, 2023

View reviewed changes

src/platform/input/constants.js Outdated Show resolved Hide resolved

Update src/platform/input/constants.js

6875316

Co-authored-by: Will Eastcott <will@playcanvas.com>

JSDoc retexting

f7dbb3b

kungfooman and others added 2 commits July 8, 2023 15:37

Fix lint

d7233cb

Merge branch 'main' into eventMouseOut

5912854

	var onMouseOut = function (e) {
	self.onMouseOut(e);
	};

	this.app.mouse.on(pc.EVENT_MOUSEDOWN, this.onMouseDown, this);
	this.app.mouse.on(pc.EVENT_MOUSEUP, this.onMouseUp, this);
	this.app.mouse.on(pc.EVENT_MOUSEMOVE, this.onMouseMove, this);
	this.app.mouse.on(pc.EVENT_MOUSEWHEEL, this.onMouseWheel, this);

	// Listen to when the mouse travels out of the window
	window.addEventListener('mouseout', onMouseOut, false);

	* 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()}.

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

Are you sure you want to change the base?

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

Conversation

kungfooman commented Dec 10, 2022 • edited by yaustar

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaustar commented Dec 13, 2022

kungfooman commented Dec 13, 2022

yaustar commented Dec 13, 2022 • edited

kungfooman commented Dec 13, 2022

yaustar commented Dec 13, 2022

kungfooman commented Dec 13, 2022

yaustar commented Dec 13, 2022

kungfooman commented Dec 13, 2022

yaustar commented Dec 13, 2022

kungfooman commented Dec 13, 2022

yaustar commented Dec 22, 2022

kungfooman commented Dec 22, 2022 • edited

yaustar commented Dec 23, 2022

kungfooman commented Dec 24, 2022

yaustar commented Jan 5, 2023 • edited

LeXXik commented Jan 5, 2023

kungfooman commented Jan 5, 2023

yaustar commented Jan 5, 2023

kungfooman commented Jan 5, 2023

yaustar commented Jan 6, 2023 • edited

kungfooman commented Jan 6, 2023

yaustar commented Jan 6, 2023

kungfooman commented Jan 6, 2023

yaustar commented Jan 6, 2023

yaustar commented May 11, 2023

yaustar May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaustar May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaustar left a comment

Choose a reason for hiding this comment

yaustar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kungfooman commented May 11, 2023

yaustar commented May 11, 2023

Choose a reason for hiding this comment

mvaligursky commented Jul 7, 2023

kungfooman commented Jul 7, 2023

kungfooman commented Jul 7, 2023

kungfooman commented Dec 10, 2022 •

edited by yaustar

yaustar commented Dec 13, 2022 •

edited

kungfooman commented Dec 22, 2022 •

edited

yaustar commented Jan 5, 2023 •

edited

yaustar commented Jan 6, 2023 •

edited

yaustar May 11, 2023 •

edited

yaustar May 11, 2023 •

edited