New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 788782 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Improve popup window support in exo

Project Member Reported by reve...@chromium.org, Nov 27 2017

Issue description

Popup windows in exo have the following limitations:

- They don't grab input properly.
- They are not dismissed when clicking outside input region of window.
- They are stacked below the shelf.
- Placement is not adjusted for screen bounds.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/032f83cffa91d583f46f15dbc0bd29b099417fdb

commit 032f83cffa91d583f46f15dbc0bd29b099417fdb
Author: David Reveman <reveman@chromium.org>
Date: Tue Nov 28 03:57:00 2017

exo: Improve placement of XDG popups.

SetParent was called by mistake for popups. That only makes
sense for top level shell surfaces. This removes the incorrect
SetParent call so that SetContainer works as expected and
switches the container type to 'menu'. Menu container behavior
matches the expected XDG client behavior much better.

The result of this change is that popups are no longer placed
below the shelf.

Bug:  788782 
Test: weston-terminal
Change-Id: If2b1fc54f23ffc517c767d9f52511d20a34d6b19
Reviewed-on: https://chromium-review.googlesource.com/792274
Commit-Queue: David Reveman <reveman@chromium.org>
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519555}
[modify] https://crrev.com/032f83cffa91d583f46f15dbc0bd29b099417fdb/components/exo/shell_surface.cc
[modify] https://crrev.com/032f83cffa91d583f46f15dbc0bd29b099417fdb/components/exo/wayland/server.cc

Owner: osh...@chromium.org
oshima@, would be great if you could provide some advice for how to best solve these issues in chrome.

The stacking issue has been fixed but other issues remain. The spec for popups can be found here:

https://cs.chromium.org/chromium/src/third_party/wayland-protocols/src/unstable/xdg-shell/xdg-shell-unstable-v6.xml?l=917

I think the most pressing issue is the lack of dismiss when clicking in a non client window. gtk3-demo-application is a good test app in general.
Labels: -M-64 M-68
Status: Assigned (was: Available)
Labels: Proj-Containers
Tagging this as Proj-Containers so it's a big more discoverable.
Components: OS>Systems>Containers
Labels: Hotlist-Crostini-UI
Cc: tbuck...@chromium.org
oshima@, do you think you'll be able to work on this soon? asking as we might want to find someone else who can work on this if not


Comment 8 by osh...@chromium.org, May 24 2018

I can look at after crbug.com/72235199. I have prototype working for 72235199, I had to put this due to other Arc N works.

If you think I should do this first, then I can do this first though.
do you mean b/72235199? Either order is fine. b/72235199 will be less important once we enable underlays but still worth doing I think. As using an opaque overlay uses less power.
Status: Started (was: Assigned)
Cc: reve...@chromium.org
 Issue 846191  has been merged into this issue.
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a96973281585e691895f83a48242c9b7ca358462

commit a96973281585e691895f83a48242c9b7ca358462
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Tue Jun 19 07:11:53 2018

Improve xdg_popup support

* use TYPE_POPUP for xdg_popup. The differences from TYPE_WINDOW are:
  - no frame
  - ash will not control its bounds
* make popups transient children: this fixes following scenarios
  - z order is correctly updated when parent's z order changes
  - they're grouped in overview.
* implement grab
  - capture even when the grab is requested on the surface
  - transfer capture to child if the child popup requested
     grab.
  - close the surface when capture(grab) is lost.
  - but transfer the grab to the parent if the parent had grab.

BUG= 788782 
TEST=covered by unit tests. manually tested with gtk3-demo.

Change-Id: I73f5bc7e555f2c28f78dc25a55f77f0a70fd9b78
Reviewed-on: https://chromium-review.googlesource.com/1102354
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568364}
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/pointer.cc
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/pointer.h
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/shell_surface.cc
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/shell_surface.h
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/shell_surface_base.cc
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/shell_surface_base.h
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/shell_surface_unittest.cc
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/touch.cc
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/touch.h
[modify] https://crrev.com/a96973281585e691895f83a48242c9b7ca358462/components/exo/wayland/server.cc

Followings are fixed.

