[ozone/wayland] Update pointer position during dragging [chromium/src : main]
Hidehiko Abe (Gerrit)
Hidehiko Abe added 1 comment![]( "Open in Gerrit")
Related details
- Kramer Ge
- Łukasz Patron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Patron (Gerrit)
Łukasz Patron voted and added 1 comment![]( "Open in Gerrit")
Votes added by Łukasz Patron
| Auto-Submit | +1 |
1 comment
Łukasz PatronCould you add tests?
yeah, just updated existing one since I broke it.
Related details
- Hidehiko Abe
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Patron (Gerrit)
Łukasz Patron voted and added 1 comment![]( "Open in Gerrit")
Votes added by Łukasz Patron
| Auto-Submit | +1 |
1 comment
oh it seems like OnPointerMotionEvent() was already handling cursor position update so I removed previous code.
Related details
- Hidehiko Abe
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kramer Ge
| Commit-Queue | +1 |
3 comments
pointer_delegate_->OnPointerMotionEvent(
location, timestamp, wl::EventDispatchPolicy::kImmediate,
/*is_synthesized=*/true);I think this should be called when `pointer_grabber_for_window_drag_` is not null.
And, IMO it doesn't resolve the underlying issue.
My observation is that `wl_pointer` leaves the surface when drag begins, and we do not send button release to `pointer_delegate_` unless `pointer_grabber_for_window_drag_`. In this case `pointer_grabber_for_window_drag_` is not null for and we artificially generated a button release for some reason, which seems to be the actual bug.
+nickdiego
auto* cursor_position = connection_->wayland_cursor_position();
if (cursor_position) {
CHECK(window_);
// TODO(crbug.com/41494257): Once we enable the input region for
// subsurfaces, we need to update this part since the location will no
// longer be relative to the window.
auto location_in_screen =
gfx::ToRoundedPoint(location) +
window_->GetBoundsInDIP().origin().OffsetFromOrigin();
cursor_position->OnCursorPositionChanged(location_in_screen);
}I don't think this should be removed as it is used as a source for cursor position elsewhere
Related details
- Hidehiko Abe
- Nick Yamane
- Łukasz Patron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Patron (Gerrit)
Łukasz Patron voted and added 2 comments![]( "Open in Gerrit")
Votes added by Łukasz Patron
| Auto-Submit | +1 |
2 comments
pointer_delegate_->OnPointerMotionEvent(
location, timestamp, wl::EventDispatchPolicy::kImmediate,
/*is_synthesized=*/true);I think this should be called when `pointer_grabber_for_window_drag_` is not null.
And, IMO it doesn't resolve the underlying issue.
My observation is that `wl_pointer` leaves the surface when drag begins, and we do not send button release to `pointer_delegate_` unless `pointer_grabber_for_window_drag_`. In this case `pointer_grabber_for_window_drag_` is not null for and we artificially generated a button release for some reason, which seems to be the actual bug.+nickdiego
>we artificially generated a button release for some reason, which seems to be the actual bug.
if you don't send artificial button release, it'll just leave you in text selection mode after the file drop.
auto* cursor_position = connection_->wayland_cursor_position();
if (cursor_position) {
CHECK(window_);
// TODO(crbug.com/41494257): Once we enable the input region for
// subsurfaces, we need to update this part since the location will no
// longer be relative to the window.
auto location_in_screen =
gfx::ToRoundedPoint(location) +
window_->GetBoundsInDIP().origin().OffsetFromOrigin();
cursor_position->OnCursorPositionChanged(location_in_screen);
}I don't think this should be removed as it is used as a source for cursor position elsewhere
I removed it because OnPointerMotionEvent() does the same already:
```
void WaylandEventSource::OnPointerMotionEvent(
const gfx::PointF& location,
base::TimeTicks timestamp,
wl::EventDispatchPolicy dispatch_policy,
bool is_synthesized) {
pointer_location_ = location;
int flags = pointer_flags_ | keyboard_modifiers_ | tablet_tool_buttons_;
if (is_synthesized) {
flags |= EF_IS_SYNTHESIZED;
}
MouseEvent event(EventType::kMouseMoved, pointer_location_, pointer_location_,
timestamp, flags, 0);
auto* target = window_manager_->GetCurrentPointerFocusedWindow();
// A window may be deleted when the event arrived from the server.
if (!target) {
return;
}
if (dispatch_policy == wl::EventDispatchPolicy::kImmediate) {
SetTargetAndDispatchEvent(&event, target);
} else {
pointer_frames_.push_back(
std::make_unique<FrameData>(event, base::NullCallback()));
}
}
```and tests still passed with it removed, which proved my assumption.
Related details
- Hidehiko Abe
- Kramer Ge
- Nick Yamane
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kramer Ge
| Commit-Queue | +1 |
1 comment
pointer_delegate_->OnPointerMotionEvent(
location, timestamp, wl::EventDispatchPolicy::kImmediate,
/*is_synthesized=*/true);Łukasz PatronI think this should be called when `pointer_grabber_for_window_drag_` is not null.
And, IMO it doesn't resolve the underlying issue.
My observation is that `wl_pointer` leaves the surface when drag begins, and we do not send button release to `pointer_delegate_` unless `pointer_grabber_for_window_drag_`. In this case `pointer_grabber_for_window_drag_` is not null for and we artificially generated a button release for some reason, which seems to be the actual bug.+nickdiego
>we artificially generated a button release for some reason, which seems to be the actual bug.
if you don't send artificial button release, it'll just leave you in text selection mode after the file drop.
You're right we have to send artificial button release. I was just wondering if the release should be sent targeting the window where pointer isn't in. If drag does not involve data exchange, GNOME always dispatches button release to the same window even if button down and up occurred in 2 different windows. Given that model, location of the event is indeed the problem.
So it's OK fixing it like this, but motion may be dispatched to the origin window if drag enters a different window. So probably skip `OnPointerMotionEvent` when pointer focused window is not equal to `window_`.
Related details
- Hidehiko Abe
- Nick Yamane
- Łukasz Patron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Patron (Gerrit)
Łukasz Patron added 1 comment![]( "Open in Gerrit")
pointer_delegate_->OnPointerMotionEvent(
location, timestamp, wl::EventDispatchPolicy::kImmediate,
/*is_synthesized=*/true);Łukasz PatronI think this should be called when `pointer_grabber_for_window_drag_` is not null.
And, IMO it doesn't resolve the underlying issue.
My observation is that `wl_pointer` leaves the surface when drag begins, and we do not send button release to `pointer_delegate_` unless `pointer_grabber_for_window_drag_`. In this case `pointer_grabber_for_window_drag_` is not null for and we artificially generated a button release for some reason, which seems to be the actual bug.+nickdiego
Kramer Ge>we artificially generated a button release for some reason, which seems to be the actual bug.
if you don't send artificial button release, it'll just leave you in text selection mode after the file drop.
You're right we have to send artificial button release. I was just wondering if the release should be sent targeting the window where pointer isn't in. If drag does not involve data exchange, GNOME always dispatches button release to the same window even if button down and up occurred in 2 different windows. Given that model, location of the event is indeed the problem.
So it's OK fixing it like this, but motion may be dispatched to the origin window if drag enters a different window. So probably skip `OnPointerMotionEvent` when pointer focused window is not equal to `window_`.
hmm, I wrapped the whole thing with `if (pointer_grabber_for_window_drag_)` and now tests fail:
../../ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc:1447: Failure
Expected equality of these values:
gfx::PointF(10, 11)
Which is: 10,11
pointer_delegate->GetPointerLocation()
Which is: 0,0
Related details
- Hidehiko Abe
- Kramer Ge
- Nick Yamane
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Patron (Gerrit)
also it seems like that caused another bug, since it likely last updated the pointer position right before it left the window, so after file drop it believes that I dropped it on the browser window and starts downloading it.
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
Does wrapping in `if (window_ == window_manager_->GetCurrentPointerFocusedWindow())` work?
Related details
- Hidehiko Abe
- Nick Yamane
- Łukasz Patron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Patron (Gerrit)
Łukasz Patron added 1 comment![]( "Open in Gerrit")
tests pass with that, but buggy behavior is still there - https://www.youtube.com/watch?v=nY1hpJbuqas
Related details
- Hidehiko Abe
- Kramer Ge
- Nick Yamane
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kramer Ge
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
pointer_delegate_->OnPointerMotionEvent(
location, timestamp, wl::EventDispatchPolicy::kImmediate,
/*is_synthesized=*/true);Łukasz PatronI think this should be called when `pointer_grabber_for_window_drag_` is not null.
And, IMO it doesn't resolve the underlying issue.
My observation is that `wl_pointer` leaves the surface when drag begins, and we do not send button release to `pointer_delegate_` unless `pointer_grabber_for_window_drag_`. In this case `pointer_grabber_for_window_drag_` is not null for and we artificially generated a button release for some reason, which seems to be the actual bug.+nickdiego
Kramer Ge>we artificially generated a button release for some reason, which seems to be the actual bug.
if you don't send artificial button release, it'll just leave you in text selection mode after the file drop.
Łukasz PatronYou're right we have to send artificial button release. I was just wondering if the release should be sent targeting the window where pointer isn't in. If drag does not involve data exchange, GNOME always dispatches button release to the same window even if button down and up occurred in 2 different windows. Given that model, location of the event is indeed the problem.
So it's OK fixing it like this, but motion may be dispatched to the origin window if drag enters a different window. So probably skip `OnPointerMotionEvent` when pointer focused window is not equal to `window_`.
Łukasz Patronhmm, I wrapped the whole thing with `if (pointer_grabber_for_window_drag_)` and now tests fail:
../../ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc:1447: Failure
Expected equality of these values:
gfx::PointF(10, 11)
Which is: 10,11
pointer_delegate->GetPointerLocation()
Which is: 0,0
Kramer Gealso it seems like that caused another bug, since it likely last updated the pointer position right before it left the window, so after file drop it believes that I dropped it on the browser window and starts downloading it.
Łukasz PatronDoes wrapping in `if (window_ == window_manager_->GetCurrentPointerFocusedWindow())` work?
tests pass with that, but buggy behavior is still there - https://www.youtube.com/watch?v=nY1hpJbuqas
Ah, it's because `window_manager_->GetCurrentPointerFocusedWindow()` is null, so this is fine after all, it's actually just setting `pointer_location_`, not dispatching the event.
Related details
- Hidehiko Abe
- Nick Yamane
- Łukasz Patron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Patron (Gerrit)
Łukasz Patron added 1 comment![]( "Open in Gerrit")
- Hidehiko Abe
- Kramer Ge
- Nick Yamane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
- Hidehiko Abe
- Nick Yamane
- Peter McNeeley
- Łukasz Patron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter McNeeley (Gerrit)
Peter McNeeley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter McNeeley
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
- Hidehiko Abe
- Nick Yamane
- Łukasz Patron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[ozone/wayland] Update pointer position during dragging
Prior to this change, dragging a file from chrome://downloads by its
filename would often open the file after it was dropped somewhere, this
is because the synthetic mouse release has outdated location.
After ensuring that pointer location is updated during drag motion,
similarly to WaylandWindowDragController::OnDragLeave(), this is no
longer a problem.
- M ui/ozone/platform/wayland/host/wayland_data_drag_controller.cc
- M ui/ozone/platform/wayland/host/wayland_data_drag_controller.h
- M ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc
Code-Review: +1 by Kramer Ge, +1 by Peter McNeeley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |