New issue
Advanced search Search tips

Issue 852241 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocking:
issue 754101


Participants' hotlists:
Harmony-Cast-Dialog


Sign in to add a comment

[Harmony Cast Dialog] Dialog cannot be opened from the hotdog menu with touch on Windows

Project Member Reported by taku...@chromium.org, Jun 13 2018

Issue description

The dialog can be opened from the toolbar or context menu with touch or a click.
It can also be opened from the hotdog menu via a click, but not with touch.
This is likely due to the dialog losing focus upon creation, which seems to be happening in some other environments as well.
 
Opening with touch works fine on Pixelbook.
Labels: -M-69 -Target-69 Target-70 M-70
Cc: bsep@chromium.org
When the dialog is opened via the app menu with a mouse click, BubbleDialogDelegateView::OnWidgetActivationChanged() is called twice: first with active=false, and second with active=true. The dialog stays open, as intended.

When the dialog is opened with touch via the app menu, OnWidgetActivationChanged() is called four times, with active=false,true,false,true. The third call is triggering the dialog to close immediately.

When you open the app menu, slide your finger on a menu item and release (this neither selects the item nor closes the menu), then touch on "Cast", OnWidgetActivationChanged() is called twice with active=false,true, and the dialog stays open.

Bret, do you have any idea for why the above is the case?
Not totally sure, but I would guess you're getting both the main touch event and the compatibility event, which is an architecture issue. Touch is due for some serious attention on Windows.
The code stack for event processing here is very deep and forking.  It would probably help narrow it down by printing a stack trace in OnWidgetActivationChanged so we can see which event handlers are responsible.

Probably the answer lies somewhere in the aura event dispatch code, i.e.
https://cs.chromium.org/chromium/src/ui/aura/window_event_dispatcher.cc?rcl=de611de1b0ace13d48f3dd392a67d03b33986a55&l=193

Unfortunately, OnWidgetActivationChanged() is called asynchronously and the stack isn't very helpful:
        views::BubbleDialogDelegateView::OnDeactivate [0x6A3834DD+63]
        views::BubbleDialogDelegateView::OnWidgetActivationChanged [0x6A383498+46]
        views::Widget::OnNativeWidgetActivationChanged [0x6A3E9601+125]
        views::DesktopNativeWidgetAura::HandleActivationChanged [0x6A40C7E6+24]
        views::HWNDMessageHandler::PostProcessActivateMessage [0x6A3F710D+135]
        views::HWNDMessageHandler::OnWndProc [0x6A3F702E+328]
        gfx::WindowImpl::WndProc [0x66B3F4B8+156]
        gfx::win::TextAnalysisSource::Create [0x66B3EED6+2210]
        AddClipboardFormatListener [0x73D4BE6B+1179]
        DispatchMessageW [0x73D4833A+2426]
        DispatchMessageW [0x73D47EDA+1306]
        SystemParametersInfoW [0x73D4A629+1193]
        KiUserCallbackDispatcher [0x771AC66D+77]
        PeekMessageW [0x73D48FF4+308]
        base::MessagePumpForUI::ProcessPumpReplacementMessage [0x6DAC6502+50]
        base::MessagePumpForUI::ProcessMessageHelper [0x6DAC66D6+262]
        base::MessagePumpForUI::DoRunLoop [0x6DAC61DF+79]
        base::MessagePumpWin::Run [0x6DAC5C2E+110]
        base::MessageLoop::Run [0x6DAC1CE7+119]

I'll look into WindowEventDispatcher.
WindowEventDispatcher::ProcessedTouchEvent() seems to only apply to touches inside WebContents.

Bret, where can I confirm that the extra event I'm seeing is indeed a compatibility event? Code-searching for it only got me files in blink/.
I believe the real touch events are in HWNDMessageHandler::HandleTouchMessage or HWNDMessageHandler::HandlePointerMessage, and the compatibility ones are in HWNDMessageHandler::HandleMouseEventInternal (see ui::IsMouseEventFromTouch). Though if that is what's happening, it's a huge rabbit hole. I'm working on cleaning this up.
Cc: powerb@chromium.org
Labels: -Target-70 -M-70 Target-71 M-71
Cc: -bsep@chromium.org taku...@chromium.org
Owner: bsep@chromium.org
Bret, I'm assigning this over to you based on our conversation over email. Thanks!
Status: Started (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 12

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

commit c6058c3e1f9a4118c88bf487e6c5a3fe2f562269
Author: Bret Sepulveda <bsep@chromium.org>
Date: Fri Oct 12 00:36:13 2018

Mark touch released events handled by default on Windows.

When a touch event is not handled, WindowEventDispatcher will generate
gesture events from it. However, when those gesture events are marked as
handled that handling doesn't propagate back to the original touch
event.

For  bug 852241 , the hamburger menu only handles gesture events. On
Windows, selecting an option with a touch would cause the hamburger menu
to close, but because the touch event wasn't marked as handled Windows
would pass it along to the underlying window, causing it to gain focus.
Most of the time it would be gaining focus anyway (since the hamburger
menu just closed) so this isn't noticeable, but the Cast dialog is a
newly-added surface that closes if it loses focus. In this case, it
seems to not appear at all.

Similarly for  bug 866421 , when the guest window becomes restored it
reveals the window underneath and then Windows passes the event along,
giving it focus.

This patch marks all touch released as handled by default, which
prevents Windows from ever propagating them. These events generate the
tap gestures that cause the state change in the hamburger menu and
window size in the above bugs.

Bug:  852241 ,  866421 
Change-Id: I10a67d56fdc46d5b91282e0f395c895339f71951
Reviewed-on: https://chromium-review.googlesource.com/c/1260507
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599037}
[modify] https://crrev.com/c6058c3e1f9a4118c88bf487e6c5a3fe2f562269/ui/views/win/hwnd_message_handler.cc

Status: Fixed (was: Started)
Thank you Bret :)

Sign in to add a comment