> They don't grab input properly.
> They are not dismissed when clicking outside input region of window.

but following are not, but I'm not sure how to fix this because
ash can't distinguish normal popup window and 

> They are stacked below the shelf.

Normal popup window can be below shelf. (at least in ash)
If we want to keep menu above shelf, we need a way to distinguish
normal popup + grab and one used for menu.

> Placement is not adjusted for screen bounds.

In ash, the client code (that create menu widget) adjusts the bounds based
on the screen size and parent's location. How this is suppose to work in wayland?

> > They are stacked below the shelf.
>
> Normal popup window can be below shelf. (at least in ash)
> If we want to keep menu above shelf, we need a way to distinguish
> normal popup + grab and one used for menu.

hm, I guess this latest change regressed in this regard as #1 solve that before. We should fix that asap. Either we make all exo popups above the shelf or we fix it using improved placement by avoiding to have the popups placed below the shelf.

> > Placement is not adjusted for screen bounds.
>
> In ash, the client code (that create menu widget) adjusts the bounds based
> on the screen size and parent's location. How this is suppose to work in wayland?

The xdg_positioner interface provides all information needed to adjust the position of the popups. See https://cs.chromium.org/chromium/src/third_party/wayland-protocols/src/unstable/xdg-shell/xdg-shell-unstable-v6.xml?l=115
> We should fix that asap. Either we make all exo popups above the shelf or we fix it using improved placement by avoiding to have the popups placed below the shelf.

There are a couple of issues using menu container.
* is all popup menu?
* menu container is above lock screen, so it gives an app a way to open something above lock screen. (Ash closes all menus before locking)

That's being said, if we place all menus inside the work are (which ash does), this won't be an issue?


> this won't be an issue?
you mentioned it in the last phrase. Sorry about that.

I'll look into it, since using menu container is a security risk.
We're allowed to dismiss all popups when lock screen is showing. We probably should, as I'd be surprised to come bad to a popup after unlocking the device.

Xdg popups are used almost exclusive for menus afaict. I think we want these menus to behave as close to other menus in Chrome as possible. E.g. if other menus appear above the shelf and are not constrained to the workarea, then xdg popups should ideally behave the same way.
Project Member

Comment 18 by bugdroid1@chromium.org, Jun 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8871e7c2bcfd2ad4d35bf9dc1034b14d6f0f9cd7

commit 8871e7c2bcfd2ad4d35bf9dc1034b14d6f0f9cd7
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Wed Jun 20 17:35:57 2018

Implement bounds adjustment to satisfy the work area constraints.

This is step 1.
Support sliding and flipping. The spec is a bit vague but this should
give the same behavior as when running on X, except that flip
propagation.

In step 2, I'll implement flip state propagation.
That is, once the direction is flipped, it should be propagated
to the children until it hits the opposite constraints.

BUG= 788782 
TEST=manually tested with gtk3-demo.

Change-Id: If288b997e68e140738529f56ebe619d28e1b474d
Reviewed-on: https://chromium-review.googlesource.com/1107130
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568903}
[modify] https://crrev.com/8871e7c2bcfd2ad4d35bf9dc1034b14d6f0f9cd7/components/exo/wayland/server.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Jun 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fbe272fb890293c9dbd68e38fd35ca6ea0392136

commit fbe272fb890293c9dbd68e38fd35ca6ea0392136
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Sat Jun 23 22:37:29 2018

Propagate flip layout proprety of the popups

BUG= 788782 
TEST=manully tested with gtk3-demo

Change-Id: I4a381b9350675d4e5407b378fbe07ae9de64fe8a
Reviewed-on: https://chromium-review.googlesource.com/1108558
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569893}
[modify] https://crrev.com/fbe272fb890293c9dbd68e38fd35ca6ea0392136/components/exo/wayland/server.cc
[modify] https://crrev.com/fbe272fb890293c9dbd68e38fd35ca6ea0392136/components/exo/xdg_shell_surface.h

Project Member

Comment 20 by bugdroid1@chromium.org, Jun 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0af15452ecfa18ee4f67b90f19feed93b7fb33f1

