MouseEvent: fix `pageX/Y` and `offsetX/Y` during dispatch by jenseng · Pull Request #3514 · jsdom/jsdom

jenseng · 2023-03-02T20:46:57Z

Ensure that pageX/Y reflect the coordinate relative to the origin of the document (i.e. clientX/Y in the absence of scrolling support) during dispatch.

Also fix the target-relative coordinate (offsetX/Y) during dispatch. This will typically be (0, 0) (e.g. in a synthetic click or a dispatched MouseEvent with default coordinates), but it will mirror clientX/Y if those are set.

Lastly, remove some scrollX/Y logic for values computed outside of dispatch. While it logically followed the spec, scrolling is not implemented and it is unsafe to base the calculation on those replaceable properties.

cc @jsnajdr

See also:

lib/jsdom/living/events/MouseEvent-impl.js

test/web-platform-tests/to-upstream/html/editing/activation/click-event-properties.html

jenseng · 2023-03-06T19:19:53Z

Filed this CSSOM View issue to address the issues around synthetic pointer events

jenseng · 2023-03-09T19:40:20Z

The second commit:

Defaults offsetX/Y to clientX/Y - domRect.x/y rather than 0. Although the values will still be zero in many cases (e.g. synthetic click, or a dispatched event w/ zeroed clientX/Y), this makes the values more useful as discussed earlier in the PR (e.g. when testing mouse movement across events). By including the target's domRect in the calculations, this also means it will also return the right values if/when jsdom adds full layout support.
Splits out some of the new and existing tests into the dontupstream variant, as explained in a comment in that test
Reworks the timing around when we finalize the event position for pageX/Y and offsetX/Y during dispatch. Rather than do it at init when we don't know what the target will be, we (re)calculate them right when the target is set (just before the bubble and cancel phases)

On a related note, I opened a wpt PR to expand the CSSOM View MouseEvent tests. There's a bit of overlap with the tests in this PR, but we can't use the wpt ones until JSDOM implements scroll, layout, etc., so for now it's probably good to have them in both places.

domenic

Thanks for all the iteration here, and sorry I haven't had time to participate in the discussion until now!

My issue is that I'm really uncomfortable with how window.scrollX and scrollY setters appear to be a key part of the story here. I'd like us to make those readonly, or at the very least assume they never change and not write tests which rely on changing them.

Where does that leave us?

...-platform-tests/to-upstream/html/editing/activation/click-event-properties-dontupstream.html

domenic · 2023-03-12T07:27:31Z

lib/jsdom/living/events/MouseEvent-impl.js

    return this.pageY;
  }

+  /** @override */


Please avoid jsdoc here and elsewhere

Would it be worth spelling this out in Contributing.md or elsewhere? Seems like there's some existing jsdoc comments/tags in the codebase

jenseng · 2023-03-12T20:20:46Z

Yeah that sounds reasonable, just two comments:

Even if the tests don't do anything w/ scrollX/Y, I think it's important for the implementation to still read them (as well as getBoundingClientRect). That way if/when scrolling and layout are implemented, the code is already correct and it's just a matter of enabling some upstream WPTs.
Any concerns around making scrollX/Y without also implementing scroll/scrollBy/etc at the same time? Seems like there could be jsdom users in the wild that are relying on being able to set those in the absence of scrolling support.

Ensure that `pageX/Y` reflect the coordinate relative to the origin of the window at the time of init (i.e. `clientX/Y` plus `scrollX/Y`). We record it during init, since `scrollX/Y` may change, which could result in the wrong `pageX/Y` values; see added tuWPT. Also track the target-relative coordinate during init (still just `(0, 0)` in the absence of layout support). This doesn't fundamentally change any behavior, but makes the implementation more future proof, making it safer to start handling Image Button submitters when constructing the form data set; see jsdom#3496 where this code was extracted from.

jsnajdr · 2023-03-13T09:37:17Z

lib/jsdom/living/events/MouseEvent-impl.js

