Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
294023002:
aura: Updates the text input client when native activation changes. (Closed)
|
Created:
6 years, 7 months ago by Yuki Modified:
6 years, 6 months ago CC:
chromium-reviews, tfarina, ben+views_chromium.org, tdanderson+views_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionaura: Updates the text input client when native activation changes.
It turned out that we should update the focused text input client not only when native focus/blur but also when native activate/deactivate.
See crbug.com/377479 for details.
Also this CL fixes the initial text input focus on CrOS login screen.
BUG=323376, 290701
TEST=Confirmed that the Omnibox gets the text input focus by default when a new window is created.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276437
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixed CrOS login screen. #
Total comments: 2
Patch Set 3 : Added two tests. #Patch Set 4 : Synced. #
Total comments: 6
Patch Set 5 : Synced. #Patch Set 6 : Addressed sky's comments. #
Total comments: 8
Patch Set 7 : Synced. #Patch Set 8 : Addressed msw's comments. #Patch Set 9 : Synced. #Patch Set 10 : Added a TODO comment. #
Messages
Total messages: 24 (0 generated)
sky@, could you review this CL? This CL is complement of https://codereview.chromium.org/173803002/
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne... ui/views/widget/widget.cc:1072: views::FocusManager* focus_manager = GetFocusManager(); Your comments make me thing it would be better to do this from Init. Would that make more sense?
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne... ui/views/widget/widget.cc:1072: views::FocusManager* focus_manager = GetFocusManager(); On 2014/05/20 13:47:15, sky wrote: > Your comments make me thing it would be better to do this from Init. Would that > make more sense? FocusTextInputClient() requires the widget to be active in order to make an actual effect. If the widget is not active, FocusTextInputClient() does nothing because only the active widget's text input client should get focused. The problem is that the widget is not active yet when Widget::Init is called. So we have to wait that the widget gets activated.
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne... ui/views/widget/widget.cc:1051: // OnNativeWidgetActivationChanged is a little confusing. At the time of NativeWidgetAura::OnWindowActivated calls this function and it also stores and restores the focused view, but *DesktopNativeWidgetAura::OnWindowActivated* oddly doesn't call this, instead *DesktopNativeWidgetAura::HandleActivationChanged* calls this, and it has a modified view focusing logic with no view storing logic... This is pretty complicated, but does the bug you're fixing only repro on desktop platforms (Win/Linux), not CrOS/Metro/Ash? Perhaps the defect is actually in the order of activation/focus operations, as you elude to in this comment? https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne... ui/views/widget/widget.cc:1071: // focused text input here in addition to OnNativeFocus and OnNativeBlur. nit: This comment is no longer accurate. I removed the OnNativeFocus and OnNativeBlur TextInputClient updates in r271312, because [Desktop]NativeWidgetAura::OnWindowFocused now stores and restores View focus itself, which should trigger the desired TextInputClient changes.
I'll address on the rest part of comments later. https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne... ui/views/widget/widget.cc:1051: // OnNativeWidgetActivationChanged is a little confusing. At the time of On 2014/05/20 17:08:07, msw wrote: > NativeWidgetAura::OnWindowActivated calls this function and it also stores and > restores the focused view, but *DesktopNativeWidgetAura::OnWindowActivated* > oddly doesn't call this, instead > *DesktopNativeWidgetAura::HandleActivationChanged* calls this, and it has a > modified view focusing logic with no view storing logic... This is pretty > complicated, but does the bug you're fixing only repro on desktop platforms > (Win/Linux), not CrOS/Metro/Ash? Perhaps the defect is actually in the order of > activation/focus operations, as you elude to in this comment? The issue is happening on Win, Linux and CrOS as I tested. Probably on all Aura platforms. Win, Linux: After window creation, Omnibox doesn't get the text input focus. CrOS: After the boot-up, sign-in screen (password field) doesn't get the text input focus.
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne... ui/views/widget/widget.cc:1051: // OnNativeWidgetActivationChanged is a little confusing. At the time of On 2014/05/21 14:36:51, Yuki wrote: > On 2014/05/20 17:08:07, msw wrote: > > NativeWidgetAura::OnWindowActivated calls this function and it also stores and > > restores the focused view, but *DesktopNativeWidgetAura::OnWindowActivated* > > oddly doesn't call this, instead > > *DesktopNativeWidgetAura::HandleActivationChanged* calls this, and it has a > > modified view focusing logic with no view storing logic... This is pretty > > complicated, but does the bug you're fixing only repro on desktop platforms > > (Win/Linux), not CrOS/Metro/Ash? Perhaps the defect is actually in the order > of > > activation/focus operations, as you elude to in this comment? > > > The issue is happening on Win, Linux and CrOS as I tested. Probably on all Aura > platforms. > > Win, Linux: After window creation, Omnibox doesn't get the text input focus. > CrOS: After the boot-up, sign-in screen (password field) doesn't get the text > input focus. Okay, then you can ignore my question, but the long comment here seems potentially indicative of a bigger problem than what you're solving by adding new [Focus|Blur]TextInputClient calls. Perhaps we'd get a better headway on the underlying defect by filing this commentary as a bug, rather than adding a big block of text here? https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne... ui/views/widget/widget.cc:1077: focus_manager->BlurTextInputClient(focus_manager->GetFocusedView()); Is this actually needed to fix the issue? Perhaps you just added it for completeness/parity?
It seems that removing calls to {Focus,Blur}TextInputClient from
Widget::OnNative{Focus,Blur} made a trouble about the initial focus on CrOS.
Or, it revealed a problem of my redesign. I'm now investigating CrOS behaviors
more. Let me address your comments after this investigation.
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne...
ui/views/widget/widget.cc:1077:
focus_manager->BlurTextInputClient(focus_manager->GetFocusedView());
On 2014/05/22 04:53:01, msw wrote:
> Is this actually needed to fix the issue? Perhaps you just added it for
> completeness/parity?
This is just for parity and completeness.
Finally I figured out two more issues, and fixed them in this CL. Could you
take another look?
Changes in c/b/chromeos/... fix the initial text input focus on CrOS login
screen.
Change in focus_manager.cc fixes msw's change that removed calls to
{Focus,Blur}TextInputClient from Widget::Native{Focus,Blur}. Unlike direct call
to {Focus,Blur}TextInputClient, storing/restoring focused view does not always
make call to FocusTextInputClient.
msw@, please let me know if I missed some of your comments.
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne...
ui/views/widget/widget.cc:1051: // OnNativeWidgetActivationChanged is a little
confusing. At the time of
On 2014/05/22 04:53:01, msw wrote:
> On 2014/05/21 14:36:51, Yuki wrote:
> > On 2014/05/20 17:08:07, msw wrote:
> > > NativeWidgetAura::OnWindowActivated calls this function and it also stores
> and
> > > restores the focused view, but
*DesktopNativeWidgetAura::OnWindowActivated*
> > > oddly doesn't call this, instead
> > > *DesktopNativeWidgetAura::HandleActivationChanged* calls this, and it has
a
> > > modified view focusing logic with no view storing logic... This is pretty
> > > complicated, but does the bug you're fixing only repro on desktop
platforms
> > > (Win/Linux), not CrOS/Metro/Ash? Perhaps the defect is actually in the
order
> > of
> > > activation/focus operations, as you elude to in this comment?
> >
> >
> > The issue is happening on Win, Linux and CrOS as I tested. Probably on all
> Aura
> > platforms.
> >
> > Win, Linux: After window creation, Omnibox doesn't get the text input focus.
> > CrOS: After the boot-up, sign-in screen (password field) doesn't get the
text
> > input focus.
>
> Okay, then you can ignore my question, but the long comment here seems
> potentially indicative of a bigger problem than what you're solving by adding
> new [Focus|Blur]TextInputClient calls. Perhaps we'd get a better headway on
the
> underlying defect by filing this commentary as a bug, rather than adding a big
> block of text here?
Done.
https://codereview.chromium.org/294023002/diff/1/ui/views/widget/widget.cc#ne...
ui/views/widget/widget.cc:1071: // focused text input here in addition to
OnNativeFocus and OnNativeBlur.
On 2014/05/20 17:08:07, msw wrote:
> nit: This comment is no longer accurate. I removed the OnNativeFocus and
> OnNativeBlur TextInputClient updates in r271312, because
> [Desktop]NativeWidgetAura::OnWindowFocused now stores and restores View focus
> itself, which should trigger the desired TextInputClient changes.
Done.
Please add test coverage too. https://codereview.chromium.org/294023002/diff/20001/ui/views/focus/focus_man... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/294023002/diff/20001/ui/views/focus/focus_man... ui/views/focus/focus_manager.cc:319: FocusTextInputClient(focused_view_); We also can end up here if View::RequestFocus() is invoked and the view already has focus. Does anything bad happen in that case?
Will add unittests. https://codereview.chromium.org/294023002/diff/20001/ui/views/focus/focus_man... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/294023002/diff/20001/ui/views/focus/focus_man... ui/views/focus/focus_manager.cc:319: FocusTextInputClient(focused_view_); On 2014/05/27 17:05:44, sky wrote: > We also can end up here if View::RequestFocus() is invoked and the view already > has focus. Does anything bad happen in that case? No, nothing bad happen. If the widget is active, |focused_view_->GetTextInputClient()| must already be set and the same text input client is going to be set again. It's actually no-op. If the widget is not active, we do nothing.
I've added two test cases. I've been struggling to add a test case for chromeos login screen, and it seems not easy. Can I commit this CL first, and later add a test case for cros login screen?
LGTM https://codereview.chromium.org/294023002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/294023002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:333: chrome::FocusLocationBar(browser()); This is done in SetUp, so it shouldn't be necessary here too. https://codereview.chromium.org/294023002/diff/60001/ui/views/focus/focus_man... File ui/views/focus/focus_manager_unittest.cc (right): https://codereview.chromium.org/294023002/diff/60001/ui/views/focus/focus_man... ui/views/focus/focus_manager_unittest.cc:779: }; DISALLOW_... https://codereview.chromium.org/294023002/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/294023002/diff/60001/ui/views/widget/widget.c... ui/views/widget/widget.cc:1034: // We have to update the focused text input client when the active widget This just describes the code, document why this is needed.
msw@, could you take another look at this CL? https://codereview.chromium.org/294023002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right): https://codereview.chromium.org/294023002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:333: chrome::FocusLocationBar(browser()); On 2014/05/30 16:30:15, sky wrote: > This is done in SetUp, so it shouldn't be necessary here too. This is done in SetUpOnMainThread without kEnableTextInputFocusManager flag enabled, so I'd like to test it with the flag enabled here. I didn't want to affect other tests with the flag, so I wanted to enable the flag only within this test fixture. Added a TODO comment to remove this call once the transition to TextInputFocusManager completes. https://codereview.chromium.org/294023002/diff/60001/ui/views/focus/focus_man... File ui/views/focus/focus_manager_unittest.cc (right): https://codereview.chromium.org/294023002/diff/60001/ui/views/focus/focus_man... ui/views/focus/focus_manager_unittest.cc:779: }; On 2014/05/30 16:30:15, sky wrote: > DISALLOW_... Done. https://codereview.chromium.org/294023002/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/294023002/diff/60001/ui/views/widget/widget.c... ui/views/widget/widget.cc:1034: // We have to update the focused text input client when the active widget On 2014/05/30 16:30:15, sky wrote: > This just describes the code, document why this is needed. Done.
https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:318: // changing the focused view, we have to make the text input client focused. When does the widget lose and regain focus without saving and restoring the focused view? That seems like the root defect preventing the [Blur|Focus]TextInputClient calls below to trigger as expected. You said separately "Unlike direct call to {Focus,Blur}TextInputClient, storing/restoring focused view does not always make call to FocusTextInputClient." Is that truly what's happening (different from this comment here), and if so why? That also seems like a more fundamental defect. https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager_unittest.cc (left): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager_unittest.cc:769: GetFocusManager()->StoreFocusedView(false); nit: EXPECT_EQ(NULL, GetFocusManager()->GetFocusedView()) after both StoreFocusedView calls. https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager_unittest.cc (right): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager_unittest.cc:787: TEST_F(FocusManagerTest, StoreFocusedView) { Make a separate test for TextInputClient checks, this was a simple test.
Sorry for the late response. I got sick last week, but now recovered. Could you take another look? We may need careful refactoring about what should be done in activation changes and what should be done in focus changes, including the order of calls. https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:318: // changing the focused view, we have to make the text input client focused. On 2014/06/02 17:17:04, msw wrote: > When does the widget lose and regain focus without saving and restoring the > focused view? That seems like the root defect preventing the > [Blur|Focus]TextInputClient calls below to trigger as expected. > > You said separately "Unlike direct call to {Focus,Blur}TextInputClient, > storing/restoring focused view does not always make call to > FocusTextInputClient." Is that truly what's happening (different from this > comment here), and if so why? That also seems like a more fundamental defect. Okay, I found the cause. Here is the scenario. Suppose you have two windows and move the focus from one to the other one. 1. [activation change] NativeWidget's OnWindowActivated calls RestoreFocusedView. At this point, the focused view changes and the focused text input client, too. 2. [focus change] FocusController resets the text input client first of all. After that, NativeWidget's OnWindowFocused calls RestoreFocusedView. Since the focused view doesn't change from the last time, we return at this line immediately, and the focused text input client is left as null. This is the scenario we're facing. I think we cannot simply remove the call to RestoreFocusedView from OnWindowActivated, and as I wrote before the order of activation/focus is wrong at window creation time. What do you think? Can we go with this way or any better ideas? https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager_unittest.cc (left): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager_unittest.cc:769: GetFocusManager()->StoreFocusedView(false); On 2014/06/02 17:17:04, msw wrote: > nit: EXPECT_EQ(NULL, GetFocusManager()->GetFocusedView()) after both > StoreFocusedView calls. Done. https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager_unittest.cc (right): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager_unittest.cc:787: TEST_F(FocusManagerTest, StoreFocusedView) { On 2014/06/02 17:17:04, msw wrote: > Make a separate test for TextInputClient checks, this was a simple test. Done.
Please consider my suggestion, but I wouldn't really oppose landing your CL as-is, so long as the new tests will prevent regressions of the intended text input client focus. We can try to simplify the text input client focus/blur code later, but the more immediate goal is to get it working as intended. Please make sure that your tests fail without the behavioral changes of this CL. https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:318: // changing the focused view, we have to make the text input client focused. On 2014/06/08 11:08:16, Yuki wrote: > On 2014/06/02 17:17:04, msw wrote: > > When does the widget lose and regain focus without saving and restoring the > > focused view? That seems like the root defect preventing the > > [Blur|Focus]TextInputClient calls below to trigger as expected. > > > > You said separately "Unlike direct call to {Focus,Blur}TextInputClient, > > storing/restoring focused view does not always make call to > > FocusTextInputClient." Is that truly what's happening (different from this > > comment here), and if so why? That also seems like a more fundamental defect. > > Okay, I found the cause. Here is the scenario. Suppose you have two windows > and move the focus from one to the other one. > > 1. [activation change] NativeWidget's OnWindowActivated calls > RestoreFocusedView. At this point, the focused view changes and the focused > text input client, too. > 2. [focus change] FocusController resets the text input client first of all. > After that, NativeWidget's OnWindowFocused calls RestoreFocusedView. Since the > focused view doesn't change from the last time, we return at this line > immediately, and the focused text input client is left as null. Hmm, maybe FocusController::SetFocusedWindow should not call FocusTextInputClient(NULL) when a valid |window| is focused. Does removing that call fix this defect without causing other issues?
msw@, thank you for your review. If you're okay, I'll commit this CL as is for this moment, and will enable the --enable-text-input-focus-manager flag by default. I checked a new test in FocusManager fails without the change, and passes with the change. https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:318: // changing the focused view, we have to make the text input client focused. On 2014/06/10 04:05:08, msw wrote: > On 2014/06/08 11:08:16, Yuki wrote: > > On 2014/06/02 17:17:04, msw wrote: > > > When does the widget lose and regain focus without saving and restoring the > > > focused view? That seems like the root defect preventing the > > > [Blur|Focus]TextInputClient calls below to trigger as expected. > > > > > > You said separately "Unlike direct call to {Focus,Blur}TextInputClient, > > > storing/restoring focused view does not always make call to > > > FocusTextInputClient." Is that truly what's happening (different from this > > > comment here), and if so why? That also seems like a more fundamental > defect. > > > > Okay, I found the cause. Here is the scenario. Suppose you have two windows > > and move the focus from one to the other one. > > > > 1. [activation change] NativeWidget's OnWindowActivated calls > > RestoreFocusedView. At this point, the focused view changes and the focused > > text input client, too. > > 2. [focus change] FocusController resets the text input client first of all. > > After that, NativeWidget's OnWindowFocused calls RestoreFocusedView. Since > the > > focused view doesn't change from the last time, we return at this line > > immediately, and the focused text input client is left as null. > > Hmm, maybe FocusController::SetFocusedWindow should not call > FocusTextInputClient(NULL) when a valid |window| is focused. Does removing that > call fix this defect without causing other issues? I understand that the reason why sky@ recommended to call FocusTextInputClient(NULL) in FocusController is to guarantee that an invalid (or already-died) text input client never gets focused. From a point of view of wm::FocusController, even when |window| is valid, it's not guaranteed that the |window| sets the focus to the correct text input client. Maybe it could be forgotten. That's the reason why sky@ highly recommended to call FocusTextInputClient(NULL) here. So, simply removing the call looks a bad idea to me. It's |window|'s duty to set the correct text input client focused, I think.
On 2014/06/10 05:59:00, Yuki wrote: > msw@, thank you for your review. If you're okay, I'll commit this CL as is for > this moment, and will enable the --enable-text-input-focus-manager flag by > default. > > I checked a new test in FocusManager fails without the change, and passes with > the change. > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > File ui/views/focus/focus_manager.cc (right): > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > ui/views/focus/focus_manager.cc:318: // changing the focused view, we have to > make the text input client focused. > On 2014/06/10 04:05:08, msw wrote: > > On 2014/06/08 11:08:16, Yuki wrote: > > > On 2014/06/02 17:17:04, msw wrote: > > > > When does the widget lose and regain focus without saving and restoring > the > > > > focused view? That seems like the root defect preventing the > > > > [Blur|Focus]TextInputClient calls below to trigger as expected. > > > > > > > > You said separately "Unlike direct call to {Focus,Blur}TextInputClient, > > > > storing/restoring focused view does not always make call to > > > > FocusTextInputClient." Is that truly what's happening (different from this > > > > comment here), and if so why? That also seems like a more fundamental > > defect. > > > > > > Okay, I found the cause. Here is the scenario. Suppose you have two > windows > > > and move the focus from one to the other one. > > > > > > 1. [activation change] NativeWidget's OnWindowActivated calls > > > RestoreFocusedView. At this point, the focused view changes and the focused > > > text input client, too. > > > 2. [focus change] FocusController resets the text input client first of all. > > > > After that, NativeWidget's OnWindowFocused calls RestoreFocusedView. Since > > the > > > focused view doesn't change from the last time, we return at this line > > > immediately, and the focused text input client is left as null. > > > > Hmm, maybe FocusController::SetFocusedWindow should not call > > FocusTextInputClient(NULL) when a valid |window| is focused. Does removing > that > > call fix this defect without causing other issues? > > I understand that the reason why sky@ recommended to call > FocusTextInputClient(NULL) in FocusController is to guarantee that an invalid > (or already-died) text input client never gets focused. From a point of view of > wm::FocusController, even when |window| is valid, it's not guaranteed that the > |window| sets the focus to the correct text input client. Maybe it could be > forgotten. That's the reason why sky@ highly recommended to call > FocusTextInputClient(NULL) here. > > So, simply removing the call looks a bad idea to me. It's |window|'s duty to > set the correct text input client focused, I think. Actually, I believe the intent was to call FocusTextInputClient with |window|'s focused TextInputClient, not NULL. Otherwise, we're senselessly clobbering the TextInputClient focused just prior by the aura::Window's Widget/NativeWidget. If FocusController doesn't know what the currently focused TextInputClient is, it probably shouldn't stomp the values set by objects that do. Scott said "All this code should work regardless of using views or not. Changing the active aura::Window should reset the textinputclient just as changing the focused view does." <https://codereview.chromium.org/173803002/#msg33>
On 2014/06/10 06:38:26, msw wrote: > On 2014/06/10 05:59:00, Yuki wrote: > > msw@, thank you for your review. If you're okay, I'll commit this CL as is > for > > this moment, and will enable the --enable-text-input-focus-manager flag by > > default. > > > > I checked a new test in FocusManager fails without the change, and passes with > > the change. > > > > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > > File ui/views/focus/focus_manager.cc (right): > > > > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > > ui/views/focus/focus_manager.cc:318: // changing the focused view, we have to > > make the text input client focused. > > On 2014/06/10 04:05:08, msw wrote: > > > On 2014/06/08 11:08:16, Yuki wrote: > > > > On 2014/06/02 17:17:04, msw wrote: > > > > > When does the widget lose and regain focus without saving and restoring > > the > > > > > focused view? That seems like the root defect preventing the > > > > > [Blur|Focus]TextInputClient calls below to trigger as expected. > > > > > > > > > > You said separately "Unlike direct call to {Focus,Blur}TextInputClient, > > > > > storing/restoring focused view does not always make call to > > > > > FocusTextInputClient." Is that truly what's happening (different from > this > > > > > comment here), and if so why? That also seems like a more fundamental > > > defect. > > > > > > > > Okay, I found the cause. Here is the scenario. Suppose you have two > > windows > > > > and move the focus from one to the other one. > > > > > > > > 1. [activation change] NativeWidget's OnWindowActivated calls > > > > RestoreFocusedView. At this point, the focused view changes and the > focused > > > > text input client, too. > > > > 2. [focus change] FocusController resets the text input client first of > all. > > > > > > After that, NativeWidget's OnWindowFocused calls RestoreFocusedView. > Since > > > the > > > > focused view doesn't change from the last time, we return at this line > > > > immediately, and the focused text input client is left as null. > > > > > > Hmm, maybe FocusController::SetFocusedWindow should not call > > > FocusTextInputClient(NULL) when a valid |window| is focused. Does removing > > that > > > call fix this defect without causing other issues? > > > > I understand that the reason why sky@ recommended to call > > FocusTextInputClient(NULL) in FocusController is to guarantee that an invalid > > (or already-died) text input client never gets focused. From a point of view > of > > wm::FocusController, even when |window| is valid, it's not guaranteed that the > > |window| sets the focus to the correct text input client. Maybe it could be > > forgotten. That's the reason why sky@ highly recommended to call > > FocusTextInputClient(NULL) here. > > > > So, simply removing the call looks a bad idea to me. It's |window|'s duty to > > set the correct text input client focused, I think. > > Actually, I believe the intent was to call FocusTextInputClient with |window|'s > focused TextInputClient, not NULL. Otherwise, we're senselessly clobbering the > TextInputClient focused just prior by the aura::Window's Widget/NativeWidget. If > FocusController doesn't know what the currently focused TextInputClient is, it > probably shouldn't stomp the values set by objects that do. Scott said "All this > code should work regardless of using views or not. Changing the active > aura::Window should reset the textinputclient just as changing the focused view > does." <https://codereview.chromium.org/173803002/#msg33> Agreed. Ideally, we should have aura::Window::GetTextInputClient (and probably Widget::GetTextInputClient, etc?), and this function should returns the text input client to be focused. Then, things will be simpler. Let me revisit this point later. Maybe I need to update the design doc (or create another).
On 2014/06/10 06:51:51, Yuki wrote: > On 2014/06/10 06:38:26, msw wrote: > > On 2014/06/10 05:59:00, Yuki wrote: > > > msw@, thank you for your review. If you're okay, I'll commit this CL as is > > for > > > this moment, and will enable the --enable-text-input-focus-manager flag by > > > default. > > > > > > I checked a new test in FocusManager fails without the change, and passes > with > > > the change. > > > > > > > > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > > > File ui/views/focus/focus_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > > > ui/views/focus/focus_manager.cc:318: // changing the focused view, we have > to > > > make the text input client focused. > > > On 2014/06/10 04:05:08, msw wrote: > > > > On 2014/06/08 11:08:16, Yuki wrote: > > > > > On 2014/06/02 17:17:04, msw wrote: > > > > > > When does the widget lose and regain focus without saving and > restoring > > > the > > > > > > focused view? That seems like the root defect preventing the > > > > > > [Blur|Focus]TextInputClient calls below to trigger as expected. > > > > > > > > > > > > You said separately "Unlike direct call to > {Focus,Blur}TextInputClient, > > > > > > storing/restoring focused view does not always make call to > > > > > > FocusTextInputClient." Is that truly what's happening (different from > > this > > > > > > comment here), and if so why? That also seems like a more fundamental > > > > defect. > > > > > > > > > > Okay, I found the cause. Here is the scenario. Suppose you have two > > > windows > > > > > and move the focus from one to the other one. > > > > > > > > > > 1. [activation change] NativeWidget's OnWindowActivated calls > > > > > RestoreFocusedView. At this point, the focused view changes and the > > focused > > > > > text input client, too. > > > > > 2. [focus change] FocusController resets the text input client first of > > all. > > > > > > > > After that, NativeWidget's OnWindowFocused calls RestoreFocusedView. > > Since > > > > the > > > > > focused view doesn't change from the last time, we return at this line > > > > > immediately, and the focused text input client is left as null. > > > > > > > > Hmm, maybe FocusController::SetFocusedWindow should not call > > > > FocusTextInputClient(NULL) when a valid |window| is focused. Does removing > > > that > > > > call fix this defect without causing other issues? > > > > > > I understand that the reason why sky@ recommended to call > > > FocusTextInputClient(NULL) in FocusController is to guarantee that an > invalid > > > (or already-died) text input client never gets focused. From a point of > view > > of > > > wm::FocusController, even when |window| is valid, it's not guaranteed that > the > > > |window| sets the focus to the correct text input client. Maybe it could be > > > forgotten. That's the reason why sky@ highly recommended to call > > > FocusTextInputClient(NULL) here. > > > > > > So, simply removing the call looks a bad idea to me. It's |window|'s duty > to > > > set the correct text input client focused, I think. > > > > Actually, I believe the intent was to call FocusTextInputClient with > |window|'s > > focused TextInputClient, not NULL. Otherwise, we're senselessly clobbering the > > TextInputClient focused just prior by the aura::Window's Widget/NativeWidget. > If > > FocusController doesn't know what the currently focused TextInputClient is, it > > probably shouldn't stomp the values set by objects that do. Scott said "All > this > > code should work regardless of using views or not. Changing the active > > aura::Window should reset the textinputclient just as changing the focused > view > > does." <https://codereview.chromium.org/173803002/#msg33> > > Agreed. Ideally, we should have aura::Window::GetTextInputClient (and probably > Widget::GetTextInputClient, etc?), and this function should returns the text > input client to be focused. Then, things will be simpler. > > Let me revisit this point later. Maybe I need to update the design doc (or > create another). Please add TODOs and/or file a bug, the current behavior is clearly wrong. I suppose this LGTM if we want a workaround fix.
On 2014/06/10 17:06:47, msw wrote: > On 2014/06/10 06:51:51, Yuki wrote: > > On 2014/06/10 06:38:26, msw wrote: > > > On 2014/06/10 05:59:00, Yuki wrote: > > > > msw@, thank you for your review. If you're okay, I'll commit this CL as > is > > > for > > > > this moment, and will enable the --enable-text-input-focus-manager flag by > > > > default. > > > > > > > > I checked a new test in FocusManager fails without the change, and passes > > with > > > > the change. > > > > > > > > > > > > > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > > > > File ui/views/focus/focus_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/294023002/diff/100001/ui/views/focus/focus_ma... > > > > ui/views/focus/focus_manager.cc:318: // changing the focused view, we have > > to > > > > make the text input client focused. > > > > On 2014/06/10 04:05:08, msw wrote: > > > > > On 2014/06/08 11:08:16, Yuki wrote: > > > > > > On 2014/06/02 17:17:04, msw wrote: > > > > > > > When does the widget lose and regain focus without saving and > > restoring > > > > the > > > > > > > focused view? That seems like the root defect preventing the > > > > > > > [Blur|Focus]TextInputClient calls below to trigger as expected. > > > > > > > > > > > > > > You said separately "Unlike direct call to > > {Focus,Blur}TextInputClient, > > > > > > > storing/restoring focused view does not always make call to > > > > > > > FocusTextInputClient." Is that truly what's happening (different > from > > > this > > > > > > > comment here), and if so why? That also seems like a more > fundamental > > > > > defect. > > > > > > > > > > > > Okay, I found the cause. Here is the scenario. Suppose you have two > > > > windows > > > > > > and move the focus from one to the other one. > > > > > > > > > > > > 1. [activation change] NativeWidget's OnWindowActivated calls > > > > > > RestoreFocusedView. At this point, the focused view changes and the > > > focused > > > > > > text input client, too. > > > > > > 2. [focus change] FocusController resets the text input client first > of > > > all. > > > > > > > > > > After that, NativeWidget's OnWindowFocused calls RestoreFocusedView. > > > Since > > > > > the > > > > > > focused view doesn't change from the last time, we return at this line > > > > > > immediately, and the focused text input client is left as null. > > > > > > > > > > Hmm, maybe FocusController::SetFocusedWindow should not call > > > > > FocusTextInputClient(NULL) when a valid |window| is focused. Does > removing > > > > that > > > > > call fix this defect without causing other issues? > > > > > > > > I understand that the reason why sky@ recommended to call > > > > FocusTextInputClient(NULL) in FocusController is to guarantee that an > > invalid > > > > (or already-died) text input client never gets focused. From a point of > > view > > > of > > > > wm::FocusController, even when |window| is valid, it's not guaranteed that > > the > > > > |window| sets the focus to the correct text input client. Maybe it could > be > > > > forgotten. That's the reason why sky@ highly recommended to call > > > > FocusTextInputClient(NULL) here. > > > > > > > > So, simply removing the call looks a bad idea to me. It's |window|'s duty > > to > > > > set the correct text input client focused, I think. > > > > > > Actually, I believe the intent was to call FocusTextInputClient with > > |window|'s > > > focused TextInputClient, not NULL. Otherwise, we're senselessly clobbering > the > > > TextInputClient focused just prior by the aura::Window's > Widget/NativeWidget. > > If > > > FocusController doesn't know what the currently focused TextInputClient is, > it > > > probably shouldn't stomp the values set by objects that do. Scott said "All > > this > > > code should work regardless of using views or not. Changing the active > > > aura::Window should reset the textinputclient just as changing the focused > > view > > > does." <https://codereview.chromium.org/173803002/#msg33> > > > > Agreed. Ideally, we should have aura::Window::GetTextInputClient (and > probably > > Widget::GetTextInputClient, etc?), and this function should returns the text > > input client to be focused. Then, things will be simpler. > > > > Let me revisit this point later. Maybe I need to update the design doc (or > > create another). > > Please add TODOs and/or file a bug, the current behavior is clearly wrong. > I suppose this LGTM if we want a workaround fix. I've added a TODO comment and also filed a bug: http://crbug.com/383236
The CQ bit was checked by yukishiino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/294023002/180001
Message was sent while issue was closed.
Change committed as 276437 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