commit 0af15452ecfa18ee4f67b90f19feed93b7fb33f1
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Mon Jun 25 18:28:11 2018

Release capture when locking screen

The capture window will not receive events while locked because
there is an extra mechanism to block events in locked state, but
having capture window in locked state will make lock UI unusable
by mouse/touch.

Explicitly release capture so that mouse/touch event works
when locked.

BUG= 788782 
TEST=covered by unittest

Change-Id: I01849f71ec0f059a21b433295fbdf816962d44d4
Reviewed-on: https://chromium-review.googlesource.com/1112938
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570100}
[modify] https://crrev.com/0af15452ecfa18ee4f67b90f19feed93b7fb33f1/ash/login/ui/lock_screen.cc
[modify] https://crrev.com/0af15452ecfa18ee4f67b90f19feed93b7fb33f1/ash/login/ui/lock_window_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 22 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6488ecd773add2cef8efdeab1c0500109e14e95d

commit 6488ecd773add2cef8efdeab1c0500109e14e95d
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Tue Jun 26 23:41:16 2018

Revert "Propagate flip layout proprety of the popups"

This reverts commit fbe272fb890293c9dbd68e38fd35ca6ea0392136.

Reason for revert: exo_unittests are flaky on Linux ASan:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29?limit=200

Original change's description:
> Propagate flip layout proprety of the popups
> 
> BUG= 788782 
> TEST=manully tested with gtk3-demo
> 
> Change-Id: I4a381b9350675d4e5407b378fbe07ae9de64fe8a
> Reviewed-on: https://chromium-review.googlesource.com/1108558
> Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: David Reveman <reveman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#569893}

TBR=reveman@chromium.org,oshima@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  788782 
Change-Id: Iae9444d706ff01f37787781955bfbbb8dff63c01
Reviewed-on: https://chromium-review.googlesource.com/1115918
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570586}
[modify] https://crrev.com/6488ecd773add2cef8efdeab1c0500109e14e95d/components/exo/wayland/server.cc
[modify] https://crrev.com/6488ecd773add2cef8efdeab1c0500109e14e95d/components/exo/xdg_shell_surface.h

Project Member

Comment 23 by bugdroid1@chromium.org, Jul 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/22ec5b518fd636d35c529b736b4ceec1a911fb31

commit 22ec5b518fd636d35c529b736b4ceec1a911fb31
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Mon Jul 02 16:04:01 2018

Reland "Propagate flip layout proprety of the popups"

This reverts commit 6488ecd773add2cef8efdeab1c0500109e14e95d.

Reason for revert: the leak was identified in crbug.com/859262,
and same leak happened after this revert.

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/28109

Original change's description:
> Revert "Propagate flip layout proprety of the popups"
> 
> This reverts commit fbe272fb890293c9dbd68e38fd35ca6ea0392136.
> 
> Reason for revert: exo_unittests are flaky on Linux ASan:
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29?limit=200
> 
> Original change's description:
> > Propagate flip layout proprety of the popups
> > 
> > BUG= 788782 
> > TEST=manully tested with gtk3-demo
> > 
> > Change-Id: I4a381b9350675d4e5407b378fbe07ae9de64fe8a
> > Reviewed-on: https://chromium-review.googlesource.com/1108558
> > Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
> > Reviewed-by: David Reveman <reveman@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#569893}
> 
> TBR=reveman@chromium.org,oshima@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  788782 
> Change-Id: Iae9444d706ff01f37787781955bfbbb8dff63c01
> Reviewed-on: https://chromium-review.googlesource.com/1115918
> Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
> Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#570586}

TBR=dgozman@chromium.org,reveman@chromium.org,oshima@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  788782 
Change-Id: Ibb60d892830e070683ac1d8190144d50fa8d876a
Reviewed-on: https://chromium-review.googlesource.com/1122876
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571909}
[modify] https://crrev.com/22ec5b518fd636d35c529b736b4ceec1a911fb31/components/exo/wayland/server.cc
[modify] https://crrev.com/22ec5b518fd636d35c529b736b4ceec1a911fb31/components/exo/xdg_shell_surface.h

Sign in to add a comment