Make light dismiss use click instead of pointerdown/up [chromium/src : main]
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
+masonf for core/html and tests
+mustaq for everything else
Related details
- Mason Freed
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 6 comments![]( "Open in Gerrit")
if (FlatTreeTraversal::IsDescendantOf(*event.target()->ToNode(),I think you likely want `IsInclusiveDescendantOf` which doesn't yet exist. But it's possible to click on the popover itself, right? (This case likely needs a test also.)
select_->GetDocument().SetPopoverPointerdownTarget(popover_);Can you add a CHECK inside SetPopoverPointerdownTarget() that `!RuntimeEnabledFeatures::LightDismissFromClickEnabled()`?
static void HandleDialogLightDismissForClick(const Node& pointer_down_target,Can you add comments explaining the two versions?
if (auto* pseudo = DynamicTo<PseudoElement>(target)) {Is this a spec change? I think the effect should be identical, and I can't think of a way that it might be observable, but maybe I'm missing one.
Current spec includes the bounds check:
static void HandlePopoverLightDismissForClick(const Node& pointer_down_target,ditto a comment would be nice
Well, I guess at least this is observable. Should you change the test?
Related details
- Joey Arhar
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 3 comments![]( "Open in Gerrit")
Thanks for the long wait for my comments here. The overall plan here looks good. I _think_ the PEF logging of down/up targets could bypass the need for extra parameters in some methods here, apologies if I am missing something here!
Node* target_node = nullptr,Did you mean to call it `possibly_pseudo_target` or something?
I am still scratching my head to understand why event dispatch would worry about a pseudo-target. What about a quick sync?
if (!pointer_down_target) {Ideally `MEM::DispatchMouseEvent` callers won't worry about the two extra parameters but would pull the data from PEF. What do you think?
I believe the whole `if` blocking staring at L235 belongs to PEM where we already have access to PEF. Does it sound like a separate cleanup that could precede this CL to simplify things here? We switched click-like events from MEs to PEs but didn't get a chance to simplify the code after the flag got removed!
Related details
- Joey Arhar
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 4 comments![]( "Open in Gerrit")
if (pointer_id_to_targets_.Contains(pointer_id)) {Use `pointer_id_to_targets_.insert(MakeGarbageCollected<PointerTargets>())` to avoid double lookup. Then you can use the returned pair's value, similar to [CopyToActiveInterpolationsMap](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/effect_stack.cc;l=48?q=%5C.insert%5C(.*,%5C%20MakeGarbageCollected).
// prevent light dismiss from closing the picker.I'm a bit confused by this. Is the intention to ignore events within inner elements? If so, can we do this by only acting on events whose event phase is kAtTarget to ignore clicks bubbling up from inner elements?
Node* target_node = nullptr,Did you mean to call it `possibly_pseudo_target` or something?
I am still scratching my head to understand why event dispatch would worry about a pseudo-target. What about a quick sync?
This also seems problematic for standardizing. If we have a standardized way of accessing the down and up targets they would be the same targets that are observable on the events.
mev.GetHitTestResult().InnerPossiblyPseudoNode());This doesn't take into account pointer capture, however I think that we should.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
Node* target_node = nullptr,Did you mean to call it `possibly_pseudo_target` or something?
I am still scratching my head to understand why event dispatch would worry about a pseudo-target. What about a quick sync?
Dialog light dismiss needs to worry about pseudo-elements because of the ::backdrop pseudo-element. When clicking on ::backdrop we should trigger light dismiss, but if we click on its corresponding dialog element then we should not light dismiss. If we just passed the corresponding element of a pseudo-element, then ::backdrop would always look like the dialog element itself. We currently have a hacky workaround for this, which this patch can effectively get rid of: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_dialog_element.cc;l=273-283;drc=a3c9c4dcd0d6de500f41d7272d9515b6b81b7729
if (!pointer_down_target) {Ideally `MEM::DispatchMouseEvent` callers won't worry about the two extra parameters but would pull the data from PEF. What do you think?
I believe the whole `if` blocking staring at L235 belongs to PEM where we already have access to PEF. Does it sound like a separate cleanup that could precede this CL to simplify things here? We switched click-like events from MEs to PEs but didn't get a chance to simplify the code after the flag got removed!
So we can move this code to PointerEventManager? Can we move the entire DispatchMouseEvent method there?
Related details
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 9 comments![]( "Open in Gerrit")
if (pointer_id_to_targets_.Contains(pointer_id)) {Use `pointer_id_to_targets_.insert(MakeGarbageCollected<PointerTargets>())` to avoid double lookup. Then you can use the returned pair's value, similar to [CopyToActiveInterpolationsMap](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/effect_stack.cc;l=48?q=%5C.insert%5C(.*,%5C%20MakeGarbageCollected).
Done
// prevent light dismiss from closing the picker.I'm a bit confused by this. Is the intention to ignore events within inner elements? If so, can we do this by only acting on events whose event phase is kAtTarget to ignore clicks bubbling up from inner elements?
Events can bubble up from the author provided `<button>` or the author provided `<option>`/etc elements which are slotted into the picker, so I don't think we can figure this out without looking at the event path.
if (FlatTreeTraversal::IsDescendantOf(*event.target()->ToNode(),I think you likely want `IsInclusiveDescendantOf` which doesn't yet exist. But it's possible to click on the popover itself, right? (This case likely needs a test also.)
I tried for a while to write a test, but testdriver is refusing to click on the area below the select's button while the picker is open. I included it as select-click-picker-light-dismiss.tentative.html
I added a check to see if the event target is the popover.
select_->GetDocument().SetPopoverPointerdownTarget(popover_);Can you add a CHECK inside SetPopoverPointerdownTarget() that `!RuntimeEnabledFeatures::LightDismissFromClickEnabled()`?
Done
static void HandleDialogLightDismissForClick(const Node& pointer_down_target,Can you add comments explaining the two versions?
Done
if (auto* pseudo = DynamicTo<PseudoElement>(target)) {Is this a spec change? I think the effect should be identical, and I can't think of a way that it might be observable, but maybe I'm missing one.
Current spec includes the bounds check:
Yeah I will certainly have to rewrite this portion of the spec, at the very least to remove the existing x and y coordinate checks to figure out if the ::backdrop was clicked. Does this answer your question?
static void HandlePopoverLightDismissForClick(const Node& pointer_down_target,ditto a comment would be nice
Done
mev.GetHitTestResult().InnerPossiblyPseudoNode());This doesn't take into account pointer capture, however I think that we should.
What is pointer capture?
Well, I guess at least this is observable. Should you change the test?
I'm not sure if we should change the test before changing the spec
Related details
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Mason Freed (Gerrit)
Mason Freed added 6 comments![]( "Open in Gerrit")
if (FlatTreeTraversal::IsDescendantOf(*event.target()->ToNode(),Joey ArharI think you likely want `IsInclusiveDescendantOf` which doesn't yet exist. But it's possible to click on the popover itself, right? (This case likely needs a test also.)
I tried for a while to write a test, but testdriver is refusing to click on the area below the select's button while the picker is open. I included it as select-click-picker-light-dismiss.tentative.html
I added a check to see if the event target is the popover.
Acknowledged
Joey ArharIs this a spec change? I think the effect should be identical, and I can't think of a way that it might be observable, but maybe I'm missing one.
Current spec includes the bounds check:
Yeah I will certainly have to rewrite this portion of the spec, at the very least to remove the existing x and y coordinate checks to figure out if the ::backdrop was clicked. Does this answer your question?
Yes, thanks.
const option = document.querySelector('option');unused (and `undefined`)
// We can't get layout information of ::picker(select), so this is a guess ofFor the test, you can always position and size it however you want, which feels like the right thing to do here.
bodyClicked = true;The click event bubbles, so I'm not sure this is testing what you think it's testing...
Joey ArharWell, I guess at least this is observable. Should you change the test?
I'm not sure if we should change the test before changing the spec
Ok. But perhaps it's a good idea to copy/paste this to a .tentative test and modify it, so you have some proof that it does what you expect, and you can rename/delete when the spec PR lands?
Related details
- Joey Arhar
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
bodyClicked = true;The click event bubbles, so I'm not sure this is testing what you think it's testing...
Since this assert is failing, it proves to me that test driver isn't clicking anything at all when i ask it to click an x and y where the picker is. I'm not sure how to make this test work.
Related details
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
bodyClicked = true;Joey ArharThe click event bubbles, so I'm not sure this is testing what you think it's testing...
Since this assert is failing, it proves to me that test driver isn't clicking anything at all when i ask it to click an x and y where the picker is. I'm not sure how to make this test work.
Weird. Maybe add another couple `await rAF()`s after the click?
This would likely help a little, if nothing else, for readability:
```
::picker(select) {
width: 600px;
height: 600px;
top:0;
left:0;
}
```
and then just
```
.pointerMove(300, 300)
```
You might (?) also need to add
```
position-area: none;
position-try-fallbacks: none;
```
Related details
- Joey Arhar
- Mustaq Ahmed
- Robert Flack
Joey Arhar (Gerrit)
Joey Arhar added 7 comments![]( "Open in Gerrit")
Joey ArharDid you mean to call it `possibly_pseudo_target` or something?
I am still scratching my head to understand why event dispatch would worry about a pseudo-target. What about a quick sync?
Dialog light dismiss needs to worry about pseudo-elements because of the ::backdrop pseudo-element. When clicking on ::backdrop we should trigger light dismiss, but if we click on its corresponding dialog element then we should not light dismiss. If we just passed the corresponding element of a pseudo-element, then ::backdrop would always look like the dialog element itself. We currently have a hacky workaround for this, which this patch can effectively get rid of: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_dialog_element.cc;l=273-283;drc=a3c9c4dcd0d6de500f41d7272d9515b6b81b7729
I re-added the ::backdrop logic since it sounds like we can't use pseudo-elements in the spec.
mev.GetHitTestResult().InnerPossiblyPseudoNode());Joey ArharThis doesn't take into account pointer capture, however I think that we should.
What is pointer capture?
I removed this parameter. Does that address your question?
if (!pointer_down_target) {Joey ArharIdeally `MEM::DispatchMouseEvent` callers won't worry about the two extra parameters but would pull the data from PEF. What do you think?
I believe the whole `if` blocking staring at L235 belongs to PEM where we already have access to PEF. Does it sound like a separate cleanup that could precede this CL to simplify things here? We switched click-like events from MEs to PEs but didn't get a chance to simplify the code after the flag got removed!
So we can move this code to PointerEventManager? Can we move the entire DispatchMouseEvent method there?
As per offline discussion, I'll plan on landing this as-is and we can try improving afterwards.
const option = document.querySelector('option');Joey Arharunused (and `undefined`)
Done
// We can't get layout information of ::picker(select), so this is a guess ofFor the test, you can always position and size it however you want, which feels like the right thing to do here.
Done
Joey ArharThe click event bubbles, so I'm not sure this is testing what you think it's testing...
Mason FreedSince this assert is failing, it proves to me that test driver isn't clicking anything at all when i ask it to click an x and y where the picker is. I'm not sure how to make this test work.
Weird. Maybe add another couple `await rAF()`s after the click?
This would likely help a little, if nothing else, for readability:
```
::picker(select) {
width: 600px;
height: 600px;
top:0;
left:0;
}
```and then just
```
.pointerMove(300, 300)
```
You might (?) also need to add```
position-area: none;
position-try-fallbacks: none;
```
hey that fixed it, thanks!!!
Joey ArharWell, I guess at least this is observable. Should you change the test?
Mason FreedI'm not sure if we should change the test before changing the spec
Ok. But perhaps it's a good idea to copy/paste this to a .tentative test and modify it, so you have some proof that it does what you expect, and you can rename/delete when the spec PR lands?
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
// prevent light dismiss from closing the picker.Joey ArharI'm a bit confused by this. Is the intention to ignore events within inner elements? If so, can we do this by only acting on events whose event phase is kAtTarget to ignore clicks bubbling up from inner elements?
Events can bubble up from the author provided `<button>` or the author provided `<option>`/etc elements which are slotted into the picker, so I don't think we can figure this out without looking at the event path.
Hmm, it seems like you're only interested in handling events targeting the popover. It seems like you wouldn't need these ancestor checks if the event handling happened in PopoverElementForAppearanceBase::DefaultEventHandler since then you would only be seeing events targeting the popover or some descendant of it.
Related details
- Joey Arhar
- Mason Freed
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
// prevent light dismiss from closing the picker.Joey ArharI'm a bit confused by this. Is the intention to ignore events within inner elements? If so, can we do this by only acting on events whose event phase is kAtTarget to ignore clicks bubbling up from inner elements?
Robert FlackEvents can bubble up from the author provided `<button>` or the author provided `<option>`/etc elements which are slotted into the picker, so I don't think we can figure this out without looking at the event path.
Hmm, it seems like you're only interested in handling events targeting the popover. It seems like you wouldn't need these ancestor checks if the event handling happened in PopoverElementForAppearanceBase::DefaultEventHandler since then you would only be seeing events targeting the popover or some descendant of it.
I think the problem is that we want to handle clicks on the <select> button or its descendants, but **not** descendants of the ::picker element popover (which is also a descendant of the <select>. So it's part of the tree, not just the popover and not just the select. Right jarhar@?
Related details
- Joey Arhar
- Mustaq Ahmed
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
// prevent light dismiss from closing the picker.Joey ArharI'm a bit confused by this. Is the intention to ignore events within inner elements? If so, can we do this by only acting on events whose event phase is kAtTarget to ignore clicks bubbling up from inner elements?
Robert FlackEvents can bubble up from the author provided `<button>` or the author provided `<option>`/etc elements which are slotted into the picker, so I don't think we can figure this out without looking at the event path.
Mason FreedHmm, it seems like you're only interested in handling events targeting the popover. It seems like you wouldn't need these ancestor checks if the event handling happened in PopoverElementForAppearanceBase::DefaultEventHandler since then you would only be seeing events targeting the popover or some descendant of it.
I think the problem is that we want to handle clicks on the <select> button or its descendants, but **not** descendants of the ::picker element popover (which is also a descendant of the <select>. So it's part of the tree, not just the popover and not just the select. Right jarhar@?
Ah right, it's the inversion, it doesn't want to handle events whose target includes the popover, but it does want to handle events in other descendants.
It seems like architecturally it would be cleaner to have the popover preventDefault or stopPropagation the event, but that would probably be too heavy handed since it would also prevent other default behaviors.
Robert Flack (Gerrit)
Robert Flack added 3 comments![]( "Open in Gerrit")
PointerAttributes attributes = pointer_id_to_attributes_.Take(mapped_id);It really feels like we should add the down and up nodes to the PointerAttributes data rather than create a second map.
pointer_id_to_targets_.erase(mapped_id);For touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
pointer_event, target, &mouse_event);You should probably be using the effective_target here so that if pointer capture is used we use the captured target and have the same target as the actual down and up events are dispatched to.
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 3 comments![]( "Open in Gerrit")
Node* GetPointerTarget(WebInputEvent::Type type, PointerId pointer_id) const;I think two separate functions are better because this is only for two types: `GetPointerDownTarget` and `GetPointerUpTarget`.
if (!pointer_down_target) {Joey ArharIdeally `MEM::DispatchMouseEvent` callers won't worry about the two extra parameters but would pull the data from PEF. What do you think?
I believe the whole `if` blocking staring at L235 belongs to PEM where we already have access to PEF. Does it sound like a separate cleanup that could precede this CL to simplify things here? We switched click-like events from MEs to PEs but didn't get a chance to simplify the code after the flag got removed!
Joey ArharSo we can move this code to PointerEventManager? Can we move the entire DispatchMouseEvent method there?
As per offline discussion, I'll plan on landing this as-is and we can try improving afterwards.
Acknowledged
pointer_event, target, &mouse_event);You should probably be using the effective_target here so that if pointer capture is used we use the captured target and have the same target as the actual down and up events are dispatched to.
The cleanest (and ideal) way to achieve it would be to call `PEF::SetPointerTarget` when dispatching down/up events at `PEM::DispatchPointerEvent`. But pseudo-element make it tricky!
I think the core question is: what is the expected behavior when `pointerdown` goes to the backdrop and `pointerup` goes to the popover element but anytime in-between, the pointer was captured to the popover element? Maybe we could consider pseudo targets only for the default implicit capture with touch but that sounds hacky.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
PointerAttributes attributes = pointer_id_to_attributes_.Take(mapped_id);It really feels like we should add the down and up nodes to the PointerAttributes data rather than create a second map.
I initially tried this but gave up, and I just tried again and I think I got it to work. How does it look?
pointer_id_to_targets_.erase(mapped_id);For touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
I have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
PointerAttributes attributes = pointer_id_to_attributes_.Take(mapped_id);Joey ArharIt really feels like we should add the down and up nodes to the PointerAttributes data rather than create a second map.
I initially tried this but gave up, and I just tried again and I think I got it to work. How does it look?
It looks like I ended up causing a weird memory issue by doing this:
```
# Fatal error in ../../v8/src/heap/cppgc/memory.h, line 33
# Check failed: kZappedValue == reinterpret_cast<ConstAddress>(address)[i] (\xdc vs. '\0').
#
#
#
#FailureMessage Object: 0x7ffcbdbfd780#0 0x59765c472792 base::debug::CollectStackTrace()
#1 0x59765c459d41 base::debug::StackTrace::StackTrace()
#2 0x5976611b660d gin::(anonymous namespace)::PrintStackTrace()
#3 0x59765e5b9b93 V8_Fatal()
#4 0x597655f316de cppgc::internal::CheckMemoryIsInaccessible()
#5 0x597655f43de1 cppgc::internal::HeapVisitor<>::Traverse()
#6 0x597655f43256 cppgc::internal::(anonymous namespace)::MutatorThreadSweeper::Sweep()
#7 0x597655f42dfb cppgc::internal::Sweeper::SweeperImpl::Finish()
#8 0x597655f3d852 cppgc::internal::Sweeper::SweeperImpl::FinishIfRunning()
#9 0x5976549b4ae7 v8::internal::CppHeap::FinishAtomicSweepingIfRunning()
#10 0x597654a8bdc9 v8::internal::Heap::CollectGarbage()::$_0::operator()()
#11 0x597654a8b95f heap::base::Stack::SetMarkerAndCallbackImpl<>()
#12 0x597655f4ae8b PushAllRegistersAndIterateStack
```
I can try figuring out what went wrong, but do you still feel that making PointerAttributes GarbageCollected is a good idea?
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Node* GetPointerTarget(WebInputEvent::Type type, PointerId pointer_id) const;I think two separate functions are better because this is only for two types: `GetPointerDownTarget` and `GetPointerUpTarget`.
- Mustaq Ahmed
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
Yes, this is where we get the attributes for a click for gestures, see PointerEventFactory::GetPointerIdForTouchGesture. If we need to know the down and up nodes for clicks as well we should add it here rather than creating a new structure.
pointer_id_to_targets_.erase(mapped_id);Joey ArharFor touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
I have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Have you tested with a page that has double tap to zoom? In these cases the click comes 300ms after the pointerup.
I do think we should look in the recently removed pointers as well, as this was added specifically for this case - to be able to get data for the click event (like the pointerid) that normally would have been forgotten by the time the click was generated.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 4 comments![]( "Open in Gerrit")
PointerAttributes attributes = pointer_id_to_attributes_.Take(mapped_id);Done
// prevent light dismiss from closing the picker.Joey ArharI'm a bit confused by this. Is the intention to ignore events within inner elements? If so, can we do this by only acting on events whose event phase is kAtTarget to ignore clicks bubbling up from inner elements?
Robert FlackEvents can bubble up from the author provided `<button>` or the author provided `<option>`/etc elements which are slotted into the picker, so I don't think we can figure this out without looking at the event path.
Mason FreedHmm, it seems like you're only interested in handling events targeting the popover. It seems like you wouldn't need these ancestor checks if the event handling happened in PopoverElementForAppearanceBase::DefaultEventHandler since then you would only be seeing events targeting the popover or some descendant of it.
Robert FlackI think the problem is that we want to handle clicks on the <select> button or its descendants, but **not** descendants of the ::picker element popover (which is also a descendant of the <select>. So it's part of the tree, not just the popover and not just the select. Right jarhar@?
Ah right, it's the inversion, it doesn't want to handle events whose target includes the popover, but it does want to handle events in other descendants.
It seems like architecturally it would be cleaner to have the popover preventDefault or stopPropagation the event, but that would probably be too heavy handed since it would also prevent other default behaviors.
I'm guessing this is ok as is
mev.GetHitTestResult().InnerPossiblyPseudoNode());Joey ArharThis doesn't take into account pointer capture, however I think that we should.
Joey ArharWhat is pointer capture?
I removed this parameter. Does that address your question?
I'm guessing it does
pointer_event, target, &mouse_event);Mustaq AhmedYou should probably be using the effective_target here so that if pointer capture is used we use the captured target and have the same target as the actual down and up events are dispatched to.
The cleanest (and ideal) way to achieve it would be to call `PEF::SetPointerTarget` when dispatching down/up events at `PEM::DispatchPointerEvent`. But pseudo-element make it tricky!
I think the core question is: what is the expected behavior when `pointerdown` goes to the backdrop and `pointerup` goes to the popover element but anytime in-between, the pointer was captured to the popover element? Maybe we could consider pseudo targets only for the default implicit capture with touch but that sounds hacky.
I'm no longer using pseudo-elements with this patch. Is this comment still relevant? Should I be passing something else to DispatchMouseClickIfNeeded later in this method?
Related details
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
This looks good I think. Just one question about resetting the pointers.
// The number of entries in the list is kept below the inline capacity of the
// `Deque` for performance reasons---we don't expect delayed requests for an
// inactive pointer after a few latter pointers became inactive.While there's no more inline capacity (intentional?) this is still the limit for the number of entries in the deque. Perhaps a quick comment about why "10"?
pointer_event_factory_->SetPointerDownTarget(pointer_event->pointerId(),Do these need to be cleared somewhere? It feels like they could get left with stale results, which might be misinterpreted later. E.g. touchscreen seems to sometimes fire just a pointerup, without a corresponding pointerdown.
Related details
- Joey Arhar
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Recitation-Check
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 3 comments![]( "Open in Gerrit")
// The number of entries in the list is kept below the inline capacity of the
// `Deque` for performance reasons---we don't expect delayed requests for an
// inactive pointer after a few latter pointers became inactive.While there's no more inline capacity (intentional?) this is still the limit for the number of entries in the deque. Perhaps a quick comment about why "10"?
I moved the int to the cc file and re-added this comment with some more context.
pointer_id_to_targets_.erase(mapped_id);Joey ArharFor touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
Robert FlackI have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Have you tested with a page that has double tap to zoom? In these cases the click comes 300ms after the pointerup.
I do think we should look in the recently removed pointers as well, as this was added specifically for this case - to be able to get data for the click event (like the pointerid) that normally would have been forgotten by the time the click was generated.
How does a page opt into double tap to zoom? Or how do i test this?
Also, if the user is performing an action which zooms the page, I don't see why we would need to trigger light dismiss anyway.
pointer_event_factory_->SetPointerDownTarget(pointer_event->pointerId(),Do these need to be cleared somewhere? It feels like they could get left with stale results, which might be misinterpreted later. E.g. touchscreen seems to sometimes fire just a pointerup, without a corresponding pointerdown.
I tried changing it to a WeakMember, lets see if the tests still pass
Related details
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
pointer_id_to_targets_.erase(mapped_id);Joey ArharFor touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
Robert FlackI have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Joey ArharHave you tested with a page that has double tap to zoom? In these cases the click comes 300ms after the pointerup.
I do think we should look in the recently removed pointers as well, as this was added specifically for this case - to be able to get data for the click event (like the pointerid) that normally would have been forgotten by the time the click was generated.
How does a page opt into double tap to zoom? Or how do i test this?
Also, if the user is performing an action which zooms the page, I don't see why we would need to trigger light dismiss anyway.
The important difference is that when double tap to zoom is possible, we delay the click by 300ms so that if you double-tap within 300ms we don't click on the first tap. Double-tap to zoom is enabled by default on most sites that don't have a viewport meta tag, e.g. https://output.jsbin.com/feculinubo
This means that when the click event is generated is long after the pointerdown/pointerup events.
Related details
- Joey Arhar
- Mason Freed
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
pointer_event_factory_->SetPointerDownTarget(pointer_event->pointerId(),Joey ArharDo these need to be cleared somewhere? It feels like they could get left with stale results, which might be misinterpreted later. E.g. touchscreen seems to sometimes fire just a pointerup, without a corresponding pointerdown.
I tried changing it to a WeakMember, lets see if the tests still pass
I was thinking more about proactively setting the targets back to null when you're done with them. Relying on garbage collection to do that feels like a bug waiting to happen.
Related details
- Joey Arhar
- Mustaq Ahmed
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
pointer_id_to_targets_.erase(mapped_id);Joey ArharFor touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
Robert FlackI have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Joey ArharHave you tested with a page that has double tap to zoom? In these cases the click comes 300ms after the pointerup.
I do think we should look in the recently removed pointers as well, as this was added specifically for this case - to be able to get data for the click event (like the pointerid) that normally would have been forgotten by the time the click was generated.
Robert FlackHow does a page opt into double tap to zoom? Or how do i test this?
Also, if the user is performing an action which zooms the page, I don't see why we would need to trigger light dismiss anyway.
The important difference is that when double tap to zoom is possible, we delay the click by 300ms so that if you double-tap within 300ms we don't click on the first tap. Double-tap to zoom is enabled by default on most sites that don't have a viewport meta tag, e.g. https://output.jsbin.com/feculinubo
This means that when the click event is generated is long after the pointerdown/pointerup events.
Thanks, I manually tested this out with mobile device emulation. I observed that double tap to zoom did zoom, and I could see that clicks were delayed. Light dismiss worked as expected, including with nested popovers, so I think the current patch is good.
pointer_event_factory_->SetPointerDownTarget(pointer_event->pointerId(),Joey ArharDo these need to be cleared somewhere? It feels like they could get left with stale results, which might be misinterpreted later. E.g. touchscreen seems to sometimes fire just a pointerup, without a corresponding pointerdown.
Mason FreedI tried changing it to a WeakMember, lets see if the tests still pass
I was thinking more about proactively setting the targets back to null when you're done with them. Relying on garbage collection to do that feels like a bug waiting to happen.
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
// recently_removed_pointers_ is kept below the inline capacity of the `Deque`Is there a Deque somewhere that still uses this? `recently_removed_pointers_` no longer uses it, anyway...
pointer_event_factory_->SetPointerDownTarget(pointer_event->pointerId(),
nullptr);
pointer_event_factory_->SetPointerUpTarget(pointer_event->pointerId(),
nullptr);Thanks.
Related details
- Joey Arhar
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
// recently_removed_pointers_ is kept below the inline capacity of the `Deque`Is there a Deque somewhere that still uses this? `recently_removed_pointers_` no longer uses it, anyway...
We are using this on recently_removed_pointers_ in order to keep the size below 10. So there is a deque which uses it, depending on how you define using it, right?
Related details
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
// recently_removed_pointers_ is kept below the inline capacity of the `Deque`Joey ArharIs there a Deque somewhere that still uses this? `recently_removed_pointers_` no longer uses it, anyway...
We are using this on recently_removed_pointers_ in order to keep the size below 10. So there is a deque which uses it, depending on how you define using it, right?
I just meant that it says "the inline capacity of the Deque" and the Deque doesn't have any inline capacity any longer:
```
HeapDeque<Member<PointerIdAndAttributes>> recently_removed_pointers_;
```
Related details
- Joey Arhar
- Mustaq Ahmed
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
pointer_id_to_targets_.erase(mapped_id);Joey ArharFor touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
Robert FlackI have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Joey ArharHave you tested with a page that has double tap to zoom? In these cases the click comes 300ms after the pointerup.
I do think we should look in the recently removed pointers as well, as this was added specifically for this case - to be able to get data for the click event (like the pointerid) that normally would have been forgotten by the time the click was generated.
Robert FlackHow does a page opt into double tap to zoom? Or how do i test this?
Also, if the user is performing an action which zooms the page, I don't see why we would need to trigger light dismiss anyway.
Joey ArharThe important difference is that when double tap to zoom is possible, we delay the click by 300ms so that if you double-tap within 300ms we don't click on the first tap. Double-tap to zoom is enabled by default on most sites that don't have a viewport meta tag, e.g. https://output.jsbin.com/feculinubo
This means that when the click event is generated is long after the pointerdown/pointerup events.
Thanks, I manually tested this out with mobile device emulation. I observed that double tap to zoom did zoom, and I could see that clicks were delayed. Light dismiss worked as expected, including with nested popovers, so I think the current patch is good.
I'm sorry, I don't understand how this could work.
See PointerEventFactory::GetPointerIdForTouchGesture for how we search recently_removed_pointers_ to get the pointer id for clicks for delayed taps (called from [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=1395;drc=e7c5ae7ea3380cd3dcb7cde4d784b7bd96fdabee;bpv=1;bpt=1)).
If we have to search the recently_removed_pointers_ to get the pointer id for a delayed tap then I'd expect we'd have to do something similar when finding the last down or up node. Maybe dev tools isn't sufficient to test this code path and it needs to be run on a real device?
Related details
- Joey Arhar
- Mustaq Ahmed
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
pointer_id_to_targets_.erase(mapped_id);Joey ArharFor touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
Robert FlackI have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Joey ArharHave you tested with a page that has double tap to zoom? In these cases the click comes 300ms after the pointerup.
I do think we should look in the recently removed pointers as well, as this was added specifically for this case - to be able to get data for the click event (like the pointerid) that normally would have been forgotten by the time the click was generated.
Robert FlackHow does a page opt into double tap to zoom? Or how do i test this?
Also, if the user is performing an action which zooms the page, I don't see why we would need to trigger light dismiss anyway.
Joey ArharThe important difference is that when double tap to zoom is possible, we delay the click by 300ms so that if you double-tap within 300ms we don't click on the first tap. Double-tap to zoom is enabled by default on most sites that don't have a viewport meta tag, e.g. https://output.jsbin.com/feculinubo
This means that when the click event is generated is long after the pointerdown/pointerup events.
Robert FlackThanks, I manually tested this out with mobile device emulation. I observed that double tap to zoom did zoom, and I could see that clicks were delayed. Light dismiss worked as expected, including with nested popovers, so I think the current patch is good.
I'm sorry, I don't understand how this could work.
See PointerEventFactory::GetPointerIdForTouchGesture for how we search recently_removed_pointers_ to get the pointer id for clicks for delayed taps (called from [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=1395;drc=e7c5ae7ea3380cd3dcb7cde4d784b7bd96fdabee;bpv=1;bpt=1)).
If we have to search the recently_removed_pointers_ to get the pointer id for a delayed tap then I'd expect we'd have to do something similar when finding the last down or up node. Maybe dev tools isn't sufficient to test this code path and it needs to be run on a real device?
I built this patch and ran it on my pixel 7, and I got the same results from testing it out as I did with mobile device emulation - the light dismiss is delayed like the click is, but it works and also works with nested popovers.
Maybe this works because PointerEventFactory::Remove in the latest patchset doesn't have much changes to it anymore, now that I combined the new struct with the existing one?
Related details
- Mustaq Ahmed
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 7 comments![]( "Open in Gerrit")
pointer_id_to_targets_.erase(mapped_id);Joey ArharFor touch we remove the pointer before the click is generated, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=600;drc=6ef235e211f9d0e6f493d65570859fef5e9d54f0;bpv=1;bpt=1. You might need to add this to the data saved in SaveRecentlyRemovedPointer so that when the click is generated we can look this up. That said, touch also has implicit capture so unless the developer explicitly removed the implicit capture the target of the click would also be the target of both the down and up event.
Robert FlackI have been able to successfully get the pointer targets when a click is generated on a touch input, but now that PointerAttributes has all the info with the latest patchset I could look in the recently removed pointers for the down/up targets when retrieving the up/down targets. Should I do that in GetPointerTarget?
Joey ArharHave you tested with a page that has double tap to zoom? In these cases the click comes 300ms after the pointerup.
I do think we should look in the recently removed pointers as well, as this was added specifically for this case - to be able to get data for the click event (like the pointerid) that normally would have been forgotten by the time the click was generated.
Robert FlackHow does a page opt into double tap to zoom? Or how do i test this?
Also, if the user is performing an action which zooms the page, I don't see why we would need to trigger light dismiss anyway.
Joey ArharThe important difference is that when double tap to zoom is possible, we delay the click by 300ms so that if you double-tap within 300ms we don't click on the first tap. Double-tap to zoom is enabled by default on most sites that don't have a viewport meta tag, e.g. https://output.jsbin.com/feculinubo
This means that when the click event is generated is long after the pointerdown/pointerup events.
Robert FlackThanks, I manually tested this out with mobile device emulation. I observed that double tap to zoom did zoom, and I could see that clicks were delayed. Light dismiss worked as expected, including with nested popovers, so I think the current patch is good.
Joey ArharI'm sorry, I don't understand how this could work.
See PointerEventFactory::GetPointerIdForTouchGesture for how we search recently_removed_pointers_ to get the pointer id for clicks for delayed taps (called from [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/pointer_event_manager.cc;l=1395;drc=e7c5ae7ea3380cd3dcb7cde4d784b7bd96fdabee;bpv=1;bpt=1)).
If we have to search the recently_removed_pointers_ to get the pointer id for a delayed tap then I'd expect we'd have to do something similar when finding the last down or up node. Maybe dev tools isn't sufficient to test this code path and it needs to be run on a real device?
I built this patch and ran it on my pixel 7, and I got the same results from testing it out as I did with mobile device emulation - the light dismiss is delayed like the click is, but it works and also works with nested popovers.
Maybe this works because PointerEventFactory::Remove in the latest patchset doesn't have much changes to it anymore, now that I combined the new struct with the existing one?
I figured out why this is working, see my new comment in gesture_manager.cc.
FindNearestDialog(pointer_up_target, client_x, client_y);It seems to me that since we use the final position this doesn't handle the drag out case that it is intended to handle. Testing on https://jsbin.com/ralifet/edit?html,css,js,output it seems that drags that start selecting text and end outside the dialog still close it which I thought was not the intended behavior.
/*pointer_up_target=*/current_hit_test.InnerElement());Ah I see, you're not currently using the remembered pointerdown and pointerup targets for clicks from taps. This is why it didn't matter that you weren't checking recently_removed_pointers_.
This won't necessarily match the down and up events that lead to the click which could have modified the dom or used pointer capture which I assume means it won't match the specified behavior in these cases. Is this okay?
mouse_event_type == event_type_names::kClick && pointer_up_target) {It seems to me that we should do this after calling DispatchEvent and determining that the input_event_result is kNotHandled so that if the developer calls preventDefault it will prevent the dialog from closing, e.g. https://jsbin.com/ralifet/edit?html,css,js,output
// isn't.What's an example of when this happens? Is this behavior broken?
// which might be null in cases where there is no HitTestResult.Do you mean target?
pointer_event, target, &mouse_event);Mustaq AhmedYou should probably be using the effective_target here so that if pointer capture is used we use the captured target and have the same target as the actual down and up events are dispatched to.
Joey ArharThe cleanest (and ideal) way to achieve it would be to call `PEF::SetPointerTarget` when dispatching down/up events at `PEM::DispatchPointerEvent`. But pseudo-element make it tricky!
I think the core question is: what is the expected behavior when `pointerdown` goes to the backdrop and `pointerup` goes to the popover element but anytime in-between, the pointer was captured to the popover element? Maybe we could consider pseudo targets only for the default implicit capture with touch but that sounds hacky.
I'm no longer using pseudo-elements with this patch. Is this comment still relevant? Should I be passing something else to DispatchMouseClickIfNeeded later in this method?
Yes, my comment applies to regular targets. If the developer has called setPointerCapture this line can change the effective target. We should already be taking capture targets into account for click targeting, and as such we should also use captured targets for the pointer down / up nodes.
Related details
- Joey Arhar
- Mustaq Ahmed
Joey Arhar (Gerrit)
Joey Arhar added 6 comments![]( "Open in Gerrit")
FindNearestDialog(pointer_up_target, client_x, client_y);It seems to me that since we use the final position this doesn't handle the drag out case that it is intended to handle. Testing on https://jsbin.com/ralifet/edit?html,css,js,output it seems that drags that start selecting text and end outside the dialog still close it which I thought was not the intended behavior.
You're right, and I don't think we're storing the x and y coordinates of the pointerdown anywhere. I wonder if that would be even harder to spell out in the spec.
This makes me want to go back to using pseudo-elements so we can get rid of the x and y coordinates here.
Do you have any recommendation on which to choose?
/*pointer_up_target=*/current_hit_test.InnerElement());Ah I see, you're not currently using the remembered pointerdown and pointerup targets for clicks from taps. This is why it didn't matter that you weren't checking recently_removed_pointers_.
This won't necessarily match the down and up events that lead to the click which could have modified the dom or used pointer capture which I assume means it won't match the specified behavior in these cases. Is this okay?
I think its fine, I'm having a hard time imagining a scenario where a click and drag with a touch device with pointer capture ends up leading to a gesturetap in a way which actually affects light dismiss.
The reason I wrote this code is because I assumed that a gesturetap event means that it was not a click and drag
mouse_event_type == event_type_names::kClick && pointer_up_target) {It seems to me that we should do this after calling DispatchEvent and determining that the input_event_result is kNotHandled so that if the developer calls preventDefault it will prevent the dialog from closing, e.g. https://jsbin.com/ralifet/edit?html,css,js,output
That would make sense, but we had a whole spec discussion when implementing light dismiss for popovers about how the reviewer believed that we should do events completely separately from the actual light dismiss handling, and cited this: https://dom.spec.whatwg.org/#action-versus-occurrence
You can see in the existing implementation that calling preventDefault on pointerdown doesn't stop the light dismiss from happening.
I think that making preventDefault work makes sense, but I think it would be a separate discussion from this change.
// isn't.What's an example of when this happens? Is this behavior broken?
I'm going to try removing it to find the case again
// which might be null in cases where there is no HitTestResult.Joey ArharDo you mean target?
Removed this comment, thanks
pointer_event, target, &mouse_event);Mustaq AhmedYou should probably be using the effective_target here so that if pointer capture is used we use the captured target and have the same target as the actual down and up events are dispatched to.
Joey ArharThe cleanest (and ideal) way to achieve it would be to call `PEF::SetPointerTarget` when dispatching down/up events at `PEM::DispatchPointerEvent`. But pseudo-element make it tricky!
I think the core question is: what is the expected behavior when `pointerdown` goes to the backdrop and `pointerup` goes to the popover element but anytime in-between, the pointer was captured to the popover element? Maybe we could consider pseudo targets only for the default implicit capture with touch but that sounds hacky.
Robert FlackI'm no longer using pseudo-elements with this patch. Is this comment still relevant? Should I be passing something else to DispatchMouseClickIfNeeded later in this method?
Yes, my comment applies to regular targets. If the developer has called setPointerCapture this line can change the effective target. We should already be taking capture targets into account for click targeting, and as such we should also use captured targets for the pointer down / up nodes.
Ok, I changed this to store effective_target instead of target
Related details
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
// isn't.Joey ArharWhat's an example of when this happens? Is this behavior broken?
I'm going to try removing it to find the case again
I wasn't able to reproduce this issue, so I replaced this with a CHECK that both are non-null
Related details
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 4 comments![]( "Open in Gerrit")
FindNearestDialog(pointer_up_target, client_x, client_y);Joey ArharIt seems to me that since we use the final position this doesn't handle the drag out case that it is intended to handle. Testing on https://jsbin.com/ralifet/edit?html,css,js,output it seems that drags that start selecting text and end outside the dialog still close it which I thought was not the intended behavior.
You're right, and I don't think we're storing the x and y coordinates of the pointerdown anywhere. I wonder if that would be even harder to spell out in the spec.
This makes me want to go back to using pseudo-elements so we can get rid of the x and y coordinates here.
Do you have any recommendation on which to choose?
I think keeping the pseudoelement targets internally is cleaner.
/*pointer_up_target=*/current_hit_test.InnerElement());Joey ArharAh I see, you're not currently using the remembered pointerdown and pointerup targets for clicks from taps. This is why it didn't matter that you weren't checking recently_removed_pointers_.
This won't necessarily match the down and up events that lead to the click which could have modified the dom or used pointer capture which I assume means it won't match the specified behavior in these cases. Is this okay?
I think its fine, I'm having a hard time imagining a scenario where a click and drag with a touch device with pointer capture ends up leading to a gesturetap in a way which actually affects light dismiss.
The reason I wrote this code is because I assumed that a gesturetap event means that it was not a click and drag
Yes, a gesturetap is a stationary down and up event, and yes, I agree that it will probably do what you want. However, from a specification point of view, there are a few situations that can happen where these are not technically correct:
1. A developer calls releasePointerCapture in the pointerdown listener releasing the [implicit pointer capture](https://w3c.github.io/pointerevents/#dfn-implicit-pointer-capture), and modifies the dom such that the up event has a different target.
2. A developer calls [setPointerCapture](https://w3c.github.io/pointerevents/#dom-element-setpointercapture) on pointerdown to capture the events (and clicks) to some other node.
While it's probably going to work fine most of the time, these edge cases are not going to match your specified behavior if you ignore the resolved down and up targets.
mouse_event_type == event_type_names::kClick && pointer_up_target) {Joey ArharIt seems to me that we should do this after calling DispatchEvent and determining that the input_event_result is kNotHandled so that if the developer calls preventDefault it will prevent the dialog from closing, e.g. https://jsbin.com/ralifet/edit?html,css,js,output
That would make sense, but we had a whole spec discussion when implementing light dismiss for popovers about how the reviewer believed that we should do events completely separately from the actual light dismiss handling, and cited this: https://dom.spec.whatwg.org/#action-versus-occurrence
You can see in the existing implementation that calling preventDefault on pointerdown doesn't stop the light dismiss from happening.
I think that making preventDefault work makes sense, but I think it would be a separate discussion from this change.
Acknowledged, treating this as a separate issue seems reasonable.
// isn't.Joey ArharWhat's an example of when this happens? Is this behavior broken?
Joey ArharI'm going to try removing it to find the case again
I wasn't able to reproduce this issue, so I replaced this with a CHECK that both are non-null
I suppose an example of this would be if an earlier event listener removes the pointerdown or pointerup target. We maybe need similar behavior to how we track the still attached ancestor if a node is removed, see MouseEventManager handling here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/mouse_event_manager.cc?q=file:mouse_event_manager%5C.cc%20MouseEventManager::HandleRemoveSubtree&ss=chromium%2Fchromium%2Fsrc
Related details
- Joey Arhar
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
4 comments
Still seems ok to me, modulo flackr@ comments/questions.
Node* target,
double client_x,
double client_y);Any reason not to take in a PointerTarget instead of the three parts?
const Node& pointer_down_target,
double pointer_down_client_x,
double pointer_down_client_y,Perhaps here too?
// pointer events weren't fired for this touch, so we need to fake theCan you elaborate on when/why that happens?
Related details
- Joey Arhar
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
FindNearestDialog(pointer_up_target, client_x, client_y);Joey ArharIt seems to me that since we use the final position this doesn't handle the drag out case that it is intended to handle. Testing on https://jsbin.com/ralifet/edit?html,css,js,output it seems that drags that start selecting text and end outside the dialog still close it which I thought was not the intended behavior.
Robert FlackYou're right, and I don't think we're storing the x and y coordinates of the pointerdown anywhere. I wonder if that would be even harder to spell out in the spec.
This makes me want to go back to using pseudo-elements so we can get rid of the x and y coordinates here.
Do you have any recommendation on which to choose?
I think keeping the pseudoelement targets internally is cleaner.
I tried going back to pseudo-elements briefly, but found that it was harder with the way this patch has gone now. I am now storing the x and y coordinates along with the pointerdown and pointerup target elements, and I added a test for this case.
/*pointer_up_target=*/current_hit_test.InnerElement());Joey ArharAh I see, you're not currently using the remembered pointerdown and pointerup targets for clicks from taps. This is why it didn't matter that you weren't checking recently_removed_pointers_.
This won't necessarily match the down and up events that lead to the click which could have modified the dom or used pointer capture which I assume means it won't match the specified behavior in these cases. Is this okay?
Robert FlackI think its fine, I'm having a hard time imagining a scenario where a click and drag with a touch device with pointer capture ends up leading to a gesturetap in a way which actually affects light dismiss.
The reason I wrote this code is because I assumed that a gesturetap event means that it was not a click and drag
Yes, a gesturetap is a stationary down and up event, and yes, I agree that it will probably do what you want. However, from a specification point of view, there are a few situations that can happen where these are not technically correct:
1. A developer calls releasePointerCapture in the pointerdown listener releasing the [implicit pointer capture](https://w3c.github.io/pointerevents/#dfn-implicit-pointer-capture), and modifies the dom such that the up event has a different target.
2. A developer calls [setPointerCapture](https://w3c.github.io/pointerevents/#dom-element-setpointercapture) on pointerdown to capture the events (and clicks) to some other node.While it's probably going to work fine most of the time, these edge cases are not going to match your specified behavior if you ignore the resolved down and up targets.
Thanks, I added a test for this case and changed things to use the stored targets of pointer events if pointer events were fired.
Related details
- Mustaq Ahmed
- Robert Flack
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 6 comments![]( "Open in Gerrit")
Here are a few comments from me. I tried to make them mostly independent from Rob's to avoid confusions.
My biggest concern here is attempting an XL-sized CL that touches the input event "state machine" that is known to cause compat problems with even subtle changes! I suggested two possible splits below: making PEM etc GCed, and event target tracking with DOM changes. I will be happy to suggest other possibilities if you also see this as a logical way forward here.
HeapDeque<Member<PointerIdAndAttributes>> recently_removed_pointers_;It seems [`HeapVector`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/heap/collection_support/heap_vector.h;drc=5bd71e097f24bdc168e1add9159532f7951bd673;l=175) supports inline capacity, right? Since we are enforcing `kRemovedPointersCapacity` in .cc anyway, please consider adding back the capacity here.
: public GarbageCollected<PointerEventFactory> {Any chance we can split this XL-sized CL into smaller ones? Perhaps "making the PEM/PEF GarbageCollected" is an independent change that could land w/o waiting for any popover discussion here.
// pointer events weren't fired for this touch, so we need to fake theCan you elaborate on when/why that happens?
I would suggest a comment like this: "The browser didn't send any pointer events for this touch (e.g. because there is no event handlers) ...".
// isn't.Joey ArharWhat's an example of when this happens? Is this behavior broken?
Joey ArharI'm going to try removing it to find the case again
Robert FlackI wasn't able to reproduce this issue, so I replaced this with a CHECK that both are non-null
I suppose an example of this would be if an earlier event listener removes the pointerdown or pointerup target. We maybe need similar behavior to how we track the still attached ancestor if a node is removed, see MouseEventManager handling here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/mouse_event_manager.cc?q=file:mouse_event_manager%5C.cc%20MouseEventManager::HandleRemoveSubtree&ss=chromium%2Fchromium%2Fsrc
I would suggest that in a separate CL before/after this one to track the removed down/up target. I fear making PEF aware of DOM changes is another big change that should perhaps go separately from this.
// bugs for touch inputs.Nit: maybe a crbug ref?
Related details
- Joey Arhar
- Robert Flack
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
27->28 looks like a rebase. Not sure what cleared my +1. But here it is again.
Related details
- Joey Arhar
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 6 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Commit-Queue | +1 |
Here are a few comments from me. I tried to make them mostly independent from Rob's to avoid confusions.
My biggest concern here is attempting an XL-sized CL that touches the input event "state machine" that is known to cause compat problems with even subtle changes! I suggested two possible splits below: making PEM etc GCed, and event target tracking with DOM changes. I will be happy to suggest other possibilities if you also see this as a logical way forward here.
Thanks, I split it into 3 patches
HeapDeque<Member<PointerIdAndAttributes>> recently_removed_pointers_;It seems [`HeapVector`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/heap/collection_support/heap_vector.h;drc=5bd71e097f24bdc168e1add9159532f7951bd673;l=175) supports inline capacity, right? Since we are enforcing `kRemovedPointersCapacity` in .cc anyway, please consider adding back the capacity here.
Ok, I changed it to heapvector in the new code review
Any chance we can split this XL-sized CL into smaller ones? Perhaps "making the PEM/PEF GarbageCollected" is an independent change that could land w/o waiting for any popover discussion here.
Sure, here's a separate one to make PEM/PEF GC'd: https://chromium-review.googlesource.com/c/chromium/src/+/6966502
Let me know if you'd like anything else split into separate patches.
// pointer events weren't fired for this touch, so we need to fake theMustaq AhmedCan you elaborate on when/why that happens?
I would suggest a comment like this: "The browser didn't send any pointer events for this touch (e.g. because there is no event handlers) ...".
Done
// isn't.Joey ArharWhat's an example of when this happens? Is this behavior broken?
Joey ArharI'm going to try removing it to find the case again
Robert FlackI wasn't able to reproduce this issue, so I replaced this with a CHECK that both are non-null
Mustaq AhmedI suppose an example of this would be if an earlier event listener removes the pointerdown or pointerup target. We maybe need similar behavior to how we track the still attached ancestor if a node is removed, see MouseEventManager handling here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/input/mouse_event_manager.cc?q=file:mouse_event_manager%5C.cc%20MouseEventManager::HandleRemoveSubtree&ss=chromium%2Fchromium%2Fsrc
I would suggest that in a separate CL before/after this one to track the removed down/up target. I fear making PEF aware of DOM changes is another big change that should perhaps go separately from this.
Ok, I made a third patch to do that: https://chromium-review.googlesource.com/c/chromium/src/+/6966511
I suppose an example of this would be if an earlier event listener removes the pointerdown or pointerup target
I added a test which removes the target during pointerdown, called light-dismiss-remove-target.html. What do you think?
Nit: maybe a crbug ref?
- Mustaq Ahmed
- Robert Flack
Joey Arhar (Gerrit)
Joey Arhar voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Commit-Queue | +1 |
3 comments
Any reason not to take in a PointerTarget instead of the three parts?
Done
// recently_removed_pointers_ is kept below the inline capacity of the `Deque`Joey ArharIs there a Deque somewhere that still uses this? `recently_removed_pointers_` no longer uses it, anyway...
Mason FreedWe are using this on recently_removed_pointers_ in order to keep the size below 10. So there is a deque which uses it, depending on how you define using it, right?
I just meant that it says "the inline capacity of the Deque" and the Deque doesn't have any inline capacity any longer:
```
HeapDeque<Member<PointerIdAndAttributes>> recently_removed_pointers_;
```
I moved this to a different patch, changed it from a HeapDeque to a HeapVector, and re-added the actual inline capacity
const Node& pointer_down_target,
double pointer_down_client_x,
double pointer_down_client_y,Joey ArharPerhaps here too?
Done
Related details
- Joey Arhar
- Mason Freed
- Mustaq Ahmed
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
FindNearestDialog(pointer_up_target, client_x, client_y);Joey ArharIt seems to me that since we use the final position this doesn't handle the drag out case that it is intended to handle. Testing on https://jsbin.com/ralifet/edit?html,css,js,output it seems that drags that start selecting text and end outside the dialog still close it which I thought was not the intended behavior.
Robert FlackYou're right, and I don't think we're storing the x and y coordinates of the pointerdown anywhere. I wonder if that would be even harder to spell out in the spec.
This makes me want to go back to using pseudo-elements so we can get rid of the x and y coordinates here.
Do you have any recommendation on which to choose?
Joey ArharI think keeping the pseudoelement targets internally is cleaner.
I tried going back to pseudo-elements briefly, but found that it was harder with the way this patch has gone now. I am now storing the x and y coordinates along with the pointerdown and pointerup target elements, and I added a test for this case.
What is the complication with pseudo-elements? It seems like a cleaner solution to store pseudo-elements internally but use the originating element where that is web exposed.
If you use the position you can still have cases where the target is not the same, e.g.
1. The DOM or styles could change before you are doing your hit test
2. Taps have their target adjusted towards elements which accept clicks but retain their original coordinates
3. If the pointer was actually captured to the element itself it would also presumably ignore any pseudo-element targets.
Related details
- Joey Arhar
- Mason Freed
- Mustaq Ahmed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |