tabstrip -> sdy

The issue seems to be that FocusManager::SetFocusedViewWithReason() gets called more than once during a drag operation on the tab strip. The first time, no focus manager instance exists and all is well.

The second time around, because a focus manager instance now exists, FocusManager::ClearNativeFocus() ends up being called. Then, when FocusManager::RestoreFocus() tries to restore focus to the tab strip, it finds that the tab strip is not focusable and advances the focus to the next focusable element, the omnibox.

Not repro on Material Refresh.

This is focusing the tab close button for me (with and without Refresh).

Deleted: Untitled.mov 987 KB

	Untitled.mov 987 KB View Download

I think the underlying issue still exists on Material Refresh, it's just that the "next focusable element" after the tab strip is the tab close button, which is only focusable in certain accessibility modes. So if the tab close button is not focusable on a certain machine, it appears to be fixed, but it isn't.

Also I have Material Refresh on and the bug is still occuring with the omnibox focusing for me.

robliao@: I still can repro it.

Please let me know if you need more information. Thanks.

The issue seems to be that NativeWidgetMac::ClearNativeFocus() ends up calling FocusManager::RestoreFocusedView(). Does NativeWidgetAura::ClearNativeFocus() call FocusManager::RestoreFocusedView()? And if not, how best can NativeWidgetMac::ClearNativeFocus() be modified to avoid calling FocusManager::RestoreFocusedView()?

#15:

It does not *look* like it does - it looks like ::RestoreFocusedView() is only called in Aura when windows become active. There's a puzzling comment at the top of NativeWidgetMac::ClearNativeFocus:

void NativeWidgetMac::ClearNativeFocus() {
  // To quote DesktopWindowTreeHostX11, "This method is weird and misnamed."
  // The goal is to set focus to the content window, thereby removing focus from
  // any NSView in the window that doesn't belong to toolkit-views.
  [GetNativeWindow() makeFirstResponder:GetNativeView()];
}

This doesn't seem to match what the NativeWidgetAura implementation does.

tapted@, do you know what the deal is?

NativeWidgetAura calls ResetFocusWithinActiveWindow(content_window) which ends up in wm::FocusController::SetFocusedWindow(content_window) - so I think it is similar.


HWNDMessageHandler::ClearNativeFocus() is `::SetFocus(hwnd());`. makeFirstResponder is about as close as we can get to that.

but... on Windows that calls back into

 void DesktopWindowTreeHostWin::HandleNativeFocus(HWND last_focused_window) {
   // TODO(beng): inform the native_widget_delegate_.
 }

interesting?

On X11, focus changes (x11::FocusIn?) go to  DesktopWindowTreeHostX11::OnFocusEvent and to  DesktopWindowTreeHostX11::AfterActivationStateChanged() which calls DesktopNativeWidgetAura::HandleActivationChanged(bool active), and that's where there is a call to focus_manager::RestoreFocusedView().

So I don't think what we're doing on Mac is peculiar.


I suspect what's actually happening is that other platforms focus the tab strip button of the tab being dragged, but on Mac we say "we're not in a11y mode, so don't focus buttons -> advance focus to omnibox"

This is the same bug basically as  issue 853687 , except that  issue 853687  happens with FKA turned on in MD refresh mode. I think all the Views focus save/restore logic is working as designed here, and the real bug is that the tab close buttons are receiving keyboard focus during a drag.

Interestingly, in the Cocoa browser, the omnibox is first responder during a tab drag as well - but none of the key events actually arrive until after the drag is complete because the drag happens in a nested runloop.

I think that the *proper behavior* is the listed current behavior in this bug - i.e., that the omnibox should get keyboard focus during a drag. That's how the Cocoa browser behaves and it seems not unreasonable to me. The tab close buttons getting focus is a bug. Another option might be for the web contents to retain keyboard focus, but I think that would be quite difficult :(.

Actually, no, my post in #18 is incorrect - the Cocoa behavior is that the web contents retains first responder during a drag, but does not handle any input until the drag is done. Hm.

Okay, I think the problem is here:

  void TabDragController::SaveFocus() {
    DCHECK(source_tabstrip_);
    old_focused_view_tracker_->SetView(
        source_tabstrip_->GetFocusManager()->GetFocusedView());
    source_tabstrip_->GetFocusManager()->SetFocusedView(source_tabstrip_);
    // WARNING: we may have been deleted.
  }

If I remove the call to SetFocusedView here, focus remains in the webcontents during the drag, and keyboard input/etc seems to work normally - in fact, you can even cmd-l during the drag and start typing in the omnibox.

Perhaps not calling SetFocusedView there is workable? What do you think, sky@? You last touched this logic in 2011 :)

 Issue 853687  has been merged into this issue.

The only issue I have with removing the call to SetFocusedView() is that I'm pretty sure we do want to unfocus whatever is focused in the current frame (e.g. if a textfield is focused prior to drag, it should be unfocused during the drag).

Maybe something like replacing the call to SetFocusedView() with a call to FocusManager::ClearFocus() could work? It removes the focus during drag without focusing on something undesired (the omnibox) and then the focus is restored after the drag.

#22: I'd be fine with focus remaining in the textfield during the drag, actually - but ::ClearFocus() is fine with me too. Your call :)

The tabstrip has capture (it needs to get the mouse events), so it stands that it should get focus too. To do otherwise could lead to another view getting keyevents that could result in unexpected changes. If you clear focus, make sure escape still works to cancel drag. I think it should, but I would make sure you don't break it.

Checked, escape still works to cancel drag with ClearFocus() :)

 Issue 856555  has been merged into this issue.

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

commit e8975862b8bd4e071e624f02d4fe633336ef0eb0
Author: Tessa Nijssen <tnijssen@google.com>
Date: Mon Jul 16 18:46:43 2018

[Views] Clear Focus Instead of Focusing Tab Strip on Tab Drag

When a tab is being dragged, the current instance of TabDragController
calls FocusManager::SetFocusedView()  and passes the tab strip that is
the source of the drag. Since the tab strip is not focusable, the
focus is moved to the next focusable element. In MacViews (when
keyboard accessibility is not turned on), this is the omnibox,
resulting in the omnibox being focused during a drag.

Instead of calling SetFocusedView(), FocusManager::ClearFocus() should
be called. This way, whichever element is currently focused will lose
focus during a drag, as is desired. However, no new element will be
focused, so there is no chance that an undesired focus occurs. When the
drag finishes, TabDragController restores focus to the previously
focused element as it previously did.

On ClearFocus(), the window associated with the top root view gets
focus, so key events are still forwarded properly and hitting Esc
still cancels the drag in progress.

The interactive UI test was slightly modified to check that the window
of the attached tab strip has focus rather than the tab strip itself
to reflect the new behavior.

Bug:  835550 
Change-Id: Iff846e24e31428737ae204dd9e68f2f57ee83bad
Reviewed-on: https://chromium-review.googlesource.com/1132133
Commit-Queue: Tessa Nijssen <tnijssen@google.com>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575368}
[modify] https://crrev.com/e8975862b8bd4e071e624f02d4fe633336ef0eb0/chrome/browser/ui/views/tabs/tab_drag_controller.cc
[modify] https://crrev.com/e8975862b8bd4e071e624f02d4fe633336ef0eb0/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc