|
|
DescriptionMake PointerEvent coordinates fractional for touch
The current PointerEvent spec doesn't override the UI
MouseEvent spec definition of (clientX, clientY) and
(screenX, screenY) coordinates: all of them are integers.
This is not great for the sites that currently use Touch
Events & trying to switch to Pointer Events for touch:
specially on high DPI devices Pointer Event coordinates
would feel less precise than fractional Touch Event
coodinates. The PEWG has already reached consensus that
all coordinates in Pointer Events should be floating
point values:
https://github.com/w3c/pointerevents/issues/107
Edge already reports fractional client coordinates for
PointerEvents (and not MouseEvents).
This CL exposes fractional Pointer Event coordinates for
touch input. Although the CL changes the client & screen
coordinates in Mouse Event idl to floating points, the
integer plumbing for mouse is kept intact so that mouse
coordinates still appear unchanged to JS. Pointer Events
that are fired for touches propagate appropriate
fractional coordinates to JS.
Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/1kroHAnh3BU/uYZb6uZRBwAJ
BUG=607948
Committed: https://crrev.com/48a106897274196b5897f1b0967d28c49e4ec696
Cr-Commit-Position: refs/heads/master@{#432473}
Patch Set 1 #Patch Set 2 : Updated a layout test. #Patch Set 3 : Fixed zoom issues #
Total comments: 2
Patch Set 4 : Fixed layout tests. #Patch Set 5 : Updated a TODO. #
Total comments: 6
Patch Set 6 : Made mouse page & scroll coodinates fractional, updated tests. #Patch Set 7 : Reverted Init type back to int. Updated tests. #Patch Set 8 : Fixed layout tests again. #Patch Set 9 : Back to double type in MouseEventInit #Patch Set 10 : Moved all coords to DoublePoints #Patch Set 11 : Fixed test values in {mouse,wheel}-event-constructor.html. Rebased. #Patch Set 12 : Fixed test values in {mouse,wheel}-event-constructor.html. Rebased. #
Total comments: 4
Patch Set 13 : DoublePoint -> FloatPoint #Patch Set 14 : FloatPoint -> DoublePoint again. #Patch Set 15 : Rebased. #Messages
Total messages: 69 (34 generated)
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Switched MouseEvent coords to floating points. BUG=607948 ========== to ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged from JS. Pointer Events that are fired for touches propagate appropriate fractional values to JS. BUG=607948 ==========
Description was changed from ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged from JS. Pointer Events that are fired for touches propagate appropriate fractional values to JS. BUG=607948 ========== to ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional values to JS. BUG=607948 ==========
Description was changed from ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional values to JS. BUG=607948 ========== to ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional coordinates to JS. BUG=607948 ==========
mustaq@chromium.org changed reviewers: + rbyers@chromium.org
ptal for early comments. This is working fine, so want to double-check if exposed fractional ME coordinates could be ignored given that clientXY-in-zoom-and-scroll.html is failing.
Seems mostly reasonable to me. I think the key outstanding question to answer is whether or not this change is exposed to JavaScript-generated MouseEvents. I.e. what does '(new MouseEvent('mousedown', {clientX=1.5})).clientX' evaluate to? I assumed we'd want it to be (and so you'd get 1.5), but it looks like this CL doesn't do that. The CSS OM View spec indicates that should work, but we should check other browsers. Making this change would be the most logical path IMHO, but might have some additional compat risk (though it seems unlikely to me). And it'll need an "intent to ship" though I expect it won't be at all contentious (as long as UA-generated mouse events are guaranteed to only ever have integer co-ordinates). Going the more conservative route and explicitly truncating the return values for any MouseEvents which aren't PointerEvents is also an option, but seems like a hack to me. If we're going to go that route I think we should have a good reason why it's preferable for now. https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/MouseRelatedEvent.h:47: return isPointerEvent() ? m_screenLocation.x().toDouble() It seems odd that this pattern of conditional return values is necessary. Can't you just initialize the event in the first place to truncate to integers for mouse events but not for pointer events?
On 2016/10/12 14:09:54, Rick Byers wrote: > Seems mostly reasonable to me. > > I think the key outstanding question to answer is whether or not this change is > exposed to JavaScript-generated MouseEvents. I.e. what does '(new > MouseEvent('mousedown', {clientX=1.5})).clientX' evaluate to? I assumed we'd > want it to be (and so you'd get 1.5), but it looks like this CL doesn't do that. > The CSS OM View spec indicates that should work, but we should check other > browsers. Making this change would be the most logical path IMHO, but might > have some additional compat risk (though it seems unlikely to me). And it'll > need an "intent to ship" though I expect it won't be at all contentious (as long > as UA-generated mouse events are guaranteed to only ever have integer > co-ordinates). > > Going the more conservative route and explicitly truncating the return values > for any MouseEvents which aren't PointerEvents is also an option, but seems like > a hack to me. If we're going to go that route I think we should have a good > reason why it's preferable for now. > > https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:47: return > isPointerEvent() ? m_screenLocation.x().toDouble() > It seems odd that this pattern of conditional return values is necessary. Can't > you just initialize the event in the first place to truncate to integers for > mouse events but not for pointer events?
On 2016/10/12 14:14:29, dtapuska wrote: > On 2016/10/12 14:09:54, Rick Byers wrote: > > Seems mostly reasonable to me. > > > > I think the key outstanding question to answer is whether or not this change > is > > exposed to JavaScript-generated MouseEvents. I.e. what does '(new > > MouseEvent('mousedown', {clientX=1.5})).clientX' evaluate to? I assumed we'd > > want it to be (and so you'd get 1.5), but it looks like this CL doesn't do > that. > > The CSS OM View spec indicates that should work, but we should check other > > browsers. Making this change would be the most logical path IMHO, but might > > have some additional compat risk (though it seems unlikely to me). And it'll > > need an "intent to ship" though I expect it won't be at all contentious (as > long > > as UA-generated mouse events are guaranteed to only ever have integer > > co-ordinates). > > > > Going the more conservative route and explicitly truncating the return values > > for any MouseEvents which aren't PointerEvents is also an option, but seems > like > > a hack to me. If we're going to go that route I think we should have a good > > reason why it's preferable for now. > > > > > https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > > > > https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:47: return > > isPointerEvent() ? m_screenLocation.x().toDouble() > > It seems odd that this pattern of conditional return values is necessary. > Can't > > you just initialize the event in the first place to truncate to integers for > > mouse events but not for pointer events? there are a number of failures in the layout tests. We should try to figure out what other vendors do in these cases and update our layout tests. They seem mostly about the type conversion of a double->long.
ptal. rbyers@: Since we want to have touch fractional coordinates asap, we have to merge this change to stable (or even some beta if possible). We want to minimize changes related to MouseEvents here so that we can merge required PointerEvent changes quickly by avoiding approvals for MouseEvents. Moreover, if we truncate doubles for all non-PointerEvents, the compat risk is nearly zero. E.g. direct coordinate comparisons or array access through MouseEvent.clientX would still work if any site happen to rely on these. dtapuska@: Fixed all layout tests. The constructor tests were affected because of weird JS parsing rules for ints vs floats. Not a big deal---these tests are "overfitting" our implementation anyway. E.g. just discovered that eval("new MouseEvent('type', {screenX: 2147483647}).screenX"); is devtools returns 2147483647, not MAX_CLIENT_SIZE as in the test. https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/MouseRelatedEvent.h:47: return isPointerEvent() ? m_screenLocation.x().toDouble() On 2016/10/12 14:09:54, Rick Byers wrote: > It seems odd that this pattern of conditional return values is necessary. Can't > you just initialize the event in the first place to truncate to integers for > mouse events but not for pointer events? The problem here is that the initialization is done at MouseRelatedEvent constructors where the vtable for the actual subclass being constructed (e.g. MouseEvent or PointerEvent) is not "ready". As a result, all isXyzEvent() functions return false.
> dtapuska@: Fixed all layout tests. The constructor tests were affected because > of weird JS parsing rules for ints vs floats. Not a big deal---these tests are > "overfitting" our implementation anyway. E.g. just discovered that > eval("new MouseEvent('type', {screenX: 2147483647}).screenX"); > is devtools returns 2147483647, not MAX_CLIENT_SIZE as in the test. Correction: Ignore the "new discovery" above, I messed up client vs screen. But my main point is still valid: the test overfits our implementation. That's why eval("new MouseEvent('type', {clientX: 2147483647}).clientX") produces max LayoutUnit value 33554431.
On 2016/10/12 16:03:35, mustaq wrote: > > dtapuska@: Fixed all layout tests. The constructor tests were affected because > > of weird JS parsing rules for ints vs floats. Not a big deal---these tests are > > "overfitting" our implementation anyway. E.g. just discovered that > > eval("new MouseEvent('type', {screenX: 2147483647}).screenX"); > > is devtools returns 2147483647, not MAX_CLIENT_SIZE as in the test. > > Correction: Ignore the "new discovery" above, I messed up client vs screen. > > But my main point is still valid: the test overfits our implementation. That's > why > eval("new MouseEvent('type', {clientX: 2147483647}).clientX") > produces max LayoutUnit value 33554431. Seems FF still follows UI MouseEvent Spec for both clientX & screenX (emits the exact same output for screenX): new MouseEvent('eventType', { clientX: 2147483647 }).clientX; => 2147483647 new MouseEvent('eventType', { clientX: 4294967295 }).clientX; => -1 new MouseEvent('eventType', { clientX: 123.45 }).clientX; => 123 We have had a different clientX/Y for a while, I couldn't find/recall any bugs around it. So the compat risk is pretty low IMO.
> Seems FF still follows UI MouseEvent Spec for both clientX & screenX (emits the > exact same output for screenX): > new MouseEvent('eventType', { clientX: 2147483647 }).clientX; => 2147483647 > new MouseEvent('eventType', { clientX: 4294967295 }).clientX; => -1 > new MouseEvent('eventType', { clientX: 123.45 }).clientX; => 123 > > We have had a different clientX/Y for a while, I couldn't find/recall any bugs > around it. So the compat risk is pretty low IMO. And in Edge: new MouseEvent('eventType', { clientX: 2147483647 }).clientX; => -1 new MouseEvent('eventType', { clientX: 4294967295 }).clientX; => -1 new MouseEvent('eventType', { clientX: 123.45 }).clientX; => 123 There is no way we can make this test pass across browsers!
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/12 16:00:01, mustaq wrote: > ptal. > > rbyers@: Since we want to have touch fractional coordinates asap, we have to > merge this change to stable (or even some beta if possible). We want to minimize > changes related to MouseEvents here so that we can merge required PointerEvent > changes quickly by avoiding approvals for MouseEvents. > > Moreover, if we truncate doubles for all non-PointerEvents, the compat risk is > nearly zero. E.g. direct coordinate comparisons or array access through > MouseEvent.clientX would still work if any site happen to rely on these. Ok, keeping the risk as low as possible for M55 SGTM, thanks. > > dtapuska@: Fixed all layout tests. The constructor tests were affected because > of weird JS parsing rules for ints vs floats. Not a big deal---these tests are > "overfitting" our implementation anyway. E.g. just discovered that > eval("new MouseEvent('type', {screenX: 2147483647}).screenX"); > is devtools returns 2147483647, not MAX_CLIENT_SIZE as in the test. > > https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > https://codereview.chromium.org/2406263003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:47: return > isPointerEvent() ? m_screenLocation.x().toDouble() > On 2016/10/12 14:09:54, Rick Byers wrote: > > It seems odd that this pattern of conditional return values is necessary. > Can't > > you just initialize the event in the first place to truncate to integers for > > mouse events but not for pointer events? > > The problem here is that the initialization is done at MouseRelatedEvent > constructors where the vtable for the actual subclass being constructed (e.g. > MouseEvent or PointerEvent) is not "ready". As a result, all isXyzEvent() > functions return false. Ok, if you add a TODO to eliminate this in 56 (along with the other cleanups you're planning) that's fine with me.
Found a few more issues looking at this in more detail. Sorry I didn't mention these sooner. https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/constructors/mouse-event-constructor-expected.txt (right): https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/constructors/mouse-event-constructor-expected.txt:71: PASS new MouseEvent('eventType', { screenX: NaN }).screenX threw exception TypeError: Failed to construct 'MouseEvent': The provided double value is non-finite.. it's possible this could be a web compat issue. If you want to keep the compat risk near 0 then you probably want to continue coercing this case to '0' (and those below which are now throwing). https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html (right): https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html:92: shouldBeEqualToNumber("receivedPEsAtTarget["+i+"].clientY", y+19.25); add test for fractional screen and page co-ordinates as well. Since the code paths are the same (and pointermove is the important case) I'm fine with testing fractional co-ordinates on just the pointermove event. https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/MouseEvent.idl (right): https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/MouseEvent.idl:61: readonly attribute long pageY; the Touch page* co-ordinates are double today, so we should upgrade those as well here. Maybe just do them all? It seems weird to change some of the co-ordinates and not others. If there's a good reason to delay the ones below though (i.e. if it complicates the CL much) then I'm OK doing them separately.
rbyers@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/constructors/mouse-event-constructor-expected.txt (right): https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/constructors/mouse-event-constructor-expected.txt:71: PASS new MouseEvent('eventType', { screenX: NaN }).screenX threw exception TypeError: Failed to construct 'MouseEvent': The provided double value is non-finite.. On 2016/10/13 19:08:20, Rick Byers wrote: > it's possible this could be a web compat issue. If you want to keep the compat > risk near 0 then you probably want to continue coercing this case to '0' (and > those below which are now throwing). This exception is triggered by JS parser (in V8Binding.cpp::toRestrictedDouble) because we changed the type to |double| in the idl. This happens for all floats and doubles, e.g. new WheelEvent('eventType', { deltaX: NaN }) I tried changing the type to |unrestricted double| since Web IDL spec says this should allow non-finite values. But the outcome is worse: Chrome crashes for NaN or "123a". The risk should be very low: setting MouseEvent coordinates from arbitrary data should be very rare. Wdyt? https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html (right): https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html:92: shouldBeEqualToNumber("receivedPEsAtTarget["+i+"].clientY", y+19.25); On 2016/10/13 19:08:21, Rick Byers wrote: > add test for fractional screen and page co-ordinates as well. > > Since the code paths are the same (and pointermove is the important case) I'm > fine with testing fractional co-ordinates on just the pointermove event. Done. https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/MouseEvent.idl (right): https://codereview.chromium.org/2406263003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/MouseEvent.idl:61: readonly attribute long pageY; On 2016/10/13 19:08:21, Rick Byers wrote: > the Touch page* co-ordinates are double today, so we should upgrade those as > well here. > > Maybe just do them all? It seems weird to change some of the co-ordinates and > not others. If there's a good reason to delay the ones below though (i.e. if it > complicates the CL much) then I'm OK doing them separately. Let's do them separately, to minimize MouseEvent related risks.
ptal. JS parsing no longer throws exception where it used to default to zeroes. The code is in a bit messy state but it's better than exposing behavior diffs through merged changes.
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/10/17 19:16:09, mustaq wrote: > ptal. JS parsing no longer throws exception where it used to default to zeroes. > The code is in a bit messy state but it's better than exposing behavior diffs > through merged changes. ptal, moved back to double types in MouseEventInit: the separate initialization path still needs isPointerEvent() hack on MouseRelatedEvent getters, and then mouse vs TouchPoint coordinate differences kicks in. The messy temporary initialization path would need a lot of attention to fix, doesn't worth the effort. The good news about the double vs int parsing in JS is that 'undefined', 'null' and empty-strings behave similarly for double & int, so the risk looks very small as we discussed yesterday.
On 2016/10/18 14:48:05, mustaq wrote: > On 2016/10/17 19:16:09, mustaq wrote: > > ptal. JS parsing no longer throws exception where it used to default to > zeroes. > > The code is in a bit messy state but it's better than exposing behavior diffs > > through merged changes. > > ptal, moved back to double types in MouseEventInit: the separate initialization > path still needs isPointerEvent() hack on MouseRelatedEvent getters, and then > mouse vs TouchPoint coordinate differences kicks in. The messy temporary > initialization path would need a lot of attention to fix, doesn't worth the > effort. > > The good news about the double vs int parsing in JS is that 'undefined', 'null' > and empty-strings behave similarly for double & int, so the risk looks very > small as we discussed yesterday. I'm nervous about doing this change to the API surface to merge it into 56. I wonder if we just leave 56 as it is and for 57 we do an intent to ship and store the correct units (doubles) in the MouseEvent. Currently it seems to truncate it to the layout point which seems grossly incorrect. It just feels we are rushing to make a fix in 56 that we could probably live with for 6 weeks and get it correct in 57. WDYT?
On 2016/10/19 15:39:39, dtapuska wrote: > On 2016/10/18 14:48:05, mustaq wrote: > > On 2016/10/17 19:16:09, mustaq wrote: > > > ptal. JS parsing no longer throws exception where it used to default to > > zeroes. > > > The code is in a bit messy state but it's better than exposing behavior > diffs > > > through merged changes. > > > > ptal, moved back to double types in MouseEventInit: the separate > initialization > > path still needs isPointerEvent() hack on MouseRelatedEvent getters, and then > > mouse vs TouchPoint coordinate differences kicks in. The messy temporary > > initialization path would need a lot of attention to fix, doesn't worth the > > effort. > > > > The good news about the double vs int parsing in JS is that 'undefined', > 'null' > > and empty-strings behave similarly for double & int, so the risk looks very > > small as we discussed yesterday. > > I'm nervous about doing this change to the API surface to merge it into 56. I > wonder if we just leave 56 as it is and for 57 we do an intent to ship and store > the correct units (doubles) in the MouseEvent. Currently it seems to truncate it > to the layout point which seems grossly incorrect. It just feels we are rushing > to make a fix in 56 that we could probably live with for 6 weeks and get it > correct in 57. WDYT? As we discussed offline, I'm OK with that. The downside to not fixing this in M55 isn't huge - it's a niche scenario (art) that's substantially impacted and I agree with your argument that it's better to pose some minor risk to those scenarios (which developers can work around by preferring TouchEvent to PointerEvent, and users can work around by disabling the pointer events flag) than it is to pose ANY risk post-branch to the billions of sites relying on MouseEvent behavior. It would certainly be much more straight forward to do all the work at once in M56 - clean this all up to be logical, having a single intent etc. If you and Mustaq decide you'd rather take that route, that's OK with me - it's really your call (not mine).
Patchset #10 (id:180001) has been deleted
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
ptal.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional coordinates to JS. BUG=607948 ========== to ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional coordinates to JS. Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/1kroHAnh3BU/uY... BUG=607948 ==========
On 2016/11/14 19:06:42, mustaq wrote: > ptal. lgtm for now. Albeit I find it odd that MouseEvent constructed from javascript doesn't carry the double property throughout. I would have expected that to be the case. And that we only truncate (to int) in the case of user generated MouseEvents.
mustaq@chromium.org changed reviewers: + bokan@chromium.org
bokan@chromium.org: Please review changes in Source/web.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint m_pageLocation; Why double? We've recently settled the debate about scroll offsets to use floats. Is this arbitrary or does this match some existing choices? Couldn't we just keep these as LayoutPoint and convert to a float in the accessors?
https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint m_pageLocation; On 2016/11/15 12:55:39, bokan wrote: > Why double? We've recently settled the debate about scroll offsets to use > floats. Is this arbitrary or does this match some existing choices? Couldn't we > just keep these as LayoutPoint and convert to a float in the accessors? I learned somewhere that floats can't represent integer precision (=1) for very "tall" pages, that's why MouseRelatedEvent had coordinates in LayoutUnits. This is the only reason that motivated me to use DoublePoint instead of FloatPoint here. After your comment, I investigated further and couldn't see why LayoutUnit is useful at all (vs float). A float can represent a max integer of 24 bits (23 digits of mantissa plus the "implied" MSB) so that max value is 2^25-1 = 33,554,431, which seems exactly the same as the LayoutUnit limit (see ToT mouse-event-constructor.html). I wasn't aware of the decision you mentioned about using floats for scroll offsets, which seems quite logical now. I will switch to FloatPoints then. Wdyt?
lgtm % FloatPoint change https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint m_pageLocation; On 2016/11/15 15:21:15, mustaq wrote: > On 2016/11/15 12:55:39, bokan wrote: > > Why double? We've recently settled the debate about scroll offsets to use > > floats. Is this arbitrary or does this match some existing choices? Couldn't > we > > just keep these as LayoutPoint and convert to a float in the accessors? > > I learned somewhere that floats can't represent integer precision (=1) for very > "tall" pages, that's why MouseRelatedEvent had coordinates in LayoutUnits. > This is the only reason that motivated me to use DoublePoint instead of > FloatPoint here. > > After your comment, I investigated further and couldn't see why LayoutUnit is > useful at all (vs float). A float can represent a max integer of 24 bits (23 > digits of mantissa plus the "implied" MSB) so that max value is > 2^25-1 = 33,554,431, > which seems exactly the same as the LayoutUnit limit (see ToT > mouse-event-constructor.html). > > I wasn't aware of the decision you mentioned about using floats for scroll > offsets, which seems quite logical now. > > I will switch to FloatPoints then. Wdyt? See crbug.com/470718 for the debate. LayoutPoint is useful in that we would align better with page content since that's all positioned using the fixed LayoutUnits. Although I think all existing scroll and event code uses floating points so might as well keep that consistent for now. FloatPoint sgtm.
https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint m_pageLocation; On 2016/11/15 15:26:00, bokan wrote: > On 2016/11/15 15:21:15, mustaq wrote: > > On 2016/11/15 12:55:39, bokan wrote: > > > Why double? We've recently settled the debate about scroll offsets to use > > > floats. Is this arbitrary or does this match some existing choices? Couldn't > > we > > > just keep these as LayoutPoint and convert to a float in the accessors? > > > > I learned somewhere that floats can't represent integer precision (=1) for > very > > "tall" pages, that's why MouseRelatedEvent had coordinates in LayoutUnits. > > This is the only reason that motivated me to use DoublePoint instead of > > FloatPoint here. > > > > After your comment, I investigated further and couldn't see why LayoutUnit is > > useful at all (vs float). A float can represent a max integer of 24 bits (23 > > digits of mantissa plus the "implied" MSB) so that max value is > > 2^25-1 = 33,554,431, > > which seems exactly the same as the LayoutUnit limit (see ToT > > mouse-event-constructor.html). > > > > I wasn't aware of the decision you mentioned about using floats for scroll > > offsets, which seems quite logical now. > > > > I will switch to FloatPoints then. Wdyt? > > See crbug.com/470718 for the debate. LayoutPoint is useful in that we would > align better with page content since that's all positioned using the fixed > LayoutUnits. Although I think all existing scroll and event code uses floating > points so might as well keep that consistent for now. FloatPoint sgtm. Done. I understand that the float vs Layout discussion has settled already but just for future records: My limit argument above seems off by a bit: float can represent 24 bits but that means the max is 2^24-1 = 16777215, half of LayoutUnit limit.
The CQ bit was checked by mustaq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/2406263003/#ps260001 (title: "DoublePoint -> FloatPoint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/15 16:51:39, mustaq wrote: > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint > m_pageLocation; > On 2016/11/15 15:26:00, bokan wrote: > > On 2016/11/15 15:21:15, mustaq wrote: > > > On 2016/11/15 12:55:39, bokan wrote: > > > > Why double? We've recently settled the debate about scroll offsets to use > > > > floats. Is this arbitrary or does this match some existing choices? > Couldn't > > > we > > > > just keep these as LayoutPoint and convert to a float in the accessors? > > > > > > I learned somewhere that floats can't represent integer precision (=1) for > > very > > > "tall" pages, that's why MouseRelatedEvent had coordinates in LayoutUnits. > > > This is the only reason that motivated me to use DoublePoint instead of > > > FloatPoint here. > > > > > > After your comment, I investigated further and couldn't see why LayoutUnit > is > > > useful at all (vs float). A float can represent a max integer of 24 bits (23 > > > digits of mantissa plus the "implied" MSB) so that max value is > > > 2^25-1 = 33,554,431, > > > which seems exactly the same as the LayoutUnit limit (see ToT > > > mouse-event-constructor.html). > > > > > > I wasn't aware of the decision you mentioned about using floats for scroll > > > offsets, which seems quite logical now. > > > > > > I will switch to FloatPoints then. Wdyt? > > > > See crbug.com/470718 for the debate. LayoutPoint is useful in that we would > > align better with page content since that's all positioned using the fixed > > LayoutUnits. Although I think all existing scroll and event code uses floating > > points so might as well keep that consistent for now. FloatPoint sgtm. > > Done. > > I understand that the float vs Layout discussion has settled already but just > for future records: > > My limit argument above seems off by a bit: float can represent 24 bits but that > means the max is 2^24-1 = 16777215, half of LayoutUnit limit. As long as we're just chatting, the difference isn't only maximum representation. Using fixed point has the advantage that the precision is constant over the whole range. Floating point numbers become less and less precise as they get further from 0. So while you might be able to represent 16777215 as a whole number, the error due precision at that point is much bigger than it would have been ~100.
On 2016/11/15 16:54:09, bokan wrote: > On 2016/11/15 16:51:39, mustaq wrote: > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint > > m_pageLocation; > > On 2016/11/15 15:26:00, bokan wrote: > > > On 2016/11/15 15:21:15, mustaq wrote: > > > > On 2016/11/15 12:55:39, bokan wrote: > > > > > Why double? We've recently settled the debate about scroll offsets to > use > > > > > floats. Is this arbitrary or does this match some existing choices? > > Couldn't > > > > we > > > > > just keep these as LayoutPoint and convert to a float in the accessors? > > > > > > > > I learned somewhere that floats can't represent integer precision (=1) for > > > very > > > > "tall" pages, that's why MouseRelatedEvent had coordinates in LayoutUnits. > > > > This is the only reason that motivated me to use DoublePoint instead of > > > > FloatPoint here. > > > > > > > > After your comment, I investigated further and couldn't see why LayoutUnit > > is > > > > useful at all (vs float). A float can represent a max integer of 24 bits > (23 > > > > digits of mantissa plus the "implied" MSB) so that max value is > > > > 2^25-1 = 33,554,431, > > > > which seems exactly the same as the LayoutUnit limit (see ToT > > > > mouse-event-constructor.html). > > > > > > > > I wasn't aware of the decision you mentioned about using floats for scroll > > > > offsets, which seems quite logical now. > > > > > > > > I will switch to FloatPoints then. Wdyt? > > > > > > See crbug.com/470718 for the debate. LayoutPoint is useful in that we would > > > align better with page content since that's all positioned using the fixed > > > LayoutUnits. Although I think all existing scroll and event code uses > floating > > > points so might as well keep that consistent for now. FloatPoint sgtm. > > > > Done. > > > > I understand that the float vs Layout discussion has settled already but just > > for future records: > > > > My limit argument above seems off by a bit: float can represent 24 bits but > that > > means the max is 2^24-1 = 16777215, half of LayoutUnit limit. > > As long as we're just chatting, the difference isn't only maximum > representation. Using fixed point has the advantage that the precision is > constant over the whole range. Floating point numbers become less and less > precise as they get further from 0. So while you might be able to represent > 16777215 as a whole number, the error due precision at that point is much bigger > than it would have been ~100. Agree, the delta of a float near 16777215 is 1, unlike LayoutUnit.
On 2016/11/15 16:58:10, mustaq wrote: > On 2016/11/15 16:54:09, bokan wrote: > > On 2016/11/15 16:51:39, mustaq wrote: > > > > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > > > > > > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint > > > m_pageLocation; > > > On 2016/11/15 15:26:00, bokan wrote: > > > > On 2016/11/15 15:21:15, mustaq wrote: > > > > > On 2016/11/15 12:55:39, bokan wrote: > > > > > > Why double? We've recently settled the debate about scroll offsets to > > use > > > > > > floats. Is this arbitrary or does this match some existing choices? > > > Couldn't > > > > > we > > > > > > just keep these as LayoutPoint and convert to a float in the > accessors? > > > > > > > > > > I learned somewhere that floats can't represent integer precision (=1) > for > > > > very > > > > > "tall" pages, that's why MouseRelatedEvent had coordinates in > LayoutUnits. > > > > > This is the only reason that motivated me to use DoublePoint instead of > > > > > FloatPoint here. > > > > > > > > > > After your comment, I investigated further and couldn't see why > LayoutUnit > > > is > > > > > useful at all (vs float). A float can represent a max integer of 24 bits > > (23 > > > > > digits of mantissa plus the "implied" MSB) so that max value is > > > > > 2^25-1 = 33,554,431, > > > > > which seems exactly the same as the LayoutUnit limit (see ToT > > > > > mouse-event-constructor.html). > > > > > > > > > > I wasn't aware of the decision you mentioned about using floats for > scroll > > > > > offsets, which seems quite logical now. > > > > > > > > > > I will switch to FloatPoints then. Wdyt? > > > > > > > > See crbug.com/470718 for the debate. LayoutPoint is useful in that we > would > > > > align better with page content since that's all positioned using the fixed > > > > LayoutUnits. Although I think all existing scroll and event code uses > > floating > > > > points so might as well keep that consistent for now. FloatPoint sgtm. > > > > > > Done. > > > > > > I understand that the float vs Layout discussion has settled already but > just > > > for future records: > > > > > > My limit argument above seems off by a bit: float can represent 24 bits but > > that > > > means the max is 2^24-1 = 16777215, half of LayoutUnit limit. > > > > As long as we're just chatting, the difference isn't only maximum > > representation. Using fixed point has the advantage that the precision is > > constant over the whole range. Floating point numbers become less and less > > precise as they get further from 0. So while you might be able to represent > > 16777215 as a whole number, the error due precision at that point is much > bigger > > than it would have been ~100. > > Agree, the delta of a float near 16777215 is 1, unlike LayoutUnit. javascript is purely 64 bit precision. So our idea was to make the DOM events lossless in this respect. Using a float here will cause the DOMEvents to be lossy which I don't really think it buys us much. Is the motivation for moving to floats driven by performance or space? Ultimately m_screenLocation, m_clientLocation, m_movementDelta are all user provided values (which I'd prefer as doubles); yet the other 4 are all computed values (which could remain as floats).
On 2016/11/15 17:06:08, dtapuska wrote: > On 2016/11/15 16:58:10, mustaq wrote: > > On 2016/11/15 16:54:09, bokan wrote: > > > On 2016/11/15 16:51:39, mustaq wrote: > > > > > > > > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: DoublePoint > > > > m_pageLocation; > > > > On 2016/11/15 15:26:00, bokan wrote: > > > > > On 2016/11/15 15:21:15, mustaq wrote: > > > > > > On 2016/11/15 12:55:39, bokan wrote: > > > > > > > Why double? We've recently settled the debate about scroll offsets > to > > > use > > > > > > > floats. Is this arbitrary or does this match some existing choices? > > > > Couldn't > > > > > > we > > > > > > > just keep these as LayoutPoint and convert to a float in the > > accessors? > > > > > > > > > > > > I learned somewhere that floats can't represent integer precision (=1) > > for > > > > > very > > > > > > "tall" pages, that's why MouseRelatedEvent had coordinates in > > LayoutUnits. > > > > > > This is the only reason that motivated me to use DoublePoint instead > of > > > > > > FloatPoint here. > > > > > > > > > > > > After your comment, I investigated further and couldn't see why > > LayoutUnit > > > > is > > > > > > useful at all (vs float). A float can represent a max integer of 24 > bits > > > (23 > > > > > > digits of mantissa plus the "implied" MSB) so that max value is > > > > > > 2^25-1 = 33,554,431, > > > > > > which seems exactly the same as the LayoutUnit limit (see ToT > > > > > > mouse-event-constructor.html). > > > > > > > > > > > > I wasn't aware of the decision you mentioned about using floats for > > scroll > > > > > > offsets, which seems quite logical now. > > > > > > > > > > > > I will switch to FloatPoints then. Wdyt? > > > > > > > > > > See crbug.com/470718 for the debate. LayoutPoint is useful in that we > > would > > > > > align better with page content since that's all positioned using the > fixed > > > > > LayoutUnits. Although I think all existing scroll and event code uses > > > floating > > > > > points so might as well keep that consistent for now. FloatPoint sgtm. > > > > > > > > Done. > > > > > > > > I understand that the float vs Layout discussion has settled already but > > just > > > > for future records: > > > > > > > > My limit argument above seems off by a bit: float can represent 24 bits > but > > > that > > > > means the max is 2^24-1 = 16777215, half of LayoutUnit limit. > > > > > > As long as we're just chatting, the difference isn't only maximum > > > representation. Using fixed point has the advantage that the precision is > > > constant over the whole range. Floating point numbers become less and less > > > precise as they get further from 0. So while you might be able to represent > > > 16777215 as a whole number, the error due precision at that point is much > > bigger > > > than it would have been ~100. > > > > Agree, the delta of a float near 16777215 is 1, unlike LayoutUnit. > > javascript is purely 64 bit precision. So our idea was to make the DOM events > lossless in this respect. Using a float here will cause the DOMEvents to be > lossy which I don't really think it buys us much. > > Is the motivation for moving to floats driven by performance or space? > > Ultimately m_screenLocation, m_clientLocation, m_movementDelta are all user > provided values (which I'd prefer as doubles); yet the other 4 are all computed > values (which could remain as floats). The motivation is consistency with other values in Blink. We've had issues before with CC storing values as double and Blink as float. I suppose we're in a bit of a bind since it either way we're going to be lossy somewhere. In this case though I agree keeping page visible values from losing precision is important and in that case might as well make them all double. Sorry about the extra work
The CQ bit was unchecked by mustaq@chromium.org
On 2016/11/15 17:56:24, bokan wrote: > On 2016/11/15 17:06:08, dtapuska wrote: > > On 2016/11/15 16:58:10, mustaq wrote: > > > On 2016/11/15 16:54:09, bokan wrote: > > > > On 2016/11/15 16:51:39, mustaq wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/core/events/MouseRelatedEvent.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2406263003/diff/240001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/core/events/MouseRelatedEvent.h:130: > DoublePoint > > > > > m_pageLocation; > > > > > On 2016/11/15 15:26:00, bokan wrote: > > > > > > On 2016/11/15 15:21:15, mustaq wrote: > > > > > > > On 2016/11/15 12:55:39, bokan wrote: > > > > > > > > Why double? We've recently settled the debate about scroll offsets > > to > > > > use > > > > > > > > floats. Is this arbitrary or does this match some existing > choices? > > > > > Couldn't > > > > > > > we > > > > > > > > just keep these as LayoutPoint and convert to a float in the > > > accessors? > > > > > > > > > > > > > > I learned somewhere that floats can't represent integer precision > (=1) > > > for > > > > > > very > > > > > > > "tall" pages, that's why MouseRelatedEvent had coordinates in > > > LayoutUnits. > > > > > > > This is the only reason that motivated me to use DoublePoint instead > > of > > > > > > > FloatPoint here. > > > > > > > > > > > > > > After your comment, I investigated further and couldn't see why > > > LayoutUnit > > > > > is > > > > > > > useful at all (vs float). A float can represent a max integer of 24 > > bits > > > > (23 > > > > > > > digits of mantissa plus the "implied" MSB) so that max value is > > > > > > > 2^25-1 = 33,554,431, > > > > > > > which seems exactly the same as the LayoutUnit limit (see ToT > > > > > > > mouse-event-constructor.html). > > > > > > > > > > > > > > I wasn't aware of the decision you mentioned about using floats for > > > scroll > > > > > > > offsets, which seems quite logical now. > > > > > > > > > > > > > > I will switch to FloatPoints then. Wdyt? > > > > > > > > > > > > See crbug.com/470718 for the debate. LayoutPoint is useful in that we > > > would > > > > > > align better with page content since that's all positioned using the > > fixed > > > > > > LayoutUnits. Although I think all existing scroll and event code uses > > > > floating > > > > > > points so might as well keep that consistent for now. FloatPoint sgtm. > > > > > > > > > > Done. > > > > > > > > > > I understand that the float vs Layout discussion has settled already but > > > just > > > > > for future records: > > > > > > > > > > My limit argument above seems off by a bit: float can represent 24 bits > > but > > > > that > > > > > means the max is 2^24-1 = 16777215, half of LayoutUnit limit. > > > > > > > > As long as we're just chatting, the difference isn't only maximum > > > > representation. Using fixed point has the advantage that the precision is > > > > constant over the whole range. Floating point numbers become less and less > > > > precise as they get further from 0. So while you might be able to > represent > > > > 16777215 as a whole number, the error due precision at that point is much > > > bigger > > > > than it would have been ~100. > > > > > > Agree, the delta of a float near 16777215 is 1, unlike LayoutUnit. > > > > javascript is purely 64 bit precision. So our idea was to make the DOM events > > lossless in this respect. Using a float here will cause the DOMEvents to be > > lossy which I don't really think it buys us much. > > > > Is the motivation for moving to floats driven by performance or space? > > > > Ultimately m_screenLocation, m_clientLocation, m_movementDelta are all user > > provided values (which I'd prefer as doubles); yet the other 4 are all > computed > > values (which could remain as floats). > > The motivation is consistency with other values in Blink. We've had issues > before with CC storing values as double and Blink as float. I suppose we're in a > bit of a bind since it either way we're going to be lossy somewhere. In this > case though I agree keeping page visible values from losing precision is > important and in that case might as well make them all double. Sorry about the > extra work I unchecked the CQ bit, to go back to DoublePoints which seems more correct from JS initialized values.
The CQ bit was checked by mustaq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/2406263003/#ps280001 (title: "FloatPoint -> DoublePoint again.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by mustaq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/2406263003/#ps300001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mustaq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional coordinates to JS. Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/1kroHAnh3BU/uY... BUG=607948 ========== to ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional coordinates to JS. Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/1kroHAnh3BU/uY... BUG=607948 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional coordinates to JS. Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/1kroHAnh3BU/uY... BUG=607948 ========== to ========== Make PointerEvent coordinates fractional for touch The current PointerEvent spec doesn't override the UI MouseEvent spec definition of (clientX, clientY) and (screenX, screenY) coordinates: all of them are integers. This is not great for the sites that currently use Touch Events & trying to switch to Pointer Events for touch: specially on high DPI devices Pointer Event coordinates would feel less precise than fractional Touch Event coodinates. The PEWG has already reached consensus that all coordinates in Pointer Events should be floating point values: https://github.com/w3c/pointerevents/issues/107 Edge already reports fractional client coordinates for PointerEvents (and not MouseEvents). This CL exposes fractional Pointer Event coordinates for touch input. Although the CL changes the client & screen coordinates in Mouse Event idl to floating points, the integer plumbing for mouse is kept intact so that mouse coordinates still appear unchanged to JS. Pointer Events that are fired for touches propagate appropriate fractional coordinates to JS. Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/1kroHAnh3BU/uY... BUG=607948 Committed: https://crrev.com/48a106897274196b5897f1b0967d28c49e4ec696 Cr-Commit-Position: refs/heads/master@{#432473} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/48a106897274196b5897f1b0967d28c49e4ec696 Cr-Commit-Position: refs/heads/master@{#432473} |