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

Add CSSOM View extensions to MouseEvent #3484

Merged
merged 4 commits into from
Jan 10, 2023
Merged

Add CSSOM View extensions to MouseEvent #3484

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
4950be9
Fix: Add support to alias x and y to MouseEvent
ViniciusFXavier Dec 17, 2022
519f4fd
Use getters for x/y, since initMouseEvent is deprecated
jenseng Jan 5, 2023
6a99694
Add CSSOM View extensions to MouseEvent
jenseng Jan 5, 2023
94eaa7b
Address PR feedback
jenseng Jan 9, 2023
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
34 changes: 34 additions & 0 deletions lib/jsdom/living/events/MouseEvent-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,42 @@ const EventModifierMixinImpl = require("./EventModifierMixin-impl").implementati
const UIEventImpl = require("./UIEvent-impl").implementation;

const MouseEventInit = require("../generated/MouseEventInit");
const { wrapperForImpl } = require("../generated/utils");

class MouseEventImpl extends UIEventImpl {
get x() {
return this.clientX;
}
get y() {
return this.clientY;
}
get pageX() {
if (this._dispatchFlag) {
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@jenseng @domenic I'm wondering why pageX returns 0 here when _dispatchFlag is true. This behavior leads to wrong results when accessing the event in a listener:

document.body.addEventListener('mousemove', e => {
  console.log('event', e.pageX, e.pageY);
});
const ev = new MouseEvent('mousemove', { clientX: 10, clientY: 10 });
document.body.dispatchEvent(ev);

Running this in a JSDOM environment logs event 0 0 because the pageX getter is called inside a dispatch.

On the other hand, running the same code in Chrome logs the correct event 10 10.

I see that the spec says things like "if the event’s dispatch flag is set, return A, otherwise return B", but that doesn't mean 0 is the right value. It should still be the coordinate of the event relative to some origin.

Looking at how Chrome handles this, I see that pageX is

  1. Initialized to clientX
  2. Incremented by a scrollX offset if available.

There is no mention of the dispatch flag, at least not explicitly. That leaves me with the conclusion that JSDOM shouldn't implement any special behavior when _dispatchFlag is true, always returning clientX + scrollX is fine.

This is quite a serious bug when JSDOM is used with Jest and React Testing Library, as it breaks unit tests that create MouseEvents to simulate moving the mouse.

I discovered this when trying out Jest upgraded to JSDOM 21 (jestjs/jest#13825) to run unit tests of WordPress Gutenberg project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsnajdr when implementing this, my goal was to:

  1. Follow the spec as closely as possible, and
  2. Return something reasonable where we can't fully implement it, i.e. since jsdom doesn't implement layout/rendering, some things are unknowable so perhaps 0 is the least bad option

A (minor) risk with returning the current clientX + scrollX is that it could be inconsistent with browsers, since scrollX/Y could change between the time of event initialization and when pageX/pageY are read during dispatch (e.g. if explicitly set or scrollTo is called during an earlier click handler).

That said, it's probably still a better default than zero, and assuming scrollX/Y hasn't changed, it should be synonymous with the position where the event occurred relative to the origin of the initial containing block.

@domenic wdyt? Seems like a reasonable change to me, though perhaps it's still a good idea to keep the dispatch branch even if the return value is the same. That way it's a good placeholder/indicator for when layout support is added.

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 do have an existing PR that is closely related to this, and I think we can solve the minor risk outlined above by setting the new _initialContainingBlockRelativeX/Y to clientX/Y + scrollX/Y.

That way the values will be persisted at init time, and not risk changing if scrollX/Y changes later before/during dispatch.

Maybe I'll just split it out into its own PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3514 should resolve this

const offset = wrapperForImpl(this.view)?.scrollX || 0;
return offset + this.clientX;
}
get pageY() {
if (this._dispatchFlag) {
return 0;
}
const offset = wrapperForImpl(this.view)?.scrollY || 0;
return offset + this.clientY;
}
get offsetX() {
Copy link
Member

Choose a reason for hiding this comment

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

Per spec these should be the same as pageX/pageY. A test, or enabling of an existing test, would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (this._dispatchFlag) {
return 0;
}
return this.pageX;
}
get offsetY() {
if (this._dispatchFlag) {
return 0;
}
return this.pageY;
}

Copy link
Member

Choose a reason for hiding this comment

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

You're missing getters for pageX and pageY. See how they are defined in steps 2-3 of https://drafts.csswg.org/cssom-view/#dom-mouseevent-pagex .

Copy link
Contributor Author

@jenseng jenseng Jan 9, 2023 •
edited
Loading

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 the getters are actually needed, since (i think) by default the various impl properties are just regular properties (e.g. see clientX usage above, which works and doesn't have an explicit getter) Nevermind, I overlooked that bit of logic in the spec, I'll add them 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, and added a test

initMouseEvent(
type,
bubbles,
Expand Down
25 changes: 24 additions & 1 deletion lib/jsdom/living/events/MouseEvent.webidl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ interface MouseEvent : UIEvent {
readonly attribute long screenY;
readonly attribute long clientX;
readonly attribute long clientY;

readonly attribute boolean ctrlKey;
readonly attribute boolean shiftKey;
readonly attribute boolean altKey;
Expand Down Expand Up @@ -52,3 +51,27 @@ partial interface MouseEvent {
optional short buttonArg = 0,
optional EventTarget? relatedTargetArg = null);
};

// https://drafts.csswg.org/cssom-view/#extensions-to-the-mouseevent-interface
// Adds attributes and changes existing ones to doubles
partial interface MouseEvent {
readonly attribute double screenX;
readonly attribute double screenY;
readonly attribute double pageX;
readonly attribute double pageY;
readonly attribute double clientX;
readonly attribute double clientY;
readonly attribute double x;
readonly attribute double y;
readonly attribute double offsetX;
readonly attribute double offsetY;
};

// https://drafts.csswg.org/cssom-view/#extensions-to-the-mouseevent-interface
// Changes existing coordinate entries to doubles
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 how well this works in webidl2js. I guess it didn't break the build, which is good, but does it properly "override" the existing ones?

I think before merging, I need a test to ensure that these correctly come out as doubles instead of longs. So, construct a new MouseEvent with its screenX/screenY/clientX/clientY constructor options set to something fractional (e.g. 1.5), and then read off the results from the object and ensure they're still fractional.

Bonus points if you add a test that using initMouseEvent does not preserve the fractionalness, since the arguments are still defined to be longs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, it seems that although major browsers (chromium/firefox/webkit) have implemented these MouseEvent properties, they still convert doubles to longs (possibly for legacy compatibility reasons?) I don't know if that means jsdom shouldn't support doubles, but perhaps something to think about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests around this

partial dictionary MouseEventInit {
double screenX = 0.0;
double screenY = 0.0;
double clientX = 0.0;
double clientY = 0.0;
};
39 changes: 39 additions & 0 deletions test/web-platform-tests/to-upstream/html/editing/activation/click-event-properties.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@
assert_equals(e.screenY, 0);
assert_equals(e.clientX, 0);
assert_equals(e.clientY, 0);
assert_equals(e.x, 0);
assert_equals(e.y, 0);
assert_equals(e.pageX, 0);
assert_equals(e.pageY, 0);
assert_equals(e.offsetX, 0);
assert_equals(e.offsetY, 0);

assert_equals(e.button, 0);
assert_equals(e.buttons, 0);
Expand All @@ -84,4 +90,37 @@

}, "clicking using click() should produce an event object with the correct MouseEventInit values");

test(() => {
window.scrollX = 10;
window.scrollY = 20;
const e = new MouseEvent("click", {
clientX: 1, clientY: 2, view: window
});
assert_equals(e.x, 1);
assert_equals(e.y, 2);
assert_equals(e.pageX, 11);
assert_equals(e.pageY, 22);
assert_equals(e.offsetX, 11);
assert_equals(e.offsetY, 22);
}, "MouseEvent should provide the correct computed values when the dispatch flag is not set");

test(() => {
const e = new MouseEvent("click", {
screenX: 1.5, screenY: 2.5, clientX: 3.5, clientY: 4.5
});
assert_equals(e.screenX, 1.5);
assert_equals(e.screenY, 2.5);
assert_equals(e.clientX, 3.5);
assert_equals(e.clientY, 4.5);
}, "MouseEvent.constructor should preserve doubles");

test(() => {
const e = new MouseEvent("click");
e.initMouseEvent("click", true, true, null, null, 1.5, 2.5, 3.5, 4.5);
assert_equals(e.screenX, 1);
assert_equals(e.screenY, 2);
assert_equals(e.clientX, 3);
assert_equals(e.clientY, 4);

}, "MouseEvent.initMouseEvent should convert doubles to longs");
</script>