MouseEvent: fix pageX/Y and offsetX/Y during dispatch

[jenseng]

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:

• Add CSSOM View extensions to MouseEvent #3484 (comment)
• feat: upgrade to JSDOM@22 jestjs/jest#13825

[jenseng]

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

[jenseng]

The second commit:

1. 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.
2. Splits out some of the new and existing tests into the dontupstream variant, as explained in a comment in that test
3. 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?

[domenic]

Please avoid jsdoc here and elsewhere

[jenseng]

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]

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.

[jsnajdr]

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]

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]

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]

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]

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]

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

[jsnajdr]

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!