Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1971473002:
Move touch hit testing to PointerEventManager (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. |
DescriptionMove touch hit testing to PointerEventManager
Move touch hit testing to PointerEventManager class
to separate pointer event hit testing from touch event
hit testing. Now touch events use the pointer event
hit testing result if they were in the touch capturing
iframe otherwise they do another hit testing.
BUG=606822
Committed: https://crrev.com/5ce73f6e13fdb838a6c92430ca39538ed7e8a452
Cr-Commit-Position: refs/heads/master@{#394802}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Applying comments #
Total comments: 2
Patch Set 3 : Remove stale code #
Total comments: 20
Patch Set 4 : Applying comments #Patch Set 5 : Rebased #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 30 (7 generated)
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
Dave, following this link on how to run performance bots: https://www.chromium.org/developers/telemetry/performance-try-bots Is there a particular benchmark I should run?
https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:464: // TODO(crbug.com/608394): The adjustedPagePoint should be converted Isn't adjustedPagePoint now in client coordinates? The bug is fixed. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:474: result = sendTouchPointerEvent(touchInfo.touchNode, pointerEvent); This is now leaking PEs I believe! Please check if PEs are enabled or not. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (left): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:275: result = m_frame->eventHandler().hitTestResultAtPoint(pagePoint, hitType); Looks like we are not covering this case anymore. We should be careful not to change the touch behavior in any way. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.h (left): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:43: bool consumed; I don't think we can afford to lose this until crbug.com/607588 is fixed. You can either bring it back, or delay this CL until after the other bug is fixed.
https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:464: // TODO(crbug.com/608394): The adjustedPagePoint should be converted On 2016/05/11 14:30:55, mustaq wrote: > Isn't adjustedPagePoint now in client coordinates? The bug is fixed. I need to rebase this change to get the updates. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:474: result = sendTouchPointerEvent(touchInfo.touchNode, pointerEvent); On 2016/05/11 14:30:55, mustaq wrote: > This is now leaking PEs I believe! Please check if PEs are enabled or not. No. It does check the pointer enable at the last stage before sending the pointer event. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (left): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:275: result = m_frame->eventHandler().hitTestResultAtPoint(pagePoint, hitType); On 2016/05/11 14:30:55, mustaq wrote: > Looks like we are not covering this case anymore. We should be careful not to > change the touch behavior in any way. We are not. This scenario (which is generally the first touch) is handled by using the hit test result of pointer event that is already populated in TouchInfo struct. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.h (left): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.h:43: bool consumed; On 2016/05/11 14:30:55, mustaq wrote: > I don't think we can afford to lose this until crbug.com/607588 is fixed. You > can either bring it back, or delay this CL until after the other bug is fixed. Let's discuss this offline in more details. But I believe this should cause any difficulty for that. This CL just uses another mechanism for preventing the touch event and bypassing that will be easy. That mechanism lives in PointerEventManager.
On 2016/05/11 16:43:42, Navid Zolghadr wrote: > https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:464: // > TODO(crbug.com/608394): The adjustedPagePoint should be converted > On 2016/05/11 14:30:55, mustaq wrote: > > Isn't adjustedPagePoint now in client coordinates? The bug is fixed. > > I need to rebase this change to get the updates. > > https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:474: result = > sendTouchPointerEvent(touchInfo.touchNode, pointerEvent); > On 2016/05/11 14:30:55, mustaq wrote: > > This is now leaking PEs I believe! Please check if PEs are enabled or not. > > No. It does check the pointer enable at the last stage before sending the > pointer event. > > https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/TouchEventManager.cpp (left): > > https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/TouchEventManager.cpp:275: result = > m_frame->eventHandler().hitTestResultAtPoint(pagePoint, hitType); > On 2016/05/11 14:30:55, mustaq wrote: > > Looks like we are not covering this case anymore. We should be careful not to > > change the touch behavior in any way. > > We are not. This scenario (which is generally the first touch) is handled by > using the hit test result of pointer event that is already populated in > TouchInfo struct. > > https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/TouchEventManager.h (left): > > https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/TouchEventManager.h:43: bool consumed; > On 2016/05/11 14:30:55, mustaq wrote: > > I don't think we can afford to lose this until crbug.com/607588 is fixed. You > > can either bring it back, or delay this CL until after the other bug is fixed. > > Let's discuss this offline in more details. But I believe this should cause any > difficulty for that. This CL just uses another mechanism for preventing the > touch event and bypassing that will be easy. That mechanism lives in > PointerEventManager. Can you guys have a look at this one whenever you get a chance? This is kind of on critical path of the changes needed for EventHandler refactoring this quarter.
On 2016/05/10 19:21:02, Navid Zolghadr wrote: > Dave, following this link on how to run performance bots: > > https://www.chromium.org/developers/telemetry/performance-try-bots > > Is there a particular benchmark I should run? smoothness.top_25_smooth is probably good.
https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:440: TouchAction effectiveTouchAction = Is this correct to do here? What if the touch sequence document is different? https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:270: if (m_touchSequenceDocument && (!touchInfo.touchNode Can we add a comment what the intent of this code is. I understand it but since it is complex it deserves a comment.
https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:440: TouchAction effectiveTouchAction = On 2016/05/13 16:51:36, dtapuska wrote: > Is this correct to do here? What if the touch sequence document is different? I'm not sure. My understanding was that the touchaction will be per touch and the node that that particular touch is on. Mustaq, WDYT? Should I remove this from here and put it back in TouchEventManager?
https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:440: TouchAction effectiveTouchAction = On 2016/05/13 17:27:55, Navid Zolghadr wrote: > On 2016/05/13 16:51:36, dtapuska wrote: > > Is this correct to do here? What if the touch sequence document is different? > > I'm not sure. My understanding was that the touchaction will be per touch and > the node that that particular touch is on. Mustaq, WDYT? Should I remove this > from here and put it back in TouchEventManager? The info in communicated back to browser through this IPC handler: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... so the input_router for the correct frame should receive the IPC. So I think Dave is correct: this should move to TouchEventManager. Thanks Dave for the catch.
https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt:10: *** First touch down in innerFrame and move *** Nit: "First touch down..." -> "Put first finger..." etc? To emphasize it's about the physical action, not TE firing. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt:46: What about adding a few test scenarios: Scenario B: both fingers put in innerFrame, then finger1 moved to outer, then finger2 moved to outer => inner still gets {pointer,touch}moves. Scenario C: Symmetric to A, but starting in outerFrame. Scenario D: Symmetric to B, but starting in outerFrame. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:422: || !m_pendingPointerCaptureTarget.contains(pointerId)) { It took some time to realize why this is m_pendingPointerCaptureTarget instead of m_pointerCaptureTarget. Same below. Worth adding a note that we process pending capture later on. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:447: touchInfo.touchNode = m_pendingPointerCaptureTarget.get(pointerId)->toNode(); If we remove/disable implicit touch capturing, I suspect we can have pendingPointerCaptureTarget==null and pointerCaptureTarget!=null here. If this is the case, please add a TODO/comment here.
ptal. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt:10: *** First touch down in innerFrame and move *** On 2016/05/13 20:03:16, mustaq wrote: > Nit: "First touch down..." -> "Put first finger..." etc? To emphasize it's about > the physical action, not TE firing. Done. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt:46: On 2016/05/13 20:03:16, mustaq wrote: > What about adding a few test scenarios: > Scenario B: both fingers put in innerFrame, then finger1 moved to outer, then > finger2 moved to outer => inner still gets {pointer,touch}moves. > Scenario C: Symmetric to A, but starting in outerFrame. > Scenario D: Symmetric to B, but starting in outerFrame. Done. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:422: || !m_pendingPointerCaptureTarget.contains(pointerId)) { On 2016/05/13 20:03:16, mustaq wrote: > It took some time to realize why this is m_pendingPointerCaptureTarget instead > of m_pointerCaptureTarget. Same below. Worth adding a note that we process > pending capture later on. Done. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:440: TouchAction effectiveTouchAction = On 2016/05/13 19:18:01, mustaq wrote: > On 2016/05/13 17:27:55, Navid Zolghadr wrote: > > On 2016/05/13 16:51:36, dtapuska wrote: > > > Is this correct to do here? What if the touch sequence document is > different? > > > > I'm not sure. My understanding was that the touchaction will be per touch and > > the node that that particular touch is on. Mustaq, WDYT? Should I remove this > > from here and put it back in TouchEventManager? > > The info in communicated back to browser through this IPC handler: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > so the input_router for the correct frame should receive the IPC. So I think > Dave is correct: this should move to TouchEventManager. > > Thanks Dave for the catch. Done. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:447: touchInfo.touchNode = m_pendingPointerCaptureTarget.get(pointerId)->toNode(); On 2016/05/13 20:03:16, mustaq wrote: > If we remove/disable implicit touch capturing, I suspect we can have > pendingPointerCaptureTarget==null and pointerCaptureTarget!=null here. If this > is the case, please add a TODO/comment here. That is right. But I also check for this condition in the if condition and if pendingPointercaptureTarget is null I do hit test. I added a comment here as well to explain. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:464: // TODO(crbug.com/608394): The adjustedPagePoint should be converted On 2016/05/11 16:43:42, Navid Zolghadr wrote: > On 2016/05/11 14:30:55, mustaq wrote: > > Isn't adjustedPagePoint now in client coordinates? The bug is fixed. > > I need to rebase this change to get the updates. Done. https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:270: if (m_touchSequenceDocument && (!touchInfo.touchNode On 2016/05/13 16:51:36, dtapuska wrote: > Can we add a comment what the intent of this code is. I understand it but since > it is complex it deserves a comment. Done.
lgtm
LGTM... https://codereview.chromium.org/1971473002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html (right): https://codereview.chromium.org/1971473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:102: function scenario1() { I guess this is stale code?
nzolghadr@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/1971473002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html (right): https://codereview.chromium.org/1971473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:102: function scenario1() { On 2016/05/16 20:44:04, mustaq wrote: > I guess this is stale code? My bad. I missed it. done.
https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt:21: outerFrame recieved pointerdown nit: can you please add the pointer ID to these log messages, just so we can easily see that it's the right pointer getting the 'gotpointercapture' etc? https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:1: <!DOCTYPE HTML> Thanks for the thorough test! Not sure if the functionality is ready yet, but it would also be valuable to have an uncaptured variant of this test (which does releasePointerCapture on each touch). In particular, the interaction with touch events when the finger is uncaptured is ... "interesting". https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:20: <body> nit: layout tests normally omit body tags: https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:131: description("This test verifies clientX/Y of pointer events inside iframe."); This description seems wrong. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:438: // Touch events should not go to text nodes. update comment, probably even file a bug to track updating behavior. Do mouse events go to text nodes? Do pointer events on Edge? https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:476: touchInfos.append(touchInfo); So the touchinfos are now just the data needed for generating touch events? Update the function description to make that clear (it still refers to the consumed bit). Is this preserving the existing behavior though? Eg. what happens if code consumes the pointerdown but not the pointermove/pointerup? For mouse, we don't generate any compat mouse events in that case. For touch we'd send a touchmove/touchend but not a touchstart? That's probably wrong. But if you're not making this any worse in this CL, feel free to just file a bug and add a TODO here referencing it. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:279: if (m_touchSequenceDocument->frame()) { this code is mostly copy/paste with the code in PointerEventManager. Please use a common helper function somewhere. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:50: // Does the hit-testing again if the original hit test result was not inside nit: what does the return value mean? https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:56: WebInputEventResult handleTouchEvent( add comment describing how touchinfos are modified
https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:476: touchInfos.append(touchInfo); > Is this preserving the existing behavior though? ... We already have a p1 bug for this: crbug.com/607588. We will follow the Edge behavior here (see #1 in that bug), which makes the most sense. We have a nearly complete patch for GestureTap, using uniqueTouchIds. So in summary, we expect this to change soon, and we don't need to file a bug.
On 2016/05/17 14:40:14, mustaq wrote: > https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:476: > touchInfos.append(touchInfo); > > Is this preserving the existing behavior though? ... > > We already have a p1 bug for this: crbug.com/607588. We will follow the Edge > behavior here (see #1 in that bug), which makes the most sense. We have a nearly > complete patch for GestureTap, using uniqueTouchIds. > > So in summary, we expect this to change soon, and we don't need to file a bug. Excellent, thanks! I guess I knew we talked about this already. Anyway please add a TODO to the code referencing the bug so people looking at the code have context.
ptal. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe-expected.txt:21: outerFrame recieved pointerdown On 2016/05/17 14:20:01, Rick Byers wrote: > nit: can you please add the pointer ID to these log messages, just so we can > easily see that it's the right pointer getting the 'gotpointercapture' etc? Done. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:1: <!DOCTYPE HTML> On 2016/05/17 14:20:01, Rick Byers wrote: > Thanks for the thorough test! > Not sure if the functionality is ready yet, but it would also be valuable to > have an uncaptured variant of this test (which does releasePointerCapture on > each touch). In particular, the interaction with touch events when the finger > is uncaptured is ... "interesting". As you guessed, the functionality is not here yet. That will be the very next change after this change to add that and I will add that test scenarios here in that case. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:20: <body> On 2016/05/17 14:20:01, Rick Byers wrote: > nit: layout tests normally omit body tags: > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... Done. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-capture-in-iframe.html:131: description("This test verifies clientX/Y of pointer events inside iframe."); On 2016/05/17 14:20:01, Rick Byers wrote: > This description seems wrong. Right. I copy pasted from the other test and forgot to change the description or remove the body tag. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:438: // Touch events should not go to text nodes. On 2016/05/17 14:20:01, Rick Byers wrote: > update comment, probably even file a bug to track updating behavior. Do mouse > events go to text nodes? Do pointer events on Edge? I don't know to be honest with you. This is the behavior from the previous code and I just didn't change it. I added a TODO to double check it later. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:476: touchInfos.append(touchInfo); On 2016/05/17 14:40:14, mustaq wrote: > > Is this preserving the existing behavior though? ... > > We already have a p1 bug for this: crbug.com/607588. We will follow the Edge > behavior here (see #1 in that bug), which makes the most sense. We have a nearly > complete patch for GestureTap, using uniqueTouchIds. > > So in summary, we expect this to change soon, and we don't need to file a bug. Yup. I haven't changed the behavior here and just kept what it was. Mustaq is looking into changing the behavior to follow Edge. I added the TODO as you suggested. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:279: if (m_touchSequenceDocument->frame()) { On 2016/05/17 14:20:01, Rick Byers wrote: > this code is mostly copy/paste with the code in PointerEventManager. Please use > a common helper function somewhere. I have looked at this a little bit more. They do look like the same although they call different functions for hit testing. It seems a bit more efficient to refactor it after the investigation for the text node. Is that okay to do it along with that? I added this to the TODO in PointerEventManager as well. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.h (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:50: // Does the hit-testing again if the original hit test result was not inside On 2016/05/17 14:20:01, Rick Byers wrote: > nit: what does the return value mean? Done. https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.h:56: WebInputEventResult handleTouchEvent( On 2016/05/17 14:20:01, Rick Byers wrote: > add comment describing how touchinfos are modified I added a comment. However the caller of the function doesn't care about this array anymore. Let me know if there is a better way to do that. I thought about using && but was not sure if this is better or that.
LGTM https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1971473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:279: if (m_touchSequenceDocument->frame()) { On 2016/05/17 16:40:44, Navid Zolghadr wrote: > On 2016/05/17 14:20:01, Rick Byers wrote: > > this code is mostly copy/paste with the code in PointerEventManager. Please > use > > a common helper function somewhere. > > I have looked at this a little bit more. They do look like the same although > they call different functions for hit testing. It seems a bit more efficient to > refactor it after the investigation for the text node. Is that okay to do it > along with that? I added this to the TODO in PointerEventManager as well. Sure, thanks!
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/1971473002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971473002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971473002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971473002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971473002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Move touch hit testing to PointerEventManager Move touch hit testing to PointerEventManager class to separate pointer event hit testing from touch event hit testing. Now touch events use the pointer event hit testing result if they were in the touch capturing iframe otherwise they do another hit testing. BUG=606822 ========== to ========== Move touch hit testing to PointerEventManager Move touch hit testing to PointerEventManager class to separate pointer event hit testing from touch event hit testing. Now touch events use the pointer event hit testing result if they were in the touch capturing iframe otherwise they do another hit testing. BUG=606822 Committed: https://crrev.com/5ce73f6e13fdb838a6c92430ca39538ed7e8a452 Cr-Commit-Position: refs/heads/master@{#394802} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5ce73f6e13fdb838a6c92430ca39538ed7e8a452 Cr-Commit-Position: refs/heads/master@{#394802} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
