aura: Clear window tracking state on kMouseExited for child windows [chromium/src : main]
Michael Wojcicka (Gerrit)
Michael Wojcicka added 1 comment![]( "Open in Gerrit")
FYI: I'm going to run the CQ but it will still fail on the previous test because I did not fix it yet. However I added more unit tests, which I want to run on Win/Mac to see if they pass.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wojcicka (Gerrit)
Michael Wojcicka added 1 comment![]( "Open in Gerrit")
FYI: I'm going to run the CQ but it will still fail on the previous test because I did not fix it yet. However I added more unit tests, which I want to run on Win/Mac to see if they pass.
Sorry, just Windows (since macOS doesn't use Aura).
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 2 comments![]( "Open in Gerrit")
recording_view->exit_location());I think you need to test this in interactive_u_tests, which generates actual native events. I also think you need a child (embedded) window as a target of the native window?
Related details
- Michael Wojcicka
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. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
youI
sorry for incomplete comment. Could you also verify that added tests fails without your patch?
Michael Wojcicka (Gerrit)
Michael Wojcicka added 2 comments![]( "Open in Gerrit")
ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow(),Apparently `ui_controls` is not supported on ChromeOS.
Oshima-san, is `ui::test::EventGenerator` the recommended approach on CrOS?
base::RunLoop run_loop;Oshima-san, I know `base::RunLoop`'s are generally discouraged in production code, but here it doesn't seem too bad to use. WDYT?
Related details
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. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow(),Apparently `ui_controls` is not supported on ChromeOS.
Oshima-san, is `ui::test::EventGenerator` the recommended approach on CrOS?
It is supported. You have to addd test to interactive_ui_tests. please see
ash/drag_drop/drag_drop_interactive_uitest.cc
Related details
- Michael Wojcicka
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. |
Michael Wojcicka (Gerrit)
Michael Wojcicka added 1 comment![]( "Open in Gerrit")
ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow(),Mitsuru OshimaApparently `ui_controls` is not supported on ChromeOS.
Oshima-san, is `ui::test::EventGenerator` the recommended approach on CrOS?
It is supported. You have to addd test to interactive_ui_tests. please see
ash/drag_drop/drag_drop_interactive_uitest.cc
- Mitsuru Oshima
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. |
Michael Wojcicka (Gerrit)
Michael Wojcicka added 1 comment![]( "Open in Gerrit")
I think you need to test this in interactive_u_tests, which generates actual native events. I also think you need a child (embedded) window as a target of the native window?
Done
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 3 comments![]( "Open in Gerrit")
// Only handle native events here.How about
// The
child_widget->Show();Don't you have to create a child window as well so that the target window is different from root window?
base::RunLoop run_loop;Oshima-san, I know `base::RunLoop`'s are generally discouraged in production code, but here it doesn't seem too bad to use. WDYT?
- Michael Wojcicka
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. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
// Only handle native events here.How about
// The
oops sorry how about something like
// The following code is to translate the enter/exit event for platform window to ones for aura window. Filter out synthesized events as they're already translated.
Michael Wojcicka (Gerrit)
Michael Wojcicka added 3 comments![]( "Open in Gerrit")
Mitsuru OshimaHow about
// The
oops sorry how about something like
// The following code is to translate the enter/exit event for platform window to ones for aura window. Filter out synthesized events as they're already translated.
Done
Don't you have to create a child window as well so that the target window is different from root window?
I believe a `child_widget` has a window? We create `parent_widget` first and then we create `child_widget` and set `child_params.parent = parent_widget->GetNativeView()` when creating it.
Here is a stacktrace showing they both have `aura::Window`'s created: https://paste.googleplex.com/6282876755771392
I updated the comment above to reflect this better.
base::RunLoop run_loop;Mitsuru OshimaOshima-san, I know `base::RunLoop`'s are generally discouraged in production code, but here it doesn't seem too bad to use. WDYT?
You can use TestFuture<void>
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 2 comments![]( "Open in Gerrit")
can you look into win-rel failure?
child_widget->Show();Michael WojcickaDon't you have to create a child window as well so that the target window is different from root window?
I believe a `child_widget` has a window? We create `parent_widget` first and then we create `child_widget` and set `child_params.parent = parent_widget->GetNativeView()` when creating it.
Here is a stacktrace showing they both have `aura::Window`'s created: https://paste.googleplex.com/6282876755771392
I updated the comment above to reflect this better.
Sorry I missed TYPE_CONTROL. That's being said, can you use NativeViewHost to attach the child window instead of just adding a child aura window to parent? That should be closer to the real scenario.
Make sure that repros the issue without the fix.
Related details
- Michael Wojcicka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Wojcicka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Michael Wojcicka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mitsuru Oshima
| Code-Review | +0 |
1 comment
#if BUILDFLAG(SUPPORTS_OZONE_X11)This is still not updated?
Related details
- Chromium UI Views Reviews
- Michael Wojcicka
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
kyl...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ui/views/config.gwsq)
Related details
- Allen Bauer
- Michael Wojcicka
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. |
Michael Wojcicka (Gerrit)
Michael Wojcicka added 1 comment![]( "Open in Gerrit")
#if BUILDFLAG(SUPPORTS_OZONE_X11)This is still not updated?
Oh sorry, from your chat message I didn't realize you wanted me to use the method you added, you mentioned setting the CL to upstream. I thought it resolved some other issues indirectly with this CL.
Related details
- Allen Bauer
- Mitsuru Oshima
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
#if BUILDFLAG(SUPPORTS_OZONE_X11)Michael WojcickaThis is still not updated?
Oh sorry, from your chat message I didn't realize you wanted me to use the method you added, you mentioned setting the CL to upstream. I thought it resolved some other issues indirectly with this CL.
Please use RunningOnX11ForTest instead. That's the correct condition, as the connection can be null for various reasons (e.g. before it is initialized)
Related details
- Allen Bauer
- Michael Wojcicka
Michael Wojcicka (Gerrit)
Michael Wojcicka added 1 comment![]( "Open in Gerrit")
#if BUILDFLAG(SUPPORTS_OZONE_X11)Michael WojcickaThis is still not updated?
Mitsuru OshimaOh sorry, from your chat message I didn't realize you wanted me to use the method you added, you mentioned setting the CL to upstream. I thought it resolved some other issues indirectly with this CL.
Please use RunningOnX11ForTest instead. That's the correct condition, as the connection can be null for various reasons (e.g. before it is initialized)
- Allen Bauer
- Mitsuru Oshima
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. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 2 comments![]( "Open in Gerrit")
#include "ui/gfx/x/wm_sync.h"these two should be IS_LINUX (CrOS has supports_ozone_x11, which should be excluded)
You can include ozone_platform.h inside IS_LINUX because that's only case you need it.
void WaitForWindowSystemSync() {Same here, IS_LINUX
Related details
- Allen Bauer
- Michael Wojcicka
Michael Wojcicka (Gerrit)
Michael Wojcicka added 3 comments![]( "Open in Gerrit")
these two should be IS_LINUX (CrOS has supports_ozone_x11, which should be excluded)
You can include ozone_platform.h inside IS_LINUX because that's only case you need it.
Done.
#if BUILDFLAG(SUPPORTS_OZONE_X11)Michael WojcickaThis is still not updated?
Mitsuru OshimaOh sorry, from your chat message I didn't realize you wanted me to use the method you added, you mentioned setting the CL to upstream. I thought it resolved some other issues indirectly with this CL.
Michael WojcickaPlease use RunningOnX11ForTest instead. That's the correct condition, as the connection can be null for various reasons (e.g. before it is initialized)
Should be fixed, please take a look 😊
Done
void WaitForWindowSystemSync() {Michael WojcickaSame here, IS_LINUX
Done
Related details
- Allen Bauer
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mitsuru Oshima
| Code-Review | +1 |
2 comments
#if BUILDFLAG(SUPPORTS_OZONE_X11)move this to L40, as RunningOnX11ForTest should be runtime check.
Related details
- Allen Bauer
- Michael Wojcicka
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. |
Michael Wojcicka (Gerrit)
Michael Wojcicka added 1 comment![]( "Open in Gerrit")
move this to L40, as RunningOnX11ForTest should be runtime check.
Done
Related details
- Allen Bauer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
m...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ui/views/config.gwsq)
Related details
- Mike Wasserman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Wasserman (Gerrit)
Mike Wasserman added 6 comments![]( "Open in Gerrit")
Thanks for the thorough investigation and fix!
void SetUp() override {It doesn't look like setting `DIR_USER_DATA` or `ax_platform_` is necessary for testing cursor events. Other `AshInteractiveUITestBase` tests (e.g., `drag_drop_interactive_uitest.cc`) work fine without overriding `SetUp()`.
Consider removing `SetUp()`, `user_data_dir_`, `ax_platform_`, and their corresponding `#include`s unless there's a specific reason they are needed here.
DisableMouseEventsGeneratesExitEventOnChild) {Since the bug is fundamentally an Aura issue (`WindowEventDispatcher::PreDispatchMouseEvent`), could we test this in `ui/aura/window_event_dispatcher_unittest.cc` instead?
Interactive UI tests are heavy and prone to flakiness. We can simulate the exact same scenario by creating a parent/child window, dispatching a `kMouseMoved` to the child, calling `cursor_client.DisableMouseEvents()`, dispatching a `kMouseExited`, and asserting the child window's `EventFilterRecorder` catches it.
This would allow us to remove this new interactive test suite entirely.
// The following code is to translate the enter/exit event from platform
// windows to aura windows. Filter out synthesized events as they're
// already translated.
if (event->flags() & ui::EF_IS_SYNTHESIZED) {
break;
}Is this explicit handling needed? Consider removing if not.
void WaitForWindowSystemSync() {The test coverage is limited to IS_WIN, so is this actually needed?
TEST_F(DesktopNativeWidgetAuraTest, MouseEnterExitWithOverlapWindow) {Similarly to the Ash interactive test, this scenario can likely be covered by a pure Aura unit test in `ui/aura/window_event_dispatcher_unittest.cc`.
While this test uses actual OS windows to force a native `MouseLeave` event, the bug itself is in how `WindowEventDispatcher` routes that event once it reaches Aura.
You can simulate this overlapping window scenario by setting up a root window with a child, dispatching a `kMouseMoved` over the child, and then directly dispatching a `kMouseExited` to the root window (simulating the OS-level exit). Testing it at the Aura layer removes the need for `ui_controls`, `DesktopNativeWidgetAura`, and OS-specific syncs like `WaitForWindowSystemSync`.
Related details
- Michael Wojcicka
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. |