1680669 - New wpt failures in /pointerevents/pointerevent_fractional_coordinates.html

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Syncing wpt PR 26728 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

/pointerevents/pointerevent_fractional_coordinates.html: ERROR

CI Results

Missing results from treeherder
GitHub PR Head

Notes

These updates will be on mozilla-central once bug 1680370 lands.

Note: this bug is for tracking fixing the issues and is not
owned by the wpt sync bot.

This bug is linked to the relevant tests by an annotation in
https://github.com/web-platform-tests/wpt-metadata. These annotations
can be edited using the wpt interop dashboard
https://jgraham.github.io/wptdash/

If this bug is split into multiple bugs, please also update the
annotations, otherwise we are unable to track which wpt issues are
already triaged. Resolving as duplicate or closing this issue should
be cause the bot to automatically update or remove the annotation.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It seems that there is no way to make the type switchable in our WebIDL.

So, I wonder, I should change related event types directly to double. Then, at first step, they return integral values. Finally, I add a pref to make them return fractional values. Smaug, what do you think these steps to fix this bug?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, CSSOM View suggests that these values types should be changed too...

• https://www.w3.org/TR/cssom-view-1/#extensions-to-the-window-interface
• https://www.w3.org/TR/cssom-view-1/#extensions-to-the-document-interface
• https://www.w3.org/TR/cssom-view-1/#extension-to-the-element-interface

Changing only MouseEvent and PointerEvent attributes' types may break comparison with them. But we've already done...

• https://searchfox.org/mozilla-central/rev/89fa43ebb52249863ada27fa5fd67a65bd73d299/dom/webidl/Window.webidl#335-352
• https://searchfox.org/mozilla-central/rev/89fa43ebb52249863ada27fa5fd67a65bd73d299/dom/webidl/DocumentOrShadowRoot.webidl#13-23
• https://searchfox.org/mozilla-central/rev/89fa43ebb52249863ada27fa5fd67a65bd73d299/dom/webidl/Element.webidl#225-232

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Changing type to double but still internally use integer (at least for trusted events) sounds reasonable.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, as far as I've investigated the expected results of WPT, trusted events should return integer values in most cases. Only the exception is, transform changed the coordinates. So, this may be safer than what I've thought.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Coming patches do not have performance regressions:
https://perf.compare/compare-results?baseRev=c01ce29fce0751ffd960b8aff1635108f2e63089&newRev=ca3e23095f4a5f1eb93c3b09959b4b0b2e77212b&baseRepo=try&newRepo=try&framework=13

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1680669 - part 1: Move some unnecessary stuff in `UIEvent` to `MouseEvent` r=smaug!,#dom-core

mDefaultClientPoint, mMovementPoint and mPagePoint are used only in
MouseEvent. Therefore, they can be moved to MouseEvent.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1680669 - part 2: Make point of `CSSPixel` and `LayoutDevicePixel` work within `double` coordinates r=botond!

MouseEvent and its subclasses need to treat screen/client coordinates with
double variables. Although for the backward compatibility, we'll keep
rounding the value to integer, but we need to support lossless untrusted
event creation. Therefore, they need to store the points with the new point
types.

Depends on D222723

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1680669 - part 3: Add a utility method to `WidgetEvent` to check whether the corresponding DOM event supports coordinates r=smaug!,#dom-core

Depends on D222724

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1680669 - part 4: Make `EventUtils.js` round X/Y coordinates when it calls `nsIDOMWindowUtils` methods r=smaug!,emilio!,#dom-core

Currently, they use given values or computed values as-is. Therefore, if the
values are fractional values, they are truncated when calling
nsIDOMWindowUtils methods. However, in the most cases, they should be
rounded instead.

Unfortunately, correcting this issue causes that synthesizeNativeMouseEvent
on macOS may cause a mouse event 1-pixel below than expected if offset in an
element is specified. It seems that there is a hidden bug of converting
coordinates somewhere, however, I couldn't find it and this occurs only with
synthesizing native mouse events according to the test results. So, this does
not affect to most tests (no web-platform tests). Let's make this in the
backlog for now.

Depends on D222725

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1680669 - part 5: Make `MouseEvent` and subclasses work with `double` coordinates r=smaug!,#dom-core

