Mash Shell Shelf Overflow Bubble |
|||||
Issue descriptionMash Shell Shelf Overflow Bubble The mash shelf shows an overflow button, but clicking it causes a crash. See parent Issue 557406
,
Apr 28 2016
I'll take a look at the crash now
,
Apr 28 2016
+jonross I have a fix for the crash out for review at https://codereview.chromium.org/1929203002/ However, even with the crash fixed the overflow bubble doesn't open. Rather, it opens then immediately closes. This is due to a focus/activation issue -- the bubble sees an activation change and closes itself. The stack is below; I'm not sure why the InFlightFocusChange is reverted. Maybe there are two competing focus requests? #1 0x7f3a759adf48 views::NativeWidgetMus::Close() #2 0x7f3a76164428 views::Widget::Close() #3 0x7f3a7607b287 views::BubbleDialogDelegateView::OnWidgetActivationChanged() #4 0x7f3a76165c2a views::Widget::OnNativeWidgetActivationChanged() #5 0x7f3a759ac49b views::NativeWidgetMus::OnActivationChanged() #6 0x7f3a759d1c80 views::WindowTreeHostMus::OnActivationChanged() #7 0x7f3a759b6bda views::PlatformWindowMus::OnWindowFocusChanged() #8 0x7f3a759f8cbe mus::WindowTreeClientImpl::LocalSetFocus() #9 0x7f3a759d3d98 mus::InFlightFocusChange::Revert() #10 0x7f3a759fca41 mus::WindowTreeClientImpl::OnChangeCompleted() #11 0x7f3a75ad2e2a mus::mojom::WindowTreeClientStub::Accept()
,
Apr 29 2016
Something is causing your focus request to be rejected by the window server. The reversion of the InFlightFocusChange is then triggered to update the client app. I'd take a look at WindowTree::SetFocus to see where it's rejecting.
,
Apr 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a5ffa2195e262691da5d7bab4ac1364622cfcde commit 2a5ffa2195e262691da5d7bab4ac1364622cfcde Author: jamescook <jamescook@chromium.org> Date: Fri Apr 29 17:44:02 2016 mash: Fix crash when clicking on the shelf overflow icon Under mus/mash, each widget has its own RootWindow. Only the RootWindow for the display has a RootWindowController. OverflowBubble was attempting to look up the display's RootWindowController from the shelf's widget, which doesn't have one. There is a separate problem that prevents the overflow bubble from showing (it immediately closes due to a focus/activation issue) but with this CL at least we don't crash on click. We'll need to audit other callers of RootWindowController::ForWindow() to ensure they are safe; I'll file a bug for that. BUG= 595851 TEST=Run mash, launch some apps, resize window to get overflow icon to show, click icon, no crash. Review-Url: https://codereview.chromium.org/1929203002 Cr-Commit-Position: refs/heads/master@{#390690} [modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/root_window_controller.h [modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/shelf/overflow_bubble.cc [modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/system/tray/tray_background_view.cc [modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/system/tray/tray_background_view.h [modify] https://crrev.com/2a5ffa2195e262691da5d7bab4ac1364622cfcde/ash/system/tray/tray_bubble_wrapper.cc
,
Apr 29 2016
The problem is that the overflow bubble widget is being created as non-activatable (WidgetDelegate::CanActivate() is false) but shown with ui::SHOW_STATE_DEFAULT which results in an attempt to activate it in NativeWidgetMus. https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/mus/native_widget_mus.cc&q=showwithwindowstate&sq=package:chromium&type=cs&l=712 It is shown with SHOW_STATE_DEFAULT due to some unusual logic in views::Widget wrt to saved_show_state_. https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/widget.cc&q=Widget::Show&sq=package:chromium&type=cs&l=612 In this case it skips the check for WidgetDelegate::CanActivate(). NativeWidgetAura::ShowWithWindowState() has an explicit check for CanActivate(): https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/native_widget_aura.cc&q=ShowWIthWindowState&sq=package:chromium&type=cs&l=496 While views::Widget is technically wrong here (it should probably also check CanActivate) there might be cases in Chrome's views code that rely on the existing Widget::Show() behavior, and it's hard to be sure. I think we should fix this by making NativeWidgetMus match the behavior of NativeWidgetAura. I'll put up a CL shortly.
,
May 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b69da6a7e57248527c6ec7fea8a5044f6ebc804d commit b69da6a7e57248527c6ec7fea8a5044f6ebc804d Author: jamescook <jamescook@chromium.org> Date: Tue May 03 18:23:06 2016 mus: Don't activate non-activatable widgets on Show() This was preventing the shelf overflow bubble from opening. See bug for details. Make NativeWidgetMus::Show() behave similarly to NativeWidgetAura and check WidgetDelegate::CanActivate() before activating the widget. BUG= 595851 TEST=added to views_mus_unittest Review-Url: https://codereview.chromium.org/1936823002 Cr-Commit-Position: refs/heads/master@{#391303} [modify] https://crrev.com/b69da6a7e57248527c6ec7fea8a5044f6ebc804d/ui/views/mus/native_widget_mus.cc [modify] https://crrev.com/b69da6a7e57248527c6ec7fea8a5044f6ebc804d/ui/views/mus/native_widget_mus_unittest.cc
,
May 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e27b07278681ba98ec911b145968315b5eb63440 commit e27b07278681ba98ec911b145968315b5eb63440 Author: jamescook <jamescook@chromium.org> Date: Tue May 03 19:59:25 2016 mash: Fix shelf overflow bubble not closing on click outside its bounds * Add screen location to views::PointerWatcher notifications. * Convert OverflowBubble from a global ui::EventHandler to a PointerWatcher, which allows it to work on mus. Add/remove the PointerWatcher in the constructor/destructor which ensures it is properly cleaned up when the widget is destroyed (unlike the previous pre-target handler). BUG= 595851 , 608508 TEST=existing ash_unittests for overflow bubble. For mash, shrink the window to show overflow arrow, click to spawn it, click outside bubble to close it. Review-Url: https://codereview.chromium.org/1934123004 Cr-Commit-Position: refs/heads/master@{#391334} [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/pointer_watcher_delegate_aura.cc [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/pointer_watcher_delegate_aura.h [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/shelf/overflow_bubble.cc [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/shelf/overflow_bubble.h [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/system/tray/tray_event_filter.cc [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ash/system/tray/tray_event_filter.h [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ui/views/mus/window_manager_connection.cc [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ui/views/mus/window_manager_connection_unittest.cc [modify] https://crrev.com/e27b07278681ba98ec911b145968315b5eb63440/ui/views/pointer_watcher.h
,
May 12 2016
,
May 19 2016
It now opens and closes and doesn't crash. I suspect there may be drag and drop issues, but we can track that in another bug.
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1723281f4514b58fe1a1ef17c66a92e79517827c commit 1723281f4514b58fe1a1ef17c66a92e79517827c Author: James Cook <jamescook@chromium.org> Date: Thu Aug 23 19:48:28 2018 chromeos: Convert OverflowBubble to ui::EventHandler Now that ash and the ui service are in the same process we don't need to use PointerWatcher in ash anymore. This is a partial manual revert of: https://codereview.chromium.org/1934123004/ Bug: 595851 , 872450 Test: ash_unittests Change-Id: Ifbbb407210d0b3ec8d20993bff7a0f2cf44ba38e Reviewed-on: https://chromium-review.googlesource.com/1186032 Commit-Queue: James Cook <jamescook@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#585583} [modify] https://crrev.com/1723281f4514b58fe1a1ef17c66a92e79517827c/ash/shelf/overflow_bubble.cc [modify] https://crrev.com/1723281f4514b58fe1a1ef17c66a92e79517827c/ash/shelf/overflow_bubble.h |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by jamescook@chromium.org
, Apr 28 2016Status: Assigned (was: Untriaged)