+  }
+
+  set target(target) {
+    super.target = target;


Another way to implement this, maybe more elegant, would be a _receivedTarget method on the parent EventImpl class:

class EventImpl { set target(target) { this._target = target; this._receivedTarget(); } _receivedTarget() {} } class MouseEventImpl { _receivedTarget() { // calculate offsets from this.target } }

Chrome's Event implementation also has a ::ReceivedTarget method, and JSDOM also has similar prior art in the _activationBehavior() method on elements.

jsnajdr · 2023-03-13T09:49:06Z

Any concerns around making scrollX/Y without also implementing scroll/scrollBy/etc at the same time?

In a real browser, setting window.scrollX is possible, but it won't affect the scroll position at all. The scrollX position is [Replaceable] which means that you can set it, but then it will be just another custom property of window that will shadow the internal one.

Compliant code would have access to the window's internal _scrollX value (which window exposes through a replaceable scrollX getter), and there would be scroll/scrollBy methods that set that internal value. scroll/scrollBy implementation would also have to fire the scroll events on all expected targets (document, window, ...).

If we want to avoid snowballing this PR too much, I think we'll either need to accept the incorrect window.scrollX = setting for now, or to give up reading the scroll position at all, returning raw clientX as pageX, without any adjustment.

jsnajdr · 2023-03-29T10:48:15Z

Hi @domenic 👋 what are you thoughts about what needs to be done to move this PR forward? There were concerns about setting window.scrollX in tests, but if we don't want to implement full window.scrollBy including scroll and scrollend events, at least not right now, does that leave us with any other options?

domenic · 2023-04-05T01:46:36Z

I admit I've lost the plot on what we're trying to accomplish here. This thread is too long and this PR contains many changes which seem to be about future-proofing for some hypothetical future where we support layout or scrolling, which makes it hard to understand. The WPT added here does not pass in Firefox or Chrome: https://jsbin.com/sexirixixo/edit?html,output which maybe is a spec problem, maybe is a test problem.

I tried setting out guidelines I'd like us to follow about, but perhaps they were too implicit. The guidelines I'd like are:

Web platform tests should only use standardized functionality, not anything nonstandard like modifiable scrollX properties.
Web platform tests should be written such that:
- If all browsers agree, they should match that behavior. (If the spec disagrees with browser consensus, file a spec bug, but keep the tests aligned with browser behavior.)
- If not all browsers agree, and at least one of them matches the spec, then follow the spec behavior.
- If no browsers agree with the spec and at least two agree with each other, follow the two-browser behavior and file a spec bug.
- If no browsers agree with each other and there's a fourth behavior in the spec, we can't handle this on the jsdom level, and need to escalate this further in the standards process before proceeding.
A change will be accepted to jsdom if it improves our WPT pass rate, including newly-added WPTs that follow the above guidelines.
A change will be much easier for me to review if there's a 1:1 correspondence with the spec.
However, putting in dead code that only matters in some future where we support scrolling or layout, is distracting and problematic, and I'd rather we just add a comment saying why a certain spec line is being omitted.

I hope that helps... I think the current PR does not meet these criteria because the WPT that was added fails in two browsers, but admittedly I haven't tested Safari, so maybe it matches the spec and Safari. So that's the immediate problem. After that, I'd encourage getting the PR into shape such that it's easy to review, per the above guidelines.

jenseng · 2023-04-05T02:54:42Z

All fair points, I'll clean this up and remove scrollX/Y stuff in the code in favor of comments. Upstream wpts should have this covered for if/when jsdom ever implements scrolling/layout, at which point the implementation could be fleshed out and comments removed.

As for the test, there's a dumb oversight on my part, as it can be made to pass in browsers and jsdom. I wrote it with jsdom in mind, so I didn't consider the body margin and button border, which makes it fail in browsers 🙃 ... the test passes if the event is dispatched against window or some other border-less element at the page origin.

jenseng · 2023-04-05T03:42:50Z

Ok latest commit hopefully addresses everything. I also updated the PR summary to reflect the current state of things 🤞

lib/jsdom/living/events/MouseEvent-impl.js

jsnajdr · 2023-04-11T12:46:19Z

Thanks @jenseng for greatly simplifying the PR. I'm confirming that it addresses the main concern I had: when I create a MouseEvent by calling a constructor:

e = new MouseEvent('mousedown', { clientX: 10, clientY: 20 });

the clientX and clientY values are propagated to the return values of the e.pageX and e.offsetX getters, even during dispatch. Before this PR, the getters returned 0 during dispatch, ignoring the constructor parameters.

domenic

Thanks for your patience here!

jenseng marked this pull request as ready for review March 2, 2023 21:21

jenseng mentioned this pull request Mar 2, 2023

Add CSSOM View extensions to MouseEvent #3484

Merged

jsnajdr reviewed Mar 3, 2023

View reviewed changes

jenseng force-pushed the cssom-coordinate-improvements branch from f76a390 to 2db706a Compare March 9, 2023 19:29

jenseng changed the title ~~MouseEvent: fix pageX/Y during dispatch~~ MouseEvent: fix pageX/Y and offsetX/Y during dispatch Mar 9, 2023

domenic reviewed Mar 12, 2023

View reviewed changes

jenseng added 3 commits March 12, 2023 14:46

Add tests, address PR feedback, refactor

f183b3d

Remove scrollX/Y setting from tests

263d7b2

jenseng force-pushed the cssom-coordinate-improvements branch from 2db706a to 263d7b2 Compare March 12, 2023 20:46

jsnajdr approved these changes Mar 13, 2023

View reviewed changes

Incorporate _receivedTarget suggestion

5248f0b

Address PR feedback

795f81e

jenseng commented Apr 5, 2023

View reviewed changes

lib/jsdom/living/events/MouseEvent-impl.js Outdated Show resolved Hide resolved

Tweak comments

9fd7bc4

domenic approved these changes Apr 16, 2023

View reviewed changes

domenic merged commit 12a24a9 into jsdom:master Apr 16, 2023

jsnajdr mentioned this pull request Apr 17, 2023

feat: upgrade to JSDOM@22 jestjs/jest#13825

Merged

jenseng mentioned this pull request Apr 18, 2023

fix(react-router-dom): fix submitter serialization remix-run/react-router#9865

Merged

Uh oh!

Conversation

jenseng commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jenseng commented Mar 6, 2023

Uh oh!

jenseng commented Mar 9, 2023

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domenic Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

jenseng Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

jenseng commented Mar 12, 2023

Uh oh!

jsnajdr Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsnajdr commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsnajdr commented Mar 29, 2023

Uh oh!

domenic commented Apr 5, 2023

Uh oh!

jenseng commented Apr 5, 2023

Uh oh!

jenseng commented Apr 5, 2023

Uh oh!

Uh oh!

jsnajdr commented Apr 11, 2023

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jenseng commented Mar 2, 2023 •

edited

Loading

jsnajdr Mar 13, 2023 •

edited

Loading

jsnajdr commented Mar 13, 2023 •

edited

Loading