This patch makes the following patch simpler. Therefore, this patch just cast
double coordinates to int32_t points at initializing untrusted events.
Therefore, this does not change any behavior.

Depends on D222726

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1680669 - part 6: Make methods of `MouseEvent` and static methods of `Event` aware of fractional coordinates r=smaug!,emilio!,#dom-core

screenX, screenY, clientX, clientY, x, y, offsetX and offsetY
are now double. This patch makes the event classes aware of DOM events.

Note that screenX and screenY are always integers if the event is trusted
one because WidgetEvent::mRefPoint is LayoutDeviceIntPoint. The other
properties of trusted events may return fractional values.

Currently, Chrome allows fractional values only for pointer events whose type
is not click, auxclick nor contextmenu (this exception is defined by the
Pointer Events). Therefore, this patch does not replace MouseEvent::*Point()
methods with new ones, i.e., keeping the traditional methods to keep the
backward compatibility completely.

I'm not sure about the untrusted events of non-pointer events, click,
auxclick and contextmenu. Chrome does check whether it's trusted or not
before applying std::floor or std::round at getter methods (I think that
using std::round in offsetX and offsetY is their bug at least for
untrusted events). This patch follows them for now but has a pref to allow
for all untrusted events.

Finally, this patch does NOT make the computation completely lossless. I
tried it first, but that causes most pointer event coordinates became fractional
values but such behavior is not expected by webdriver tests in WPT nor some
mochitests. WPTs expect that fractional values are used only when the pointer
event target is on an element which is transformed by CSS. Therefore, this
patch keeps using integer points if a trusted pointer event which is not
click, auxclick nor contextmenu and:

• the document is transformed or zoomed in the parent document or,
• the PresShell or DeviceContext is zoomed or,
• an inclusive ancestor of the primary frame of the event target is transformed
  only when computing offsetX or offsetY

So, if the root frame of the PresShell or the primary frame of the event
target is not transformed nor scaled by any features/settings, clientX,
clientY, pageX, pageY, offsetX and offsetY keep returning integer.

Depends on D222727

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1680669 - part 7: Change the type of `width` and `height` of `PointerEvent` to `double` r=smaug!,#dom-core

The values may be used with the coords of MouseEvent. Therefore, they need
to be double right now.

Depends on D222728

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff5b928a9e88
part 1: Move some unnecessary stuff in `UIEvent` to `MouseEvent` r=smaug
https://hg.mozilla.org/integration/autoland/rev/edde7fe51b4b
part 2: Make point of `CSSPixel` and `LayoutDevicePixel` work within `double` coordinates r=botond
https://hg.mozilla.org/integration/autoland/rev/aefcf10217ab
part 3: Add a utility method to `WidgetEvent` to check whether the corresponding DOM event supports coordinates r=smaug
https://hg.mozilla.org/integration/autoland/rev/b7db9afaf0d0
part 4: Make `EventUtils.js` round X/Y coordinates when it calls `nsIDOMWindowUtils` methods r=smaug,emilio
https://hg.mozilla.org/integration/autoland/rev/98d5a05c6c6e
part 5: Make `MouseEvent` and subclasses work with `double` coordinates r=smaug
https://hg.mozilla.org/integration/autoland/rev/0ea945553f63
part 6: Make methods of `MouseEvent` and static methods of `Event` aware of fractional coordinates r=smaug,emilio,webdriver-reviewers
https://hg.mozilla.org/integration/autoland/rev/aad5ccb93e0b
part 7: Change the type of `width` and `height` of `PointerEvent` to `double` r=smaug

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/49488 for changes under testing/web-platform/tests

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ff5b928a9e88
https://hg.mozilla.org/mozilla-central/rev/edde7fe51b4b
https://hg.mozilla.org/mozilla-central/rev/aefcf10217ab
https://hg.mozilla.org/mozilla-central/rev/b7db9afaf0d0
https://hg.mozilla.org/mozilla-central/rev/98d5a05c6c6e
https://hg.mozilla.org/mozilla-central/rev/0ea945553f63
https://hg.mozilla.org/mozilla-central/rev/aad5ccb93e0b

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot