Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1635863006:
Pointerevent capture APIs (Closed)
|
Created:
4 years, 11 months ago by Navid Zolghadr Modified:
4 years, 9 months ago CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPointerevent capture APIs
Adding setPointerCapture and releasePointerCapture APIs
It's the first one in a series of planned changes to implement capture.
Adds basic capturing capabilities for mouse using the set and release APIs.
We will cover got/lost firing in crbug/592280.
Transition events for mouse is blocking on crbug/587955.
BUG=580169
Committed: https://crrev.com/ded5afeb2fc486424ed5c5b2b135edbd708d4a52
Cr-Commit-Position: refs/heads/master@{#379558}
Patch Set 1 #Patch Set 2 : #
Total comments: 22
Patch Set 3 : Rebasing and applying the comments #Patch Set 4 : Yet another rebase #
Total comments: 48
Patch Set 5 : Applying comments #Patch Set 6 : Rebasing #Patch Set 7 : #Patch Set 8 : #Messages
Total messages: 35 (6 generated)
Here is an initial implementation of pointer captures. There are a few things missing and I don't yet know the answer to. Let me know if you have any suggestions. 1. How to take care of an Element* if it gets deleted to cleanup my data structure in PointerEventManager. 2. How to hide the new APIs behind the flag. I couldn't find any existing example in Element.idl for a flag enabled API.
nzolghadr@chromium.org changed reviewers: + mustaq@chromium.org, rbyers@chromium.org
I added the tests and a few things that were missing in the first patch to this second patch. I also just realized that I forgot to add you guys to the first patch I sent out last week. ptal.
On 2016/01/26 21:30:33, Navid Zolghadr wrote: > Here is an initial implementation of pointer captures. There are a few things > missing and I don't yet know the answer to. Let me know if you have any > suggestions. > > 1. How to take care of an Element* if it gets deleted to cleanup my data > structure in PointerEventManager. You probably want something similar to m_targetForTouchID - a RefCountedWillBeMember<Element> per point that keeps the node alive until no pointers are captured to it. Even if an element is removed from the DOM, it may still have event listeners on it that should continue to fire. In fact you probably want to re-use this field since it serves exactly this purpose for touch - just generalize it to be the capture element per pointerID. > 2. How to hide the new APIs behind the flag. I couldn't find any existing > example in Element.idl for a flag enabled API. Just put [RuntimeEnabled=PointerEvent] on the methods like for the PointerEvent interface itself (see other Element members like setApplyScroll). On 2016/02/01 21:16:44, Navid Zolghadr wrote: > I added the tests and a few things that were missing in the first patch to this > second patch. I also just realized that I forgot to add you guys to the first > patch I sent out last week. ptal. Sorry for the delay on this. I'm about to head out on vacation, but please don't block landing CLs on me while I'm out. mustaq@ please take a look and iterate with Navid (bokan@ or tdresser@ may be able to help if you have input questions). Once you guys are happy, you can ask for an OWNERS stamp from someone like skobes@ or vollick@. If there are concerns, I can always review retroactively for follow-up changes after I'm back.
I took a quick skim through before I left. The basic approach seems reasonable to me. I see that you can't just use the existing touch capture field of EventHandler because you also need to track the "pending capture target". But still you probably want to use 2, not 3. I guess the key question here is what the behavior of touch events should be once the pointer has been captured to a different node. As for mouse events, I assume changing the pointer capture changes the target of the touch event too, but it would be worth checking to see what Edge does (not that we necessarily want to match them exactly here - remember that we still want implicit capture for touch-based pointer events). If we really do want to target them separately then keeping it separate like you've done is what we'd want.
Here are my comments so far. Interacting with implicit touch capturing seems tricky as Rick suggested. I think once we have the PE capture working for mouse, we can replace existing implicit touch capturing logic to use the PE capture. We may need a shortcut to avoid the lazy capturing though. Sorry for the delay. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html:21: description("Verifies that pointer capture APIs functionality."); Perhaps: "Verifies that pointer capture works for mouse."? https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html:36: targetDiv.setPointerCapture(event.pointerId); Are we testing only "setPointerCapture()" and not explicit "releasePointerCapture()"? https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html:57: eventSender.mouseMoveTo(x1, y1); It's hard to match the long output to the input tasks. Please consider these: - Don't output a message unless event.eventPhase == Event.AT_TARGET - Print a few strings in runTests to mark "task" groups in output. - Nuke unrelated tasks, e.g., the first two mouseMoveTo's. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.js (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.js:83: ++code; Curious: any clue why DomException.cpp |code| doesn't even match these? Which one is visible from JS? https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ExceptionCode.h (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ExceptionCode.h:35: IndexSizeError = 1, Wow, yet another definition of DOM exception, now in the same directory! We need to file a bug, at least the two in this folder should be combined. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventManager.h (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventManager.h:79: HashMap<IncomingId, int, WTF::PairHash<int, int>, WTF::PairHashTraits<WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::UnsignedWithZeroKeyHashTraits<int>>> m_idMapping; Please combine the next two hashmaps into one, this should halve the hash overhead: int -> struct{IncomingId, bool} Also, to improve readability, make a local typedef for WTF::UnsignedWithZeroKeyHashTraits<int>. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventManager.h:81: HashMap<int, int, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> m_lastButtons; Can you make it <int, bool>? s/m_lastButtons/m_isActive/? https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1665: EventTarget* target = m_pointerEventManager.getMouseCapturingNode(); s/target/capturingNode/ https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1669: exitedNode = nullptr; Perhaps this won't happen but not sure: any chance this could be wrong when the pointer was first captured? https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1813: int isAncestor = false; I guess you can avoid the loop below for mouseover & mouseout, right? If this is the case, please make it conditional. Alternatively, can we instead trim the node parent lists even in sendNodeTransitionEvents(), to save some iterations there? https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1815: if (node == target) { I think the loop should start at node = capturingNode, right? https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1820: // Suppress these events if the target is not the capturing element inclusive ancestors Is it clearer? "...if target is not an (inclusive) ancestor of the capturing element"
https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html:21: description("Verifies that pointer capture APIs functionality."); On 2016/02/06 04:36:31, mustaq wrote: > Perhaps: "Verifies that pointer capture works for mouse."? Will do. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html:36: targetDiv.setPointerCapture(event.pointerId); On 2016/02/06 04:36:31, mustaq wrote: > Are we testing only "setPointerCapture()" and not explicit > "releasePointerCapture()"? You are right. I totally missed that. I will add the tests for that as well. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture.html:57: eventSender.mouseMoveTo(x1, y1); On 2016/02/06 04:36:31, mustaq wrote: > It's hard to match the long output to the input tasks. Please consider these: > - Don't output a message unless event.eventPhase == Event.AT_TARGET > - Print a few strings in runTests to mark "task" groups in output. > - Nuke unrelated tasks, e.g., the first two mouseMoveTo's. Sure. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.js (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.js:83: ++code; On 2016/02/06 04:36:31, mustaq wrote: > Curious: any clue why DomException.cpp |code| doesn't even match these? Which > one is visible from JS? Which part of it doesn't match with these? I can't see it. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ExceptionCode.h (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ExceptionCode.h:35: IndexSizeError = 1, On 2016/02/06 04:36:31, mustaq wrote: > Wow, yet another definition of DOM exception, now in the same directory! We need > to file a bug, at least the two in this folder should be combined. I filed a bug for this. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventManager.h (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventManager.h:79: HashMap<IncomingId, int, WTF::PairHash<int, int>, WTF::PairHashTraits<WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::UnsignedWithZeroKeyHashTraits<int>>> m_idMapping; On 2016/02/06 04:36:31, mustaq wrote: > Please combine the next two hashmaps into one, this should halve the hash > overhead: > int -> struct{IncomingId, bool} > > Also, to improve readability, make a local typedef for > WTF::UnsignedWithZeroKeyHashTraits<int>. Did you mean m_idReverseMapping and m_lastButtons (-> m_isActive)? Sure. I will do that. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventManager.h:81: HashMap<int, int, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>> m_lastButtons; On 2016/02/06 04:36:31, mustaq wrote: > Can you make it <int, bool>? s/m_lastButtons/m_isActive/? Although right now I only use it as bool. But I thought later that I will fire got/lostpointercapture I need to read this field. But for now I change it to bool. Maybe later I can get away by passing some extra parameters around. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1665: EventTarget* target = m_pointerEventManager.getMouseCapturingNode(); On 2016/02/06 04:36:31, mustaq wrote: > s/target/capturingNode/ Done. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1669: exitedNode = nullptr; On 2016/02/06 04:36:31, mustaq wrote: > Perhaps this won't happen but not sure: any chance this could be wrong when the > pointer was first captured? Do you have any particular scenario in mind? I can't imagine any problematic one here. https://codereview.chromium.org/1635863006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1813: int isAncestor = false; On 2016/02/06 04:36:31, mustaq wrote: > I guess you can avoid the loop below for mouseover & mouseout, right? If this is > the case, please make it conditional. > > Alternatively, can we instead trim the node parent lists even in > sendNodeTransitionEvents(), to save some iterations there? I believe I can get rid of this for now as the change I made in sendNodeTransitionEvents will take care of this. I started by adding this change and later that I added the changes in sendNodeTransitionEvents I forgot to remove these changes here. Basically this change has not effect as the for loop always return true and there is no need for this portion of the change here.
I rebased this CL and applied all the comments. Hopefully now the change is smaller and cleaner to review. ptal.
Please clarify processPendingPointerCapture ordering. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:73: 57 green received pointerover Reset the counter here. Or better drop the counter completely, doesn't add much value here. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:96: if (getCapturingNode(pointerId)) Avoid repeated calls to getCapturingNode. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:130: if (getCapturingNode(pointerEvent->pointerId())) { Ditto. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:322: if (pointerEvent->buttons() == 0) Here |buttons| can be zero thru either finger lifted off the screen or touchcancel. So the above |if| with removePointerEvent sufficient, right? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:392: PassRefPtrWillBeRawPtr<EventTarget> hitTestTarget, const ... hitTestTarget. Same for the local vars below. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:404: if (hasPendingPointerCaptureTarget Your ordering is different from the spec algorithm. It is because of pointerleave vs pointerenter relative ordering? I thought leave vs enter firing happens on disjoint cases, so the inverted ordering here is not needed. Moreover, we would want to maintain the got-vs-lost pointercapture event order. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:416: if (!m_pendingPointerCaptureTarget.contains(pointerId)) if (!has...) https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:430: void PointerEventManager::removeTargetFromPointerCapturingMapping(PointerCapturingMap& map, EventTarget* target) const EventTarget* https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; Why do you need tmp here? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:482: if (m_pendingPointerCaptureTarget.contains(pointerId)) s/if/ASSERT/ https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:60: void implicitReleasePointerCapture(int); implicitReleasePointerCapture could be private. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:65: class NodeUnderPointerAttributes { EventTargetAttributes?
https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:73: 57 green received pointerover On 2016/02/29 19:55:40, mustaq wrote: > Reset the counter here. Or better drop the counter completely, doesn't add much > value here. Sure. I'll drop it. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:96: if (getCapturingNode(pointerId)) On 2016/02/29 19:55:40, mustaq wrote: > Avoid repeated calls to getCapturingNode. Done. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:130: if (getCapturingNode(pointerEvent->pointerId())) { On 2016/02/29 19:55:40, mustaq wrote: > Ditto. Done. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:322: if (pointerEvent->buttons() == 0) On 2016/02/29 19:55:40, mustaq wrote: > Here |buttons| can be zero thru either finger lifted off the screen or > touchcancel. So the above |if| with removePointerEvent sufficient, right? I believe you are right. I can't remember of any reason to have it here. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:392: PassRefPtrWillBeRawPtr<EventTarget> hitTestTarget, On 2016/02/29 19:55:40, mustaq wrote: > const ... hitTestTarget. > > Same for the local vars below. Sure. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:404: if (hasPendingPointerCaptureTarget On 2016/02/29 19:55:40, mustaq wrote: > Your ordering is different from the spec algorithm. It is because of > pointerleave vs pointerenter relative ordering? I thought leave vs enter firing > happens on disjoint cases, so the inverted ordering here is not needed. > Moreover, we would want to maintain the got-vs-lost pointercapture event order. It is because of the sendNodeTransitionEvents function. That function cares about the m_pointerCaptureTarget. As a result I need to fire the leave/out events before setting anything to that map and fire the enter/over events after remove the target from the map. At the end as you said the result is the same as the spec because they don't happen together. Is this fine? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:416: if (!m_pendingPointerCaptureTarget.contains(pointerId)) On 2016/02/29 19:55:40, mustaq wrote: > if (!has...) Done. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:430: void PointerEventManager::removeTargetFromPointerCapturingMapping(PointerCapturingMap& map, EventTarget* target) On 2016/02/29 19:55:40, mustaq wrote: > const EventTarget* Done. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/02/29 19:55:40, mustaq wrote: > Why do you need tmp here? I was going over the map. So I assumed I cannot modify the container that I'm traversing. Is that right or does this WTF map takes care of it somehow? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:482: if (m_pendingPointerCaptureTarget.contains(pointerId)) On 2016/02/29 19:55:40, mustaq wrote: > s/if/ASSERT/ Done. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:60: void implicitReleasePointerCapture(int); On 2016/02/29 19:55:41, mustaq wrote: > implicitReleasePointerCapture could be private. Yup. I forgot to move it to private. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:65: class NodeUnderPointerAttributes { On 2016/02/29 19:55:40, mustaq wrote: > EventTargetAttributes? Sure. I Will rename.
This is looking really good, thanks Navid! It's a little scary how much complexity there is around pointer capture and the relationship to transition events and legacy events. I didn't study all the edge cases in detail, I'm sure we'll continue to tweak the behavior as we do more compat testing against Edge etc. You've done a good job of abstracting the complexity into separate classes, so I'm not too worried about it. What's the interaction of touch events and pointer capture after this CL? I see your test looks just as mouse events - is your intent to focus on the mouse case first then work on touch as a follow-up? That would make sense to me. One disadvantage to the separation you have now between EventHandler and PointerEventManager is that EventHandler still assumes that it it's responsibility to target the events, and then we override that just for the purpose of dispatching DOM events. This has a couple problems: 1. It means there may be scenarios where we're doing a hit-test unnecessarily. Not a huge deal for mouse (since we do hit-tests on mousemove by default), but we definitely don't want to 2. There are likely some places where EventHandler is using it's notion of the target node when it should be using the capturing node. Eg. the default event handler or other system behavior (for example, if I mousedown on a form element that captures the pointer, then drag off and lift you should send the click event to the capturing element as well as set focus there and perform other activation behavior). To fix this you'll probably want to ultimately take capturing into account as early as possible - maybe as part of hit-testing (eg. in prepareMouseEvent). I can see how this CL can still be a step in that direction though (eg. maybe prepareMouseEvent calls into the PointerEventManager to ask for capturing state). So I'm OK with landing this as-is and iterating further. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:26: 17 grey received pointerleave is this event expected (does Edge get it in a similar situation)? I thought the spec said you should only get leave events when crossing the boundary of the element that has capture? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1641: m_pointerEventManager.setPointerCapture(pointerId, target); As discussed offline, I think keeping this state per-frame is going to cause bugs in cases where the point moves between iframes. It's a pre-existing issue with the PointerEventManager though, so I'd suggest filing a bug for moving PointerEventManager from EventHandler to FrameHost and adding a TODO referencing that bug here. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/02/29 20:10:42, Navid Zolghadr wrote: > On 2016/02/29 19:55:40, mustaq wrote: > > Why do you need tmp here? > > I was going over the map. So I assumed I cannot modify the container that I'm > traversing. Is that right or does this WTF map takes care of it somehow? I think you can (see the implementation of removeAll in HashTable.h). https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:450: void PointerEventManager::removePointerEvent( seems a little weird to me for this to take a PointerEvent instead of a pointerId (looks like PointerEventFactory::remove only uses the ID as well). I was worried for a moment that we might actually be storing PointerEvent instances somewhere. Anyway I don't have super strong opinion here. If you feel it's best to take a PointerEvent, how about renaming this method to "removePointer" to make it clearer that it removes the tracking for the pointer indicated by the supplied event, not actually removing the event from a list of events or something? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:480: void PointerEventManager::implicitReleasePointerCapture(int pointerId) nit: rename this just releasePointerCapture since it's also called in the explicit case?
Re who is in charge of the target for event dispatch: I am inclined to isolation of PE targeting off from EventHandler because we need separate states per pointerId which are different from legacy mouse anyway. This is in addition to the need to make EventHandler more modular from it's current "soup" form. Rick, do you see any per-page-vs-per-frame state issue if we move PEs off from EventHandler? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:26: 17 grey received pointerleave On 2016/03/01 16:58:34, Rick Byers wrote: > is this event expected (does Edge get it in a similar situation)? I thought the > spec said you should only get leave events when crossing the boundary of the > element that has capture? I think the MouseEvent spec requires us to fire this because the PE spec implies that the mouse has left "green" so "grey" would be receiving a *mouse*leave. The PE spec doesn't override the mouseenter/leave definition. Moreover, at this point JS would have no knowledge of where the mouse is (except that it is not on green). If green was covering the whole page, this is equivalent to mouse leaving the page. (Finally, internally it makes most sense to assume that the mouse has left the page, rather than hovering on grey-minus-green area.) https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/03/01 16:58:35, Rick Byers wrote: > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > On 2016/02/29 19:55:40, mustaq wrote: > > > Why do you need tmp here? > > > > I was going over the map. So I assumed I cannot modify the container that I'm > > traversing. Is that right or does this WTF map takes care of it somehow? > > I think you can (see the implementation of removeAll in HashTable.h). And in this case, you don't need to go further after finding the key. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:450: void PointerEventManager::removePointerEvent( On 2016/03/01 16:58:35, Rick Byers wrote: > seems a little weird to me for this to take a PointerEvent instead of a > pointerId (looks like PointerEventFactory::remove only uses the ID as well). I > was worried for a moment that we might actually be storing PointerEvent > instances somewhere. > > Anyway I don't have super strong opinion here. If you feel it's best to take a > PointerEvent, how about renaming this method to "removePointer" to make it > clearer that it removes the tracking for the pointer indicated by the supplied > event, not actually removing the event from a list of events or something? I will take the blame for PointerEventFactory::remove() taking a PE instead of an |int|. I wanted it before I proposed the Manager/Factory split, to hide the |int| from EvenHandler. It can be an |int| now. In fact, my current local patch for touch-action fix (crrev.com/1732043002) has done the same. For PointerEventManager, it is still safer to pass the event rather than an |int| id. I think Rick's renaming suggestion is better.
On 2016/03/01 17:54:58, mustaq wrote: > Re who is in charge of the target for event dispatch: I am inclined to isolation > of PE targeting off from EventHandler because we need separate states per > pointerId which are different from legacy mouse anyway. This is in addition to > the need to make EventHandler more modular from it's current "soup" form. > > Rick, do you see any per-page-vs-per-frame state issue if we move PEs off from > EventHandler? > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt > (right): > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:26: > 17 grey received pointerleave > On 2016/03/01 16:58:34, Rick Byers wrote: > > is this event expected (does Edge get it in a similar situation)? I thought > the > > spec said you should only get leave events when crossing the boundary of the > > element that has capture? > > I think the MouseEvent spec requires us to fire this because the PE spec implies > that the mouse has left "green" so "grey" would be receiving a *mouse*leave. The > PE spec doesn't override the mouseenter/leave definition. > > Moreover, at this point JS would have no knowledge of where the mouse is (except > that it is not on green). If green was covering the whole page, this is > equivalent to mouse leaving the page. > > (Finally, internally it makes most sense to assume that the mouse has left the > page, rather than hovering on grey-minus-green area.) > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: > PointerCapturingMap tmp = map; > On 2016/03/01 16:58:35, Rick Byers wrote: > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > Why do you need tmp here? > > > > > > I was going over the map. So I assumed I cannot modify the container that > I'm > > > traversing. Is that right or does this WTF map takes care of it somehow? > > > > I think you can (see the implementation of removeAll in HashTable.h). > > And in this case, you don't need to go further after finding the key. > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:450: void > PointerEventManager::removePointerEvent( > On 2016/03/01 16:58:35, Rick Byers wrote: > > seems a little weird to me for this to take a PointerEvent instead of a > > pointerId (looks like PointerEventFactory::remove only uses the ID as well). > I > > was worried for a moment that we might actually be storing PointerEvent > > instances somewhere. > > > > Anyway I don't have super strong opinion here. If you feel it's best to take > a > > PointerEvent, how about renaming this method to "removePointer" to make it > > clearer that it removes the tracking for the pointer indicated by the supplied > > event, not actually removing the event from a list of events or something? > > I will take the blame for PointerEventFactory::remove() taking a PE instead of > an |int|. I wanted it before I proposed the Manager/Factory split, to hide the > |int| from EvenHandler. It can be an |int| now. > > In fact, my current local patch for touch-action fix (crrev.com/1732043002) has > done the same. > > For PointerEventManager, it is still safer to pass the event rather than an > |int| id. I think Rick's renaming suggestion is better. I personally find it confusing we have notions of capturing behavior at different levels. eg; https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... A FrameHost based capturing state seems best to me; but I agree that the whole thing is a work in progress.
On 2016/03/01 18:04:38, dtapuska wrote: > On 2016/03/01 17:54:58, mustaq wrote: > > Re who is in charge of the target for event dispatch: I am inclined to > isolation > > of PE targeting off from EventHandler because we need separate states per > > pointerId which are different from legacy mouse anyway. This is in addition to > > the need to make EventHandler more modular from it's current "soup" form. > > > > Rick, do you see any per-page-vs-per-frame state issue if we move PEs off from > > EventHandler? > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... > > File > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt > > (right): > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:26: > > 17 grey received pointerleave > > On 2016/03/01 16:58:34, Rick Byers wrote: > > > is this event expected (does Edge get it in a similar situation)? I thought > > the > > > spec said you should only get leave events when crossing the boundary of the > > > element that has capture? > > > > I think the MouseEvent spec requires us to fire this because the PE spec > implies > > that the mouse has left "green" so "grey" would be receiving a *mouse*leave. > The > > PE spec doesn't override the mouseenter/leave definition. > > > > Moreover, at this point JS would have no knowledge of where the mouse is > (except > > that it is not on green). If green was covering the whole page, this is > > equivalent to mouse leaving the page. > > > > (Finally, internally it makes most sense to assume that the mouse has left the > > page, rather than hovering on grey-minus-green area.) > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: > > PointerCapturingMap tmp = map; > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > Why do you need tmp here? > > > > > > > > I was going over the map. So I assumed I cannot modify the container that > > I'm > > > > traversing. Is that right or does this WTF map takes care of it somehow? > > > > > > I think you can (see the implementation of removeAll in HashTable.h). > > > > And in this case, you don't need to go further after finding the key. > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/input/PointerEventManager.cpp:450: void > > PointerEventManager::removePointerEvent( > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > seems a little weird to me for this to take a PointerEvent instead of a > > > pointerId (looks like PointerEventFactory::remove only uses the ID as well). > > > I > > > was worried for a moment that we might actually be storing PointerEvent > > > instances somewhere. > > > > > > Anyway I don't have super strong opinion here. If you feel it's best to > take > > a > > > PointerEvent, how about renaming this method to "removePointer" to make it > > > clearer that it removes the tracking for the pointer indicated by the > supplied > > > event, not actually removing the event from a list of events or something? > > > > I will take the blame for PointerEventFactory::remove() taking a PE instead of > > an |int|. I wanted it before I proposed the Manager/Factory split, to hide the > > |int| from EvenHandler. It can be an |int| now. > > > > In fact, my current local patch for touch-action fix (crrev.com/1732043002) > has > > done the same. > > > > For PointerEventManager, it is still safer to pass the event rather than an > > |int| id. I think Rick's renaming suggestion is better. > > I personally find it confusing we have notions of capturing behavior at > different levels. > eg; > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > A FrameHost based capturing state seems best to me; but I agree that the whole > thing is a work in progress. Yes, FrameHost/LocalFrame level capturing makes the most sense logically. However, we have to go there incrementally, and starting at EventHandler seems reasonable because pointer capture would affect most things at the EventHandler level IMO. Here is a list of event-targeting related "concepts" that could be conflicting with pointer capture, please correct me if I missed something here: A. Capturing for plugins at WebViewImpl. B. Implicit touch-capturing at EventHandler. C. Mouse-drag capturing at EventHandler. D. Hittesting in various levels from WebViewImpl to EventHandler. I assumed that Rick's suggestion re prepareMouseEvent is at the EventHandler level, am I right?
https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:26: 17 grey received pointerleave On 2016/03/01 17:54:57, mustaq wrote: > On 2016/03/01 16:58:34, Rick Byers wrote: > > is this event expected (does Edge get it in a similar situation)? I thought > the > > spec said you should only get leave events when crossing the boundary of the > > element that has capture? > > I think the MouseEvent spec requires us to fire this because the PE spec implies > that the mouse has left "green" so "grey" would be receiving a *mouse*leave. The > PE spec doesn't override the mouseenter/leave definition. > > Moreover, at this point JS would have no knowledge of where the mouse is (except > that it is not on green). If green was covering the whole page, this is > equivalent to mouse leaving the page. > > (Finally, internally it makes most sense to assume that the mouse has left the > page, rather than hovering on grey-minus-green area.) Just to add on what Mustaq said, Edge does the same thing as well. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1641: m_pointerEventManager.setPointerCapture(pointerId, target); On 2016/03/01 16:58:34, Rick Byers wrote: > As discussed offline, I think keeping this state per-frame is going to cause > bugs in cases where the point moves between iframes. It's a pre-existing issue > with the PointerEventManager though, so I'd suggest filing a bug for moving > PointerEventManager from EventHandler to FrameHost and adding a TODO referencing > that bug here. I will file that bug but I believe the TODO belongs somewhere beyond this particular function and this change. But if you still like me to add the TODO here I'll add it. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/03/01 17:54:58, mustaq wrote: > On 2016/03/01 16:58:35, Rick Byers wrote: > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > Why do you need tmp here? > > > > > > I was going over the map. So I assumed I cannot modify the container that > I'm > > > traversing. Is that right or does this WTF map takes care of it somehow? > > > > I think you can (see the implementation of removeAll in HashTable.h). > > And in this case, you don't need to go further after finding the key. If I correctly understand you Mustaq, I don't think I can stop after finding the first instance. Note that I'm searching for all the entries with a particular value and an element may capture multiple pointers at the same time and as a result have multiple entries in this map with the value equal that node. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:450: void PointerEventManager::removePointerEvent( On 2016/03/01 17:54:57, mustaq wrote: > On 2016/03/01 16:58:35, Rick Byers wrote: > > seems a little weird to me for this to take a PointerEvent instead of a > > pointerId (looks like PointerEventFactory::remove only uses the ID as well). > I > > was worried for a moment that we might actually be storing PointerEvent > > instances somewhere. > > > > Anyway I don't have super strong opinion here. If you feel it's best to take > a > > PointerEvent, how about renaming this method to "removePointer" to make it > > clearer that it removes the tracking for the pointer indicated by the supplied > > event, not actually removing the event from a list of events or something? > > I will take the blame for PointerEventFactory::remove() taking a PE instead of > an |int|. I wanted it before I proposed the Manager/Factory split, to hide the > |int| from EvenHandler. It can be an |int| now. > > In fact, my current local patch for touch-action fix (crrev.com/1732043002) has > done the same. > > For PointerEventManager, it is still safer to pass the event rather than an > |int| id. I think Rick's renaming suggestion is better. Sure. Then I'll keep the int but with "removePointer" name. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:480: void PointerEventManager::implicitReleasePointerCapture(int pointerId) On 2016/03/01 16:58:35, Rick Byers wrote: > nit: rename this just releasePointerCapture since it's also called in the > explicit case? Is that okay to have the same name for both functions or do you want me to rename the other one to explicit?
ptal. Regarding the interaction of touch and pointer capture APIs I wanted to address it in another CL as that needs changing some of the existing capturing mechanisms for touch events. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/03/01 20:46:05, Navid Zolghadr wrote: > On 2016/03/01 17:54:58, mustaq wrote: > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > Why do you need tmp here? > > > > > > > > I was going over the map. So I assumed I cannot modify the container that > > I'm > > > > traversing. Is that right or does this WTF map takes care of it somehow? > > > > > > I think you can (see the implementation of removeAll in HashTable.h). > > > > And in this case, you don't need to go further after finding the key. > > If I correctly understand you Mustaq, I don't think I can stop after finding the > first instance. Note that I'm searching for all the entries with a particular > value and an element may capture multiple pointers at the same time and as a > result have multiple entries in this map with the value equal that node. I still could't figure out how not to create a tmp map here. The removeAll implementation had 2 sets and it is removing one set from the other one. Can you give me a few more hints? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:482: if (m_pendingPointerCaptureTarget.contains(pointerId)) On 2016/02/29 20:10:42, Navid Zolghadr wrote: > On 2016/02/29 19:55:40, mustaq wrote: > > s/if/ASSERT/ > > Done. I just remembered that this function is being called even when we don't have any capturing for a given pointerId like when the buttons=0. So we don't want to have that assert here. But I removed the if as I believe remove just does nothing if the key is not found.
Rick, was I right to interpret that your prepareMouseEvent suggestion was equivalent to start fixing incrementally from EventHandler to FrameHost? https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/03/02 16:54:49, Navid Zolghadr wrote: > On 2016/03/01 20:46:05, Navid Zolghadr wrote: > > On 2016/03/01 17:54:58, mustaq wrote: > > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > > Why do you need tmp here? > > > > > > > > > > I was going over the map. So I assumed I cannot modify the container > that > > > I'm > > > > > traversing. Is that right or does this WTF map takes care of it somehow? > > > > > > > > I think you can (see the implementation of removeAll in HashTable.h). > > > > > > And in this case, you don't need to go further after finding the key. > > > > If I correctly understand you Mustaq, I don't think I can stop after finding > the > > first instance. Note that I'm searching for all the entries with a particular > > value and an element may capture multiple pointers at the same time and as a > > result have multiple entries in this map with the value equal that node. > > I still could't figure out how not to create a tmp map here. The removeAll > implementation had 2 sets and it is removing one set from the other one. Can you > give me a few more hints? I guess I was thinking about Java iterators which lets you delete and move ahead. I guess we have to live with the map copy. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:482: if (m_pendingPointerCaptureTarget.contains(pointerId)) On 2016/03/02 16:54:49, Navid Zolghadr wrote: > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > On 2016/02/29 19:55:40, mustaq wrote: > > > s/if/ASSERT/ > > > > Done. > > I just remembered that this function is being called even when we don't have any > capturing for a given pointerId like when the buttons=0. So we don't want to > have that assert here. But I removed the if as I believe remove just does > nothing if the key is not found. Then both these release functions remove ids conditionally, I guess the distinction is not clear. "releasePointerCaptureFromTarget" and "releasePointerCapture"?
> Rick, was I right to interpret that your prepareMouseEvent suggestion was > equivalent to start fixing incrementally from EventHandler to FrameHost? I _think_ these questions are orthogonal. Yes we should incrementally (in follow up patches) move state from EventHandler to FrameHost. Separately we should ensure ALL touch/mouse hit-testing takes capture into account, such as by modifying key hit-test paths like prepareMouseEvent to consult the capture state. These changes could happen in either order from what I see - whatever you find easier is fine with me. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture-expected.txt:26: 17 grey received pointerleave On 2016/03/01 20:46:05, Navid Zolghadr wrote: > On 2016/03/01 17:54:57, mustaq wrote: > > On 2016/03/01 16:58:34, Rick Byers wrote: > > > is this event expected (does Edge get it in a similar situation)? I thought > > the > > > spec said you should only get leave events when crossing the boundary of the > > > element that has capture? > > > > I think the MouseEvent spec requires us to fire this because the PE spec > implies > > that the mouse has left "green" so "grey" would be receiving a *mouse*leave. > The > > PE spec doesn't override the mouseenter/leave definition. > > > > Moreover, at this point JS would have no knowledge of where the mouse is > (except > > that it is not on green). If green was covering the whole page, this is > > equivalent to mouse leaving the page. > > > > (Finally, internally it makes most sense to assume that the mouse has left the > > page, rather than hovering on grey-minus-green area.) > > Just to add on what Mustaq said, Edge does the same thing as well. Ok, thank you. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1641: m_pointerEventManager.setPointerCapture(pointerId, target); On 2016/03/01 20:46:05, Navid Zolghadr wrote: > On 2016/03/01 16:58:34, Rick Byers wrote: > > As discussed offline, I think keeping this state per-frame is going to cause > > bugs in cases where the point moves between iframes. It's a pre-existing > issue > > with the PointerEventManager though, so I'd suggest filing a bug for moving > > PointerEventManager from EventHandler to FrameHost and adding a TODO > referencing > > that bug here. > > I will file that bug but I believe the TODO belongs somewhere beyond this > particular function and this change. But if you still like me to add the TODO > here I'll add it. Sure, that's fine. I expect this all to change relatively soon so the TODOs don't matter much (but the bugs tracking our planned work do). https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/03/02 21:12:32, mustaq wrote: > On 2016/03/02 16:54:49, Navid Zolghadr wrote: > > On 2016/03/01 20:46:05, Navid Zolghadr wrote: > > > On 2016/03/01 17:54:58, mustaq wrote: > > > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > > > Why do you need tmp here? > > > > > > > > > > > > I was going over the map. So I assumed I cannot modify the container > > that > > > > I'm > > > > > > traversing. Is that right or does this WTF map takes care of it > somehow? > > > > > > > > > > I think you can (see the implementation of removeAll in HashTable.h). > > > > > > > > And in this case, you don't need to go further after finding the key. > > > > > > If I correctly understand you Mustaq, I don't think I can stop after finding > > the > > > first instance. Note that I'm searching for all the entries with a > particular > > > value and an element may capture multiple pointers at the same time and as a > > > result have multiple entries in this map with the value equal that node. > > > > I still could't figure out how not to create a tmp map here. The removeAll > > implementation had 2 sets and it is removing one set from the other one. Can > you > > give me a few more hints? > > I guess I was thinking about Java iterators which lets you delete and move > ahead. I guess we have to live with the map copy. Yeah sorry I mis-read the removeAll code. Anyway this isn't worth debating about - the set should always be small (in fact a linear list is likely more efficient than a hash table since the size is almost always one or two and never more than about 10). As-is is fine. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:480: void PointerEventManager::implicitReleasePointerCapture(int pointerId) On 2016/03/01 20:46:05, Navid Zolghadr wrote: > On 2016/03/01 16:58:35, Rick Byers wrote: > > nit: rename this just releasePointerCapture since it's also called in the > > explicit case? > > Is that okay to have the same name for both functions or do you want me to > rename the other one to explicit? Yep and overload seems fine to me (they're conceptually the same thing).
Oh and LGTM overall. Please get mustaq@'s approval as well before landing (eg. for some of the details I've glossed over).
I will take another pass over the CL by eod. The comment below is minor, can be done separately. https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: PointerCapturingMap tmp = map; On 2016/03/03 16:35:45, Rick Byers wrote: > On 2016/03/02 21:12:32, mustaq wrote: > > On 2016/03/02 16:54:49, Navid Zolghadr wrote: > > > On 2016/03/01 20:46:05, Navid Zolghadr wrote: > > > > On 2016/03/01 17:54:58, mustaq wrote: > > > > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > > > > Why do you need tmp here? > > > > > > > > > > > > > > I was going over the map. So I assumed I cannot modify the container > > > that > > > > > I'm > > > > > > > traversing. Is that right or does this WTF map takes care of it > > somehow? > > > > > > > > > > > > I think you can (see the implementation of removeAll in HashTable.h). > > > > > > > > > > > And in this case, you don't need to go further after finding the key. > > > > > > > > If I correctly understand you Mustaq, I don't think I can stop after > finding > > > the > > > > first instance. Note that I'm searching for all the entries with a > > particular > > > > value and an element may capture multiple pointers at the same time and as > a > > > > result have multiple entries in this map with the value equal that node. > > > > > > I still could't figure out how not to create a tmp map here. The removeAll > > > implementation had 2 sets and it is removing one set from the other one. Can > > you > > > give me a few more hints? > > > > I guess I was thinking about Java iterators which lets you delete and move > > ahead. I guess we have to live with the map copy. > > Yeah sorry I mis-read the removeAll code. Anyway this isn't worth debating > about - the set should always be small (in fact a linear list is likely more > efficient than a hash table since the size is almost always one or two and never > more than about 10). As-is is fine. I am fine with tmp, sorry for the fuss. It just happens that crrev.com/1732043002/#ps80001 sends pointercancels in the order stored in the hashmap, so the layout test outcome becomes dependent on hashmap's own gc. While that patch is likely to die, I expect that the fire-all-touch-cancels approach will be there in my final solution. So, in addition to the performance nit mentioned by Rick, we now have pointercancel ordering stability plus code maintenance ease in favor of replacing the two hashmaps with a single list.
On 2016/03/03 17:42:48, mustaq wrote: > I will take another pass over the CL by eod. The comment below is minor, can be > done separately. > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: > PointerCapturingMap tmp = map; > On 2016/03/03 16:35:45, Rick Byers wrote: > > On 2016/03/02 21:12:32, mustaq wrote: > > > On 2016/03/02 16:54:49, Navid Zolghadr wrote: > > > > On 2016/03/01 20:46:05, Navid Zolghadr wrote: > > > > > On 2016/03/01 17:54:58, mustaq wrote: > > > > > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > > > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > > > > > Why do you need tmp here? > > > > > > > > > > > > > > > > I was going over the map. So I assumed I cannot modify the > container > > > > that > > > > > > I'm > > > > > > > > traversing. Is that right or does this WTF map takes care of it > > > somehow? > > > > > > > > > > > > > > I think you can (see the implementation of removeAll in > HashTable.h). > > > > > > > > > > > > > > And in this case, you don't need to go further after finding the key. > > > > > > > > > > If I correctly understand you Mustaq, I don't think I can stop after > > finding > > > > the > > > > > first instance. Note that I'm searching for all the entries with a > > > particular > > > > > value and an element may capture multiple pointers at the same time and > as > > a > > > > > result have multiple entries in this map with the value equal that node. > > > > > > > > I still could't figure out how not to create a tmp map here. The removeAll > > > > implementation had 2 sets and it is removing one set from the other one. > Can > > > you > > > > give me a few more hints? > > > > > > I guess I was thinking about Java iterators which lets you delete and move > > > ahead. I guess we have to live with the map copy. > > > > Yeah sorry I mis-read the removeAll code. Anyway this isn't worth debating > > about - the set should always be small (in fact a linear list is likely more > > efficient than a hash table since the size is almost always one or two and > never > > more than about 10). As-is is fine. > > I am fine with tmp, sorry for the fuss. > > It just happens that crrev.com/1732043002/#ps80001 sends pointercancels in the > order stored in the hashmap, so the layout test outcome becomes dependent on > hashmap's own gc. While that patch is likely to die, I expect that the > fire-all-touch-cancels approach will be there in my final solution. > > So, in addition to the performance nit mentioned by Rick, we now have > pointercancel ordering stability plus code maintenance ease in favor of > replacing the two hashmaps with a single list. I can certainly change this to a list if we realize there is not going to be a lot of pointer capturing at the same time. Maybe in a separate CL as you suggested after getting some usage count and how people use the capturing APIs. But this particular hashmap here does not affect the ordering in which the events are fired. Do you agree?
On 2016/03/03 18:14:00, Navid Zolghadr wrote: > On 2016/03/03 17:42:48, mustaq wrote: > > I will take another pass over the CL by eod. The comment below is minor, can > be > > done separately. > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/input/PointerEventManager.cpp:436: > > PointerCapturingMap tmp = map; > > On 2016/03/03 16:35:45, Rick Byers wrote: > > > On 2016/03/02 21:12:32, mustaq wrote: > > > > On 2016/03/02 16:54:49, Navid Zolghadr wrote: > > > > > On 2016/03/01 20:46:05, Navid Zolghadr wrote: > > > > > > On 2016/03/01 17:54:58, mustaq wrote: > > > > > > > On 2016/03/01 16:58:35, Rick Byers wrote: > > > > > > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > > > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > > > > > > Why do you need tmp here? > > > > > > > > > > > > > > > > > > I was going over the map. So I assumed I cannot modify the > > container > > > > > that > > > > > > > I'm > > > > > > > > > traversing. Is that right or does this WTF map takes care of it > > > > somehow? > > > > > > > > > > > > > > > > I think you can (see the implementation of removeAll in > > HashTable.h). > > > > > > > > > > > > > > > > > And in this case, you don't need to go further after finding the > key. > > > > > > > > > > > > If I correctly understand you Mustaq, I don't think I can stop after > > > finding > > > > > the > > > > > > first instance. Note that I'm searching for all the entries with a > > > > particular > > > > > > value and an element may capture multiple pointers at the same time > and > > as > > > a > > > > > > result have multiple entries in this map with the value equal that > node. > > > > > > > > > > I still could't figure out how not to create a tmp map here. The > removeAll > > > > > implementation had 2 sets and it is removing one set from the other one. > > Can > > > > you > > > > > give me a few more hints? > > > > > > > > I guess I was thinking about Java iterators which lets you delete and move > > > > ahead. I guess we have to live with the map copy. > > > > > > Yeah sorry I mis-read the removeAll code. Anyway this isn't worth debating > > > about - the set should always be small (in fact a linear list is likely more > > > efficient than a hash table since the size is almost always one or two and > > never > > > more than about 10). As-is is fine. > > > > I am fine with tmp, sorry for the fuss. > > > > It just happens that crrev.com/1732043002/#ps80001 sends pointercancels in the > > order stored in the hashmap, so the layout test outcome becomes dependent on > > hashmap's own gc. While that patch is likely to die, I expect that the > > fire-all-touch-cancels approach will be there in my final solution. > > > > So, in addition to the performance nit mentioned by Rick, we now have > > pointercancel ordering stability plus code maintenance ease in favor of > > replacing the two hashmaps with a single list. > > I can certainly change this to a list if we realize there is not going to be a > lot of pointer capturing at the same time. Each pointer is captured to at most one element, so the size of the map is bounded by the number of active pointers, right? When I first reviewed this CL I took a quick look to see if we had a list-backed Map API but didn't find one. The map-like API is probably more convenient to use, so we should only consider switching to list-based if we see evidence of a perf issue that would be helped (seems unlikely to me). I.e. don't worry about premature optimization. > Maybe in a separate CL as you > suggested after getting some usage count and how people use the capturing APIs. > But this particular hashmap here does not affect the ordering in which the > events are fired. Do you agree? Right, as far as I saw the iteration order wouldn't be observable in any way (and I agree that's an important property).
Thanks, the code looks good, but please add a test... https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:404: if (hasPendingPointerCaptureTarget On 2016/02/29 20:10:42, Navid Zolghadr wrote: > On 2016/02/29 19:55:40, mustaq wrote: > > Your ordering is different from the spec algorithm. It is because of > > pointerleave vs pointerenter relative ordering? I thought leave vs enter > firing > > happens on disjoint cases, so the inverted ordering here is not needed. > > Moreover, we would want to maintain the got-vs-lost pointercapture event > order. > > It is because of the sendNodeTransitionEvents function. That function cares > about the m_pointerCaptureTarget. As a result I need to fire the leave/out > events before setting anything to that map and fire the enter/over events after > remove the target from the map. At the end as you said the result is the same as > the spec because they don't happen together. Is this fine? - We need to add a test that covers the independence of the ordering here, i.e., the firing of out+leave vs firing of over+enter at hittest node. I guess just capturing a specific id to node1, then capturing the same id to node2 should be enough, right? - Do you think adding firing of got/lostpointercapture here would be easy? If not, defer the change for a later CL. I understand that with the correct (spec-like) ordering, this would be trivial when you need to fire both got & lost here.
https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:404: if (hasPendingPointerCaptureTarget On 2016/03/04 15:16:40, mustaq wrote: > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > On 2016/02/29 19:55:40, mustaq wrote: > > > Your ordering is different from the spec algorithm. It is because of > > > pointerleave vs pointerenter relative ordering? I thought leave vs enter > > firing > > > happens on disjoint cases, so the inverted ordering here is not needed. > > > Moreover, we would want to maintain the got-vs-lost pointercapture event > > order. > > > > It is because of the sendNodeTransitionEvents function. That function cares > > about the m_pointerCaptureTarget. As a result I need to fire the leave/out > > events before setting anything to that map and fire the enter/over events > after > > remove the target from the map. At the end as you said the result is the same > as > > the spec because they don't happen together. Is this fine? > > - We need to add a test that covers the independence of the ordering here, i.e., > the firing of out+leave vs firing of over+enter at hittest node. I guess just > capturing a specific id to node1, then capturing the same id to node2 should be > enough, right? > > - Do you think adding firing of got/lostpointercapture here would be easy? If > not, defer the change for a later CL. I understand that with the correct > (spec-like) ordering, this would be trivial when you need to fire both got & > lost here. I'm not quite sure what you want to test here. Do you want me to add got/lost as well to test along with this change or something else? In the scenario you explained, where do you want the mouse pointer to be? On the node 1 or 2? Note that what we have right now in the test for the "explicit release" scenario does test the over+enter on blue. Right? Regarding firing got/lostpointercapture, I cannot use a nested if else for those like spec structure. I don't see a lot of value of the nested checks in the spec either tough. They only share one condition and even the second condition in the first line of each step is a subset of the condition in the further line.
On 2016/03/04 15:46:02, Navid Zolghadr wrote: > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:404: if > (hasPendingPointerCaptureTarget > On 2016/03/04 15:16:40, mustaq wrote: > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > Your ordering is different from the spec algorithm. It is because of > > > > pointerleave vs pointerenter relative ordering? I thought leave vs enter > > > firing > > > > happens on disjoint cases, so the inverted ordering here is not needed. > > > > Moreover, we would want to maintain the got-vs-lost pointercapture event > > > order. > > > > > > It is because of the sendNodeTransitionEvents function. That function cares > > > about the m_pointerCaptureTarget. As a result I need to fire the leave/out > > > events before setting anything to that map and fire the enter/over events > > after > > > remove the target from the map. At the end as you said the result is the > same > > as > > > the spec because they don't happen together. Is this fine? > > > > - We need to add a test that covers the independence of the ordering here, > i.e., > > the firing of out+leave vs firing of over+enter at hittest node. I guess just > > capturing a specific id to node1, then capturing the same id to node2 should > be > > enough, right? > > > > - Do you think adding firing of got/lostpointercapture here would be easy? If > > not, defer the change for a later CL. I understand that with the correct > > (spec-like) ordering, this would be trivial when you need to fire both got & > > lost here. > > I'm not quite sure what you want to test here. Do you want me to add got/lost as > well to test along with this change or something else? In the scenario you > explained, where do you want the mouse pointer to be? On the node 1 or 2? Note > that what we have right now in the test for the "explicit release" scenario does > test the over+enter on blue. Right? I wanted to test calls to set/releasePointerCapture at a node != hitTestNode. Looks like we need more than one test to cover transition events in such cases: - While pointer on blue, capture at green: blue should receive out+leave. - While pointer at green, capture at green, then capture at blue, then capture at green again: no event should fire because of lazy handling of pending capture. - Similar cases with release. Perhaps it would be better to add a separate test html for this with only 2 divs and minimal event logging. > Regarding firing got/lostpointercapture, I cannot use a nested if else for those > like spec structure. I don't see a lot of value of the nested checks in the spec > either tough. They only share one condition and even the second condition in the > first line of each step is a subset of the condition in the further line. Again, let's defer this, but the nested checks define this relative ordering which would need some attention: a. lostpoitercapture at old capturing node b. over/enter at hittest node c. gotpointercapture at new capturing node d. out/leave at hittest node. In current CL, we are covering only b & d, which are disjoint, so we are fine with d before b. But when we would add a & c, it won't be the case anymore. Did I get it right?
On 2016/03/04 16:25:33, mustaq wrote: > On 2016/03/04 15:46:02, Navid Zolghadr wrote: > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/input/PointerEventManager.cpp:404: if > > (hasPendingPointerCaptureTarget > > On 2016/03/04 15:16:40, mustaq wrote: > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > Your ordering is different from the spec algorithm. It is because of > > > > > pointerleave vs pointerenter relative ordering? I thought leave vs enter > > > > firing > > > > > happens on disjoint cases, so the inverted ordering here is not needed. > > > > > Moreover, we would want to maintain the got-vs-lost pointercapture event > > > > order. > > > > > > > > It is because of the sendNodeTransitionEvents function. That function > cares > > > > about the m_pointerCaptureTarget. As a result I need to fire the leave/out > > > > events before setting anything to that map and fire the enter/over events > > > after > > > > remove the target from the map. At the end as you said the result is the > > same > > > as > > > > the spec because they don't happen together. Is this fine? > > > > > > - We need to add a test that covers the independence of the ordering here, > > i.e., > > > the firing of out+leave vs firing of over+enter at hittest node. I guess > just > > > capturing a specific id to node1, then capturing the same id to node2 should > > be > > > enough, right? > > > > > > - Do you think adding firing of got/lostpointercapture here would be easy? > If > > > not, defer the change for a later CL. I understand that with the correct > > > (spec-like) ordering, this would be trivial when you need to fire both got & > > > lost here. > > > > I'm not quite sure what you want to test here. Do you want me to add got/lost > as > > well to test along with this change or something else? In the scenario you > > explained, where do you want the mouse pointer to be? On the node 1 or 2? Note > > that what we have right now in the test for the "explicit release" scenario > does > > test the over+enter on blue. Right? > > I wanted to test calls to set/releasePointerCapture at a node != hitTestNode. > Looks like we need more than one test to cover transition events in such cases: > - While pointer on blue, capture at green: blue should receive out+leave. > - While pointer at green, capture at green, then capture at blue, then capture > at green again: no event should fire because of lazy handling of pending > capture. > - Similar cases with release. > > Perhaps it would be better to add a separate test html for this with only 2 divs > and minimal event logging. > > > Regarding firing got/lostpointercapture, I cannot use a nested if else for > those > > like spec structure. I don't see a lot of value of the nested checks in the > spec > > either tough. They only share one condition and even the second condition in > the > > first line of each step is a subset of the condition in the further line. > > Again, let's defer this, but the nested checks define this relative ordering > which > would need some attention: > a. lostpoitercapture at old capturing node > b. over/enter at hittest node > c. gotpointercapture at new capturing node > d. out/leave at hittest node. > > In current CL, we are covering only b & d, which are disjoint, so we are fine > with > d before b. But when we would add a & c, it won't be the case anymore. Did I get > it right? I just realized I cannot add any more correct scenarios. Particularly, the block of the code that sends out/leave is never reached for mouse because I don't set node under mouse and I did add a to-do for sendmouseevents in this function and referred to the bug but I had forgotten about it :). The only portion that is right now fully working is sending over/enter and it is being tested in the existing test on the blue div.
On 2016/03/04 20:33:11, Navid Zolghadr wrote: > On 2016/03/04 16:25:33, mustaq wrote: > > On 2016/03/04 15:46:02, Navid Zolghadr wrote: > > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > > > > > > > > https://codereview.chromium.org/1635863006/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/input/PointerEventManager.cpp:404: if > > > (hasPendingPointerCaptureTarget > > > On 2016/03/04 15:16:40, mustaq wrote: > > > > On 2016/02/29 20:10:42, Navid Zolghadr wrote: > > > > > On 2016/02/29 19:55:40, mustaq wrote: > > > > > > Your ordering is different from the spec algorithm. It is because of > > > > > > pointerleave vs pointerenter relative ordering? I thought leave vs > enter > > > > > firing > > > > > > happens on disjoint cases, so the inverted ordering here is not > needed. > > > > > > Moreover, we would want to maintain the got-vs-lost pointercapture > event > > > > > order. > > > > > > > > > > It is because of the sendNodeTransitionEvents function. That function > > cares > > > > > about the m_pointerCaptureTarget. As a result I need to fire the > leave/out > > > > > events before setting anything to that map and fire the enter/over > events > > > > after > > > > > remove the target from the map. At the end as you said the result is the > > > same > > > > as > > > > > the spec because they don't happen together. Is this fine? > > > > > > > > - We need to add a test that covers the independence of the ordering here, > > > i.e., > > > > the firing of out+leave vs firing of over+enter at hittest node. I guess > > just > > > > capturing a specific id to node1, then capturing the same id to node2 > should > > > be > > > > enough, right? > > > > > > > > - Do you think adding firing of got/lostpointercapture here would be easy? > > If > > > > not, defer the change for a later CL. I understand that with the correct > > > > (spec-like) ordering, this would be trivial when you need to fire both got > & > > > > lost here. > > > > > > I'm not quite sure what you want to test here. Do you want me to add > got/lost > > as > > > well to test along with this change or something else? In the scenario you > > > explained, where do you want the mouse pointer to be? On the node 1 or 2? > Note > > > that what we have right now in the test for the "explicit release" scenario > > does > > > test the over+enter on blue. Right? > > > > I wanted to test calls to set/releasePointerCapture at a node != hitTestNode. > > Looks like we need more than one test to cover transition events in such > cases: > > - While pointer on blue, capture at green: blue should receive out+leave. > > - While pointer at green, capture at green, then capture at blue, then capture > > at green again: no event should fire because of lazy handling of pending > > capture. > > - Similar cases with release. > > > > Perhaps it would be better to add a separate test html for this with only 2 > divs > > and minimal event logging. > > > > > Regarding firing got/lostpointercapture, I cannot use a nested if else for > > those > > > like spec structure. I don't see a lot of value of the nested checks in the > > spec > > > either tough. They only share one condition and even the second condition in > > the > > > first line of each step is a subset of the condition in the further line. > > > > Again, let's defer this, but the nested checks define this relative ordering > > which > > would need some attention: > > a. lostpoitercapture at old capturing node > > b. over/enter at hittest node > > c. gotpointercapture at new capturing node > > d. out/leave at hittest node. > > > > In current CL, we are covering only b & d, which are disjoint, so we are fine > > with > > d before b. But when we would add a & c, it won't be the case anymore. Did I > get > > it right? > > I just realized I cannot add any more correct scenarios. Particularly, the block > of the code that sends out/leave is never reached for mouse because I don't set > node under mouse and I did add a to-do for sendmouseevents in this function and > referred to the bug but I had forgotten about it :). > The only portion that is right now fully working is sending over/enter and it is > being tested in the existing test on the blue div. After a quick chat, I am now convinced that mouse transition event needs to be separated out first to test the cases I mentioned. This CL is making good incremental progress, the core problem with transition events is separate from the capture API. So lgtm. Please update the CL description to clarify the progress: It's the first one in a series of planned changes to implement capture. Adds basic capturing capabilities for mouse... Will cover got/lost firing in upcoming CL... Transition events are blocking on crbug/nnnn.
Description was changed from ========== Pointerevent capture APIs Adding setPointerCapture and releasePointerCapture APIs BUG=580169 ========== to ========== Pointerevent capture APIs Adding setPointerCapture and releasePointerCapture APIs It's the first one in a series of planned changes to implement capture. Adds basic capturing capabilities for mouse using the set and release APIs. We will cover got/lost firing in crbug/592280. Transition events for mouse is blocking on crbug/587955. BUG=580169 ==========
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1635863006/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1635863006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1635863006/140001
Message was sent while issue was closed.
Description was changed from ========== Pointerevent capture APIs Adding setPointerCapture and releasePointerCapture APIs It's the first one in a series of planned changes to implement capture. Adds basic capturing capabilities for mouse using the set and release APIs. We will cover got/lost firing in crbug/592280. Transition events for mouse is blocking on crbug/587955. BUG=580169 ========== to ========== Pointerevent capture APIs Adding setPointerCapture and releasePointerCapture APIs It's the first one in a series of planned changes to implement capture. Adds basic capturing capabilities for mouse using the set and release APIs. We will cover got/lost firing in crbug/592280. Transition events for mouse is blocking on crbug/587955. BUG=580169 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Pointerevent capture APIs Adding setPointerCapture and releasePointerCapture APIs It's the first one in a series of planned changes to implement capture. Adds basic capturing capabilities for mouse using the set and release APIs. We will cover got/lost firing in crbug/592280. Transition events for mouse is blocking on crbug/587955. BUG=580169 ========== to ========== Pointerevent capture APIs Adding setPointerCapture and releasePointerCapture APIs It's the first one in a series of planned changes to implement capture. Adds basic capturing capabilities for mouse using the set and release APIs. We will cover got/lost firing in crbug/592280. Transition events for mouse is blocking on crbug/587955. BUG=580169 Committed: https://crrev.com/ded5afeb2fc486424ed5c5b2b135edbd708d4a52 Cr-Commit-Position: refs/heads/master@{#379558} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ded5afeb2fc486424ed5c5b2b135edbd708d4a52 Cr-Commit-Position: refs/heads/master@{#379558} |
