Skip to content

GitLab

T
tangled
Commit 6e69edc6 authored by Maksim Sisov's avatar Maksim Sisov Committed by Commit Bot
Browse files
Options

ozone/wayland: Do not grab if no seat available. 

If seat is not available, and Chromium tries to create a popup
window (like in browser tests), Wayland will drop the connection
with a protocol error.

Thus, add additional check for that. Also, create "CanGrabPopup"
method so that the same condition is shared between xdg-unstable-v6
and xdg-stable.

Test: WaylandWindowTest.DoesNotGrabPopupIfNoSeat
Bug: 578890
Change-Id: If47d9044c45b37e149fae1d67b244c54a99ee4e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207378Reviewed-by: default avatarNick Yamane <nickdiego@igalia.com>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#770086}
parent c29fc1e2
Hide whitespace changes
Inline Side-by-side
Showing with 58 additions and 63 deletions
ui/ozone/platform/wayland/host/wayland_window_unittest.cc
View file @ 6e69edc6
......@@ -2065,6 +2065,21 @@ TEST_P(WaylandWindowTest, NestedPopupWindowsGetCorrectParent) {
EXPECT_TRUE(menu_window4->parent_window() == menu_window3.get());
}
TEST_P(WaylandWindowTest, DoesNotGrabPopupIfNoSeat) {
// Create a popup window and verify the grab serial is not set.
MockPlatformWindowDelegate delegate;
auto popup = CreateWaylandWindowWithParams(
PlatformWindowType::kMenu, window_->GetWidget(), gfx::Rect(0, 0, 50, 50),
&delegate);
ASSERT_TRUE(popup);
Sync();
auto* test_popup = GetPopupByWidget(popup->GetWidget());
ASSERT_TRUE(test_popup);
EXPECT_EQ(test_popup->grab_serial(), 0u);
}
INSTANTIATE_TEST_SUITE_P(XdgVersionStableTest,
WaylandWindowTest,
::testing::Values(kXdgShellStable));
......
ui/ozone/platform/wayland/host/xdg_popup_wrapper_impl.cc
View file @ 6e69edc6
......@@ -324,41 +324,8 @@ bool XDGPopupWrapperImpl::InitializeStable(
xdg_positioner_destroy(positioner);
// According to the spec, the grab call can only be done on a key press, mouse
// press or touch down. However, there is a problem with popup windows and
// touch events so long as Chromium creates them only on consequent touch up
// events. That results in Wayland compositors dismissing popups. To fix
// the issue, do not use grab with touch events. Please note that explicit
// grab means that a Wayland compositor dismisses a popup whenever the user
// clicks outside the created surfaces. If the explicit grab is not used, the
// popups are not dismissed in such cases. What is more, current non-ozone X11
// implementation does the same. This means there is no functionality changes
// and we do things right.
//
// We cannot know what was the last event. Instead, we can check if the window
// has pointer or keyboard focus. If so, the popup will be explicitly grabbed.
//
// There is a bug in the gnome/mutter - if explicit grab is not used,
// unmapping of a wl_surface (aka destroying xdg_popup and surface) to hide a
// window results in weird behaviour. That is, a popup continues to be visible
// on a display and it results in a crash of the entire session. Thus, just
// continue to use grab here and avoid showing popups for touch events on
// gnome/mutter. That is better than crashing the entire system. Otherwise,
// Chromium has to change the way how it reacts on touch events - instead of
// creating a menu on touch up, it must do it on touch down events.
// https://gitlab.gnome.org/GNOME/mutter/issues/698#note_562601.
std::unique_ptr<base::Environment> env(base::Environment::Create());
if ((base::nix::GetDesktopEnvironment(env.get()) ==
base::nix::DESKTOP_ENVIRONMENT_GNOME) ||
(wayland_window_->parent_window()->has_pointer_focus() ||
wayland_window_->parent_window()->has_keyboard_focus())) {
// When drag process starts, as described the protocol -
// https://goo.gl/1Mskq3, the client must have an active implicit grab. If
// we try to create a popup and grab it, it will be immediately dismissed.
// Thus, do not take explicit grab during drag process.
if (!connection->IsDragInProgress())
xdg_popup_grab(xdg_popup_.get(), connection->seat(),
connection->serial());
if (CanGrabPopup(connection)) {
xdg_popup_grab(xdg_popup_.get(), connection->seat(), connection->serial());
}
xdg_popup_add_listener(xdg_popup_.get(), &xdg_popup_listener, this);
......@@ -419,34 +386,7 @@ bool XDGPopupWrapperImpl::InitializeV6(
zxdg_positioner_v6_destroy(positioner);
// According to the spec, the grab call can only be done on a key press, mouse
// press or touch down. However, there is a problem with popup windows and
// touch events so long as Chromium creates them only on consequent touch up
// events. That results in Wayland compositors dismissing popups. To fix
// the issue, do not use grab with touch events. Please note that explicit
// grab means that a Wayland compositor dismisses a popup whenever the user
// clicks outside the created surfaces. If the explicit grab is not used, the
// popups are not dismissed in such cases. What is more, current non-ozone X11
// implementation does the same. This means there is no functionality changes
// and we do things right.
//
// We cannot know what was the last event. Instead, we can check if the window
// has pointer or keyboard focus. If so, the popup will be explicitly grabbed.
//
// There is a bug in the gnome/mutter - if explicit grab is not used,
// unmapping of a wl_surface (aka destroying xdg_popup and surface) to hide a
// window results in weird behaviour. That is, a popup continues to be visible
// on a display and it results in a crash of the entire session. Thus, just
// continue to use grab here and avoid showing popups for touch events on
// gnome/mutter. That is better than crashing the entire system. Otherwise,
// Chromium has to change the way how it reacts on touch events - instead of
// creating a menu on touch up, it must do it on touch down events.
// https://gitlab.gnome.org/GNOME/mutter/issues/698#note_562601.
std::unique_ptr<base::Environment> env(base::Environment::Create());
if ((base::nix::GetDesktopEnvironment(env.get()) ==
base::nix::DESKTOP_ENVIRONMENT_GNOME) ||
(wayland_window_->parent_window()->has_pointer_focus() ||
wayland_window_->parent_window()->has_keyboard_focus())) {
if (CanGrabPopup(connection)) {
zxdg_popup_v6_grab(zxdg_popup_v6_.get(), connection->seat(),
connection->serial());
}
......@@ -508,6 +448,44 @@ MenuType XDGPopupWrapperImpl::GetMenuTypeForPositioner(
return MenuType::TYPE_3DOT_CHILD_MENU;
}
bool XDGPopupWrapperImpl::CanGrabPopup(WaylandConnection* connection) const {
// When drag process starts, as described the protocol -
// https://goo.gl/1Mskq3, the client must have an active implicit grab. If
// we try to create a popup and grab it, it will be immediately dismissed.
// Thus, do not take explicit grab during drag process.
if (connection->IsDragInProgress() || !connection->seat())
return false;
// According to the spec, the grab call can only be done on a key press, mouse
// press or touch down. However, there is a problem with popup windows and
// touch events so long as Chromium creates them only on consequent touch up
// events. That results in Wayland compositors dismissing popups. To fix the
// issue, do not use grab with touch events. Please note that explicit grab
// means that a Wayland compositor dismisses a popup whenever the user clicks
// outside the created surfaces. If the explicit grab is not used, the popups
// are not dismissed in such cases. What is more, current non-ozone X11
// implementation does the same. This means there is no functionality changes
// and we do things right.
//
// We cannot know what was the last event. Instead, we can check if the window
// has pointer or keyboard focus. If so, the popup will be explicitly grabbed.
//
// There is a bug in the gnome/mutter - if explicit grab is not used,
// unmapping of a wl_surface (aka destroying xdg_popup and surface) to hide a
// window results in weird behaviour. That is, a popup continues to be visible
// on a display and it results in a crash of the entire session. Thus, just
// continue to use grab here and avoid showing popups for touch events on
// gnome/mutter. That is better than crashing the entire system. Otherwise,
// Chromium has to change the way how it reacts on touch events - instead of
// creating a menu on touch up, it must do it on touch down events.
// https://gitlab.gnome.org/GNOME/mutter/issues/698#note_562601.
std::unique_ptr<base::Environment> env(base::Environment::Create());
return (base::nix::GetDesktopEnvironment(env.get()) ==
base::nix::DESKTOP_ENVIRONMENT_GNOME) ||
(wayland_window_->parent_window()->has_pointer_focus() ||
wayland_window_->parent_window()->has_keyboard_focus());
}
void XDGPopupWrapperImpl::Configure(void* data,
int32_t x,
int32_t y,
......
ui/ozone/platform/wayland/host/xdg_popup_wrapper_impl.h
View file @ 6e69edc6
......@@ -44,6 +44,8 @@ class XDGPopupWrapperImpl : public ShellPopupWrapper {
MenuType GetMenuTypeForPositioner(WaylandConnection* connection,
WaylandWindow* parent_window) const;
bool CanGrabPopup(WaylandConnection* connection) const;
// xdg_popup_listener
static void Configure(void* data,
int32_t x,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment