Enable touch drag-and-drop defaults on Linux [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@m...@igalia.com not 100% sure if you are the correct reviewer, "suggest reviewer" sometimes is tough. first of all thanks in advance for your time looking at the code.
right now i have checked/tested on following linux variations:
- X11 with XFCE WM
- GNOME with wayland (mutter)
- Wayland with Hyperland WM (on omarchy-os)
here is a sample video: https://www.youtube.com/watch?v=y5j1Oq66eqQ
and here is a reproducer + guide on how to configure touch on both of the display servers -> https://static.januschka.com/i-451651623/
please let me know if you want me to address anything.
also if you are not the right person to review, i'd super appreciate any hint on who could be a good reviewer.
cheers
Related details
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Max Ihlenfeldt (Gerrit)
Max Ihlenfeldt added 1 comment![]( "Open in Gerrit")
Thanks for this CL! I'll defer to Nick as he's more familiar with the intricacies of DnD.
Related details
- Helmut Januschka
- Nick Yamane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hello @nick...@igalia.com could you help me on this CL? please let me know if you want me to address anything
Related details
- Nick Yamane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
latest PS also fixes Plasma(KDE) on Wayland (and tested on X11)
Related details
- Nick Yamane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Yamane (Gerrit)
Nick Yamane added 2 comments![]( "Open in Gerrit")
}This sounds to me like a compositor-side bug. Have you confirmed with mutter/wlroots folks if that's intended behavior? That said, this code looks too hacky to me. If that's a known bug in Wayland Compositors touch support, I would consider not supporting them here through hacky code paths which increase the maintenance burden.
+fangzhoug +thomasanderson for further feedback on the non-wayland parts.
In theory, there might be several active touch points at once. What is expected behavior, for example, if a new touch down is received when a touch-triggered dnd session is about to start or has just started?
Related details
- Helmut Januschka
- Kramer Ge
- Thomas Anderson
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
This sounds to me like a compositor-side bug. Have you confirmed with mutter/wlroots folks if that's intended behavior? That said, this code looks too hacky to me. If that's a known bug in Wayland Compositors touch support, I would consider not supporting them here through hacky code paths which increase the maintenance burden.
+fangzhoug +thomasanderson for further feedback on the non-wayland parts.
thanks for pushing back on this, this is compositor-side. I spent some time digging into mutter,wlroots and Hyprland to confirm, it was only needed for wayland+hyperland
(that said it requires mutter>=50; current ubuntu stable/lts has this)
removed the hack, since hyprland is pretty niche, as of today, also filed a fix there: https://github.com/hyprwm/Hyprland/pull/15077 as soon as this lands it will work with chromium.
Related details
- Kramer Ge
- Nick Yamane
- Thomas Anderson
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaThis sounds to me like a compositor-side bug. Have you confirmed with mutter/wlroots folks if that's intended behavior? That said, this code looks too hacky to me. If that's a known bug in Wayland Compositors touch support, I would consider not supporting them here through hacky code paths which increase the maintenance burden.
+fangzhoug +thomasanderson for further feedback on the non-wayland parts.
thanks for pushing back on this, this is compositor-side. I spent some time digging into mutter,wlroots and Hyprland to confirm, it was only needed for wayland+hyperland
(that said it requires mutter>=50; current ubuntu stable/lts has this)
removed the hack, since hyprland is pretty niche, as of today, also filed a fix there: https://github.com/hyprwm/Hyprland/pull/15077 as soon as this lands it will work with chromium.
also added sway-wm to the test matrix, and fixed compositor bug there: https://github.com/swaywm/sway/pull/9182
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
Helmut JanuschkaThis sounds to me like a compositor-side bug. Have you confirmed with mutter/wlroots folks if that's intended behavior? That said, this code looks too hacky to me. If that's a known bug in Wayland Compositors touch support, I would consider not supporting them here through hacky code paths which increase the maintenance burden.
+fangzhoug +thomasanderson for further feedback on the non-wayland parts.
Helmut Januschkathanks for pushing back on this, this is compositor-side. I spent some time digging into mutter,wlroots and Hyprland to confirm, it was only needed for wayland+hyperland
(that said it requires mutter>=50; current ubuntu stable/lts has this)
removed the hack, since hyprland is pretty niche, as of today, also filed a fix there: https://github.com/hyprwm/Hyprland/pull/15077 as soon as this lands it will work with chromium.
also added sway-wm to the test matrix, and fixed compositor bug there: https://github.com/swaywm/sway/pull/9182
hyperland - landed already, resolving thread!
In theory, there might be several active touch points at once. What is expected behavior, for example, if a new touch down is received when a touch-triggered dnd session is about to start or has just started?
the multi-touch handling here was indeed undefined (the fallback always used `touch_ids[0]`, which is arbitrary on multi-touch).
fixed, the internal touch-drag path (and `GetTouchPointLocation()` along with it) was removed entirely, so touch drags now go through `wl_data_device.start_drag()` like mouse drags. The compositor owns the touch grab from the down event whose serial we pass to `start_drag`, and is responsible for whatever it wants to do with additional touch points
Related details
- Kramer Ge
- Nick Yamane
- Thomas Anderson
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")
It seems the wayland changes are no longer needed. Is the rest of this patch needed to be reviewed?
Related details
- Helmut Januschka
- Nick Yamane
- Thomas Anderson
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
It seems the wayland changes are no longer needed. Is the rest of this patch needed to be reviewed?
yes the rest is still required to properly support touch drag on linux
Related details
- Kramer Ge
- Nick Yamane
- Thomas Anderson
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaIt seems the wayland changes are no longer needed. Is the rest of this patch needed to be reviewed?
yes the rest is still required to properly support touch drag on linux
it was just the hyperland quirks, that landed upstream and i removed it in latest PS, but rest of the CL still valid and required
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaIt seems the wayland changes are no longer needed. Is the rest of this patch needed to be reviewed?
Helmut Januschkayes the rest is still required to properly support touch drag on linux
it was just the hyperland quirks, that landed upstream and i removed it in latest PS, but rest of the CL still valid and required
I'm just asking that newly added ozone/wayland code is unused in this PS. `GetTouchPointLocation()` is not referenced, and `internal_touch_drag_` is always false. So `ui/ozone/platform/wayland/host/` file changes are nop.
Related details
- Helmut Januschka
- Nick Yamane
- Thomas Anderson