Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1964523002:
Fix clientX/Y properties of touch pointer events (Closed)
|
Created:
4 years, 7 months ago by Navid Zolghadr Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix clientX/Y properties of touch pointer events
Before this change clientX/Y of touch pointer events
were incorrectly set from content coordinates.
But after this CL they are correctly translated
by scroll position and will become clientX/Y.
BUG=608394
Committed: https://crrev.com/52b840ceccfb1041681e0e85a61e9bcdee071b8a
Cr-Commit-Position: refs/heads/master@{#392722}
Patch Set 1 #
Total comments: 8
Patch Set 2 : clientPoint -> framePoint #
Total comments: 10
Patch Set 3 : Applying comments #
Total comments: 6
Patch Set 4 : Fix zoom problem #Messages
Total messages: 28 (7 generated)
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:40: FloatPoint clientPoint; bokan@: "client" seems to be an overloaded term throughout our codebase. May be this is not true for coordinates? Any chance we could be more specific here?
On 2016/05/09 14:49:06, mustaq wrote: > https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/TouchEventManager.h (right): > > https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/TouchEventManager.h:40: FloatPoint > clientPoint; > bokan@: "client" seems to be an overloaded term throughout our codebase. May be > this is not true for coordinates? Any chance we could be more specific here? I was just following this naming convention he proposed: Content/Page Coordinates: Relative to the document origin (i.e. affected by scroll offset) Frame/Client Coordinates: Relative to the frame origin (i.e. unaffected by scroll offset) Root Frame Coordinates: Same as frame coordinates but specifically denotes the root frame Viewport Coordinates: Relative to the Blink widget's origin. (i.e. unaffected by scroll or pinch-zoom) Do you think pageCoordinates is better?
On 2016/05/09 14:51:53, Navid Zolghadr wrote: > On 2016/05/09 14:49:06, mustaq wrote: > > > https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/input/TouchEventManager.h (right): > > > > > https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/input/TouchEventManager.h:40: FloatPoint > > clientPoint; > > bokan@: "client" seems to be an overloaded term throughout our codebase. May > be > > this is not true for coordinates? Any chance we could be more specific here? > > I was just following this naming convention he proposed: > > Content/Page Coordinates: Relative to the document origin (i.e. affected by > scroll offset) > Frame/Client Coordinates: Relative to the frame origin (i.e. unaffected by > scroll offset) > Root Frame Coordinates: Same as frame coordinates but specifically denotes the > root frame > Viewport Coordinates: Relative to the Blink widget's origin. (i.e. unaffected by > scroll or pinch-zoom) > > Do you think pageCoordinates is better? I want to prevent future mistakes around coordinates, so wanted a specific term. But as an expert on our coordinate space, bokan should make the call.
On 2016/05/09 15:01:15, mustaq wrote: > On 2016/05/09 14:51:53, Navid Zolghadr wrote: > > On 2016/05/09 14:49:06, mustaq wrote: > > > > > > https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/input/TouchEventManager.h (right): > > > > > > > > > https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/input/TouchEventManager.h:40: FloatPoint > > > clientPoint; > > > bokan@: "client" seems to be an overloaded term throughout our codebase. May > > be > > > this is not true for coordinates? Any chance we could be more specific here? > > > > I was just following this naming convention he proposed: > > > > Content/Page Coordinates: Relative to the document origin (i.e. affected by > > scroll offset) > > Frame/Client Coordinates: Relative to the frame origin (i.e. unaffected by > > scroll offset) > > Root Frame Coordinates: Same as frame coordinates but specifically denotes the > > root frame > > Viewport Coordinates: Relative to the Blink widget's origin. (i.e. unaffected > by > > scroll or pinch-zoom) > > > > Do you think pageCoordinates is better? > > I want to prevent future mistakes around coordinates, so wanted a specific term. > But as an expert on our coordinate space, bokan should make the call. Sure. I explicitly ask him when I add him to the review.
Description was changed from ========== Fix clientX/Y properties of touch pointer events Before this change clientX/Y of touch pointer events were incorrectly set from content coordinates. But after this CL they are correctly translated by scroll position and will become clientX/Y. BUG=608394 ========== to ========== Fix clientX/Y properties of touch pointer events Before this change clientX/Y of touch pointer events were incorrectly set from content coordinates. But after this CL they are correctly translated by scroll position and will become clientX/Y. BUG=608394 ==========
mustaq@chromium.org changed reviewers: + bokan@chromium.org
https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:40: FloatPoint clientPoint; On 2016/05/09 14:49:05, mustaq wrote: > bokan@: "client" seems to be an overloaded term throughout our codebase. May be > this is not true for coordinates? Any chance we could be more specific here? bokan@, do you think framePoint is better instead of clientPoint here or maybe totally something else?
https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:40: FloatPoint clientPoint; On 2016/05/09 17:01:43, Navid Zolghadr wrote: > On 2016/05/09 14:49:05, mustaq wrote: > > bokan@: "client" seems to be an overloaded term throughout our codebase. May > be > > this is not true for coordinates? Any chance we could be more specific here? > > bokan@, do you think framePoint is better instead of clientPoint here or maybe > totally something else? Client and Frame are interchangeable. In terms of coordinates its not overloaded and is a necessity since lots of web facing APIs use clientX/Y so we have to keep it in some places. I don't have a strong preference either way. If this isn't web facing though, framePoint is generally used for chrome internal data. If you do keep it clientPoint, then you should change contentPoint below to pagePoint to match.
I changed clientPoint to framePoint as it is an internal state. ptal.
https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:11: clientX = 92 It's not clear to me why (200,200) becomes (92,162) and not (100,170). Any clue what causes the 8-pixel gaps? Padding may be? Please try to position the iframe using top/left, and set margin=padding=0. https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:13: <iframe id='target' srcdoc=" Please indent the quoted srcdoc.
https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:11: clientX = 92 On 2016/05/09 18:08:34, mustaq wrote: > It's not clear to me why (200,200) becomes (92,162) and not (100,170). Any clue > what causes the 8-pixel gaps? Padding may be? Please try to position the iframe > using top/left, and set margin=padding=0. Default body margin is 8px, I like to set body { margin: 0px; } in tests for nice round numbers. https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html (right): https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:18: 'mouseup', 'mousedown', 'mousemove',]; For my own understanding, why aren't the mouseup|down|move events not generated here? If that's indeed expected, remove them from the event list here. https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:45: eventSender.addTouchPoint(200, 200) Please add a test that's a clone of this but with a page zoom factor. https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:388: touchInfo.framePoint.moveBy(scrollPosition.scaledBy(-scaleFactor)); Use Frame::contentsToFrame rather than manually moving the point https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:41: FloatPoint contentPoint; Since we have a reference to the targetFrame above, would it make more sense to keep just one of these and add an accessor for the other that just calculates using contentsToFrame/frameToContents. That way they can't ever be out of sync.
https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html (right): https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:40: document.getElementById('target').contentWindow.scrollTo(0, 100); Can we test the same event sequence at two different scroll positions?
ptal. https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:11: clientX = 92 On 2016/05/09 18:13:08, bokan wrote: > On 2016/05/09 18:08:34, mustaq wrote: > > It's not clear to me why (200,200) becomes (92,162) and not (100,170). Any > clue > > what causes the 8-pixel gaps? Padding may be? Please try to position the > iframe > > using top/left, and set margin=padding=0. > > Default body margin is 8px, I like to set body { margin: 0px; } in tests for > nice round numbers. Done. https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html (right): https://codereview.chromium.org/1964523002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:13: <iframe id='target' srcdoc=" On 2016/05/09 18:08:34, mustaq wrote: > Please indent the quoted srcdoc. Done. https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html (right): https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:18: 'mouseup', 'mousedown', 'mousemove',]; On 2016/05/09 18:13:08, bokan wrote: > For my own understanding, why aren't the mouseup|down|move events not generated > here? If that's indeed expected, remove them from the event list here. I needed them for my future change which I'm going to add more complete testing in this same file :). I just forgot to remove this part while removing the mouse related tests. https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:40: document.getElementById('target').contentWindow.scrollTo(0, 100); On 2016/05/09 18:16:50, mustaq wrote: > Can we test the same event sequence at two different scroll positions? Done. https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:45: eventSender.addTouchPoint(200, 200) On 2016/05/09 18:13:08, bokan wrote: > Please add a test that's a clone of this but with a page zoom factor. I added one but I was not sure what the result should be. Basically I didn't know what the anchor point is when I set the zoom factor? does the output looks okay now? https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:388: touchInfo.framePoint.moveBy(scrollPosition.scaledBy(-scaleFactor)); On 2016/05/09 18:13:08, bokan wrote: > Use Frame::contentsToFrame rather than manually moving the point Done. https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1964523002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:41: FloatPoint contentPoint; On 2016/05/09 18:13:08, bokan wrote: > Since we have a reference to the targetFrame above, would it make more sense to > keep just one of these and add an accessor for the other that just calculates > using contentsToFrame/frameToContents. That way they can't ever be out of sync. Sure. I was going to remove this whole struct in the later change but it sure is better to avoid adding redundant stuff here until it gets removed.
https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt (right): https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:30: clientY = -107 This looks wrong to me. For starters, at 2X zoom the iframe is at (100px, 200px) in DIPs so I think the touch is occurring at (50px, 0px) in CSS pixels in the iframe's client coordinates. We should certainly not be seeing negative client coordinates here. Touching/clicking at the bottom right corner of the 300x300 iframe should give client coordinates of (299,299) CSS pixels regardless of zoom level. That said, I'm assuming eventSender coordinates are "renderer coordinates", so they'd be in DIPs relative to the window origin. That makes most sense to me but it's not an assumption I've confirmed. This would mean that as you zoom in, you'd have to increase the coordinates given to eventSender to hit the bottom right of the iframe. FYI, DIP / pageZoom = CSS pixel for this case.
On 2016/05/09 21:03:07, bokan wrote: > https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt > (right): > > https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:30: > clientY = -107 > This looks wrong to me. For starters, at 2X zoom the iframe is at (100px, 200px) > in DIPs so I think the touch is occurring at (50px, 0px) in CSS pixels in the > iframe's client coordinates. We should certainly not be seeing negative client > coordinates here. > > Touching/clicking at the bottom right corner of the 300x300 iframe should give > client coordinates of (299,299) CSS pixels regardless of zoom level. > > That said, I'm assuming eventSender coordinates are "renderer coordinates", so > they'd be in DIPs relative to the window origin. That makes most sense to me but > it's not an assumption I've confirmed. This would mean that as you zoom in, > you'd have to increase the coordinates given to eventSender to hit the bottom > right of the iframe. > > FYI, DIP / pageZoom = CSS pixel for this case. I was looking at the implementation of contentsToFrame function. It seems that it doesn't consider the zoom factor. I couldn't find another function that also considers that. Is that okay that I do it manually once again (basically the same way it was in the first patch)?
https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt (right): https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:30: clientY = -107 On 2016/05/09 21:03:07, bokan wrote: > This looks wrong to me. For starters, at 2X zoom the iframe is at (100px, 200px) > in DIPs so I think the touch is occurring at (50px, 0px) in CSS pixels in the > iframe's client coordinates. We should certainly not be seeing negative client > coordinates here. > > Touching/clicking at the bottom right corner of the 300x300 iframe should give > client coordinates of (299,299) CSS pixels regardless of zoom level. > > That said, I'm assuming eventSender coordinates are "renderer coordinates", so > they'd be in DIPs relative to the window origin. That makes most sense to me but > it's not an assumption I've confirmed. This would mean that as you zoom in, > you'd have to increase the coordinates given to eventSender to hit the bottom > right of the iframe. > > FYI, DIP / pageZoom = CSS pixel for this case. I was looking at the implementation of contentsToFrame function. It seems that it doesn't consider the zoom factor. I couldn't find another function that also considers that. Is that okay that I do it manually once again (basically the same way it was in the first patch)?
https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt (right): https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:30: clientY = -107 On 2016/05/10 15:01:40, Navid Zolghadr wrote: > On 2016/05/09 21:03:07, bokan wrote: > > This looks wrong to me. For starters, at 2X zoom the iframe is at (100px, > 200px) > > in DIPs so I think the touch is occurring at (50px, 0px) in CSS pixels in the > > iframe's client coordinates. We should certainly not be seeing negative client > > coordinates here. > > > > Touching/clicking at the bottom right corner of the 300x300 iframe should give > > client coordinates of (299,299) CSS pixels regardless of zoom level. > > > > That said, I'm assuming eventSender coordinates are "renderer coordinates", so > > they'd be in DIPs relative to the window origin. That makes most sense to me > but > > it's not an assumption I've confirmed. This would mean that as you zoom in, > > you'd have to increase the coordinates given to eventSender to hit the bottom > > right of the iframe. > > > > FYI, DIP / pageZoom = CSS pixel for this case. > > I was looking at the implementation of contentsToFrame function. It seems that > it doesn't consider the zoom factor. I couldn't find another function that also > considers that. > Is that okay that I do it manually once again (basically the same way it was in > the first patch)? Ah, touchInfo is already zoomed...we should only be applying zoom factor just before we provide it to the web contents. Internally, everything should be in unzoomed DIPs. lgtm to do this manually since this is all in flux (you mentioned getting rid of TouchInfo, right) but we should avoid making manual conversions like this long term.
lgtm. https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html (right): https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:39: document.getElementById('target').contentWindow.scrollTo(scrollX, scrollY); Please print a test scenario header with the current scroll/zoom params.
https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt (right): https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe-expected.txt:30: clientY = -107 On 2016/05/10 16:33:53, bokan wrote: > On 2016/05/10 15:01:40, Navid Zolghadr wrote: > > On 2016/05/09 21:03:07, bokan wrote: > > > This looks wrong to me. For starters, at 2X zoom the iframe is at (100px, > > 200px) > > > in DIPs so I think the touch is occurring at (50px, 0px) in CSS pixels in > the > > > iframe's client coordinates. We should certainly not be seeing negative > client > > > coordinates here. > > > > > > Touching/clicking at the bottom right corner of the 300x300 iframe should > give > > > client coordinates of (299,299) CSS pixels regardless of zoom level. > > > > > > That said, I'm assuming eventSender coordinates are "renderer coordinates", > so > > > they'd be in DIPs relative to the window origin. That makes most sense to me > > but > > > it's not an assumption I've confirmed. This would mean that as you zoom in, > > > you'd have to increase the coordinates given to eventSender to hit the > bottom > > > right of the iframe. > > > > > > FYI, DIP / pageZoom = CSS pixel for this case. > > > > I was looking at the implementation of contentsToFrame function. It seems that > > it doesn't consider the zoom factor. I couldn't find another function that > also > > considers that. > > Is that okay that I do it manually once again (basically the same way it was > in > > the first patch)? > > Ah, touchInfo is already zoomed...we should only be applying zoom factor just > before we provide it to the web contents. Internally, everything should be in > unzoomed DIPs. lgtm to do this manually since this is all in flux (you mentioned > getting rid of TouchInfo, right) but we should avoid making manual conversions > like this long term. Done. https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html (right): https://codereview.chromium.org/1964523002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:39: document.getElementById('target').contentWindow.scrollTo(scrollX, scrollY); On 2016/05/10 17:26:16, mustaq wrote: > Please print a test scenario header with the current scroll/zoom params. Done. I also added the touch event listener to see clientX/Y on both touch events and pointer events to make it easier to verify.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/1964523002/#ps60001 (title: "Fix zoom problem")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964523002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964523002/60001
Message was sent while issue was closed.
Description was changed from ========== Fix clientX/Y properties of touch pointer events Before this change clientX/Y of touch pointer events were incorrectly set from content coordinates. But after this CL they are correctly translated by scroll position and will become clientX/Y. BUG=608394 ========== to ========== Fix clientX/Y properties of touch pointer events Before this change clientX/Y of touch pointer events were incorrectly set from content coordinates. But after this CL they are correctly translated by scroll position and will become clientX/Y. BUG=608394 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix clientX/Y properties of touch pointer events Before this change clientX/Y of touch pointer events were incorrectly set from content coordinates. But after this CL they are correctly translated by scroll position and will become clientX/Y. BUG=608394 ========== to ========== Fix clientX/Y properties of touch pointer events Before this change clientX/Y of touch pointer events were incorrectly set from content coordinates. But after this CL they are correctly translated by scroll position and will become clientX/Y. BUG=608394 Committed: https://crrev.com/52b840ceccfb1041681e0e85a61e9bcdee071b8a Cr-Commit-Position: refs/heads/master@{#392722} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/52b840ceccfb1041681e0e85a61e9bcdee071b8a Cr-Commit-Position: refs/heads/master@{#392722} |
