Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2371113003:
Do not give instant focus if a view's toplevelwidget is not active (Closed)
|
Created:
4 years, 2 months ago by Qiang(Joe) Xu Modified:
3 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, awdf+watch_chromium.org, mlamouri+watch-geolocation_chromium.org, aboxhall+watch_chromium.org, tfarina, mlamouri+watch-notifications_chromium.org, nektar+watch_chromium.org, chromium-apps-reviews_chromium.org, yuzo+watch_chromium.org, Peter Beverloo, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, Michael van Ouwerkerk, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not give instant focus if a view's toplevelwidget is not active
If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active.
Based on this rule, a set of unittests related are modified.
This is a following up CL for 2113163002 as there are already too many patches there.
BUG=621791
BUG=650776
BUG=152938
TEST=add an interactive_ui_test, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus
Committed: https://crrev.com/5c16e0e849526c1c627e08a05351ab38a9cfbcf1
Cr-Commit-Position: refs/heads/master@{#426905}
Patch Set 1 #Patch Set 2 : old issue base #Patch Set 3 : new patch #
Total comments: 2
Patch Set 4 : add ClearNativeFocus() #
Total comments: 2
Patch Set 5 : replace ClearNativeFocus with widget_->Activate() #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : fix test failures #
Total comments: 1
Patch Set 8 : rebase #Patch Set 9 : based on sky's comments #
Total comments: 2
Patch Set 10 : move toolbar_view_browsertest.cc #Patch Set 11 : rebase #Patch Set 12 : using WidgetActivationWaiter #Patch Set 13 : nit #
Total comments: 10
Patch Set 14 : rebase #Patch Set 15 : move added test to widget_interactive_uitest #
Total comments: 2
Patch Set 16 : rebase #Patch Set 17 : move test to native_widget_aura_interactive_uitest.cc #
Total comments: 6
Patch Set 18 : based on comments #Messages
Total messages: 114 (69 generated)
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active BUG=621791 ========== to ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) Within desktop_native_widget_aura HandleActivationChanged, we should properly restore last focused view if there is a stored one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
warx@chromium.org changed reviewers: + oshima@chromium.org, sky@chromium.org
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) Within desktop_native_widget_aura HandleActivationChanged, we should properly restore last focused view if there is a stored one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We should correctly restore the stored focus in this case. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We should correctly restore the stored focus in this case. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. (5) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. (5) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We bailed out the focus set when widget_ is not active. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. (5) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
Hi oshima, sky, this is a following up CL because the original one is a little messy. I think creating a seperate one will be much cleaner. Thanks for reviewing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/27 23:33:17, Qiang (Joe) Xu wrote: > Hi oshima, sky, this is a following up CL because the original one is a little > messy. I think creating a seperate one will be much cleaner. Thanks for > reviewing. Can you upload the old one first then new one so that we can see the diff?
On 2016/09/28 19:49:57, oshima wrote: > On 2016/09/27 23:33:17, Qiang (Joe) Xu wrote: > > Hi oshima, sky, this is a following up CL because the original one is a little > > messy. I think creating a seperate one will be much cleaner. Thanks for > > reviewing. > > Can you upload the old one first then new one so that we can see the diff? Sure. patch 2 is the old one and patch 3 is the same as patch 1.
lgtm, thanks. nit: add 650776 to the bug list
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We bailed out the focus set when widget_ is not active. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. (5) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. BUG=621791 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We bailed out the focus set when widget_ is not active. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. (5) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. BUG=621791 BUG=650776 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
I'm very worried about this change. Prior to your change calling View::RequestFocus would implicitly focus the native widget. That is no longer the case with your change. Additionally that you're having to add ui_test_utils::BringBrowserWindowToFront() in a bunch of tests implies either those tests need to be in interactive_ui_tests and/or your change is breaking valid use cases. I'm pretty sure this change will break a bunch of stuff and isn't something we should do. What is the specific bug you're trying to fix? https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:476: EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(current_browser())); Why is this necessary? In process browser tests already do this. If it is necessary, presumably it should be an ASSERT as if it fails the rest of the test won't work, right?
The basic logic in FocusManager::SetFocusedView is changed to only when |widget_| is active, the |view| can be focused. Thus, the setfocusedview call is rejected when nonactive and restored only when |widget_| becomes active. The existing tests with non active top level window but calling either requestfocus or setfocusedview are all failed. I think in actual user cases, window activation should not be implicitly caused by ime requesting focus, instead ime requesting focus should depend on an activated window. This low level change makes me worried too. However, just from the broken tests, I could feel they are all because |widget_| is not activated. I don't think it may break other stuffs. https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc (right): https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:476: EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(current_browser())); On 2016/09/28 21:56:53, sky wrote: > Why is this necessary? In process browser tests already do this. If it is > necessary, presumably it should be an ASSERT as if it fails the rest of the test > won't work, right? These two browser_tests are both related to more than one browser window (because of incognito windows). I feel I could put it on a better place for this one. Let me check it.
On Wed, Sep 28, 2016 at 5:07 PM, <warx@chromium.org> wrote: > The basic logic in FocusManager::SetFocusedView is changed to only when > |widget_| is active, I think you mean !active, right? 322 if (view && !widget_->IsActive()) { 323 SetStoredFocusView(view); 324 return; 325 } That early return means no implicit activation. Again, I think this is going to cause a lot of problems and isn't something we should do. -Scott the |view| can be focused. Thus, the setfocusedview > call is > rejected when nonactive and restored only when |widget_| becomes active. The > existing tests with non active top level window but calling either > requestfocus > or setfocusedview are all failed. > I think in actual user cases, window activation should not be implicitly > caused > by ime requesting focus, instead ime requesting focus should depend on an > activated window. > This low level change makes me worried too. However, just from the broken > tests, > I could feel they are all because |widget_| is not activated. I don't think > it > may break other stuffs. > > > https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... > File > chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc > (right): > > https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:476: > EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(current_browser())); > On 2016/09/28 21:56:53, sky wrote: >> Why is this necessary? In process browser tests already do this. If it > is >> necessary, presumably it should be an ASSERT as if it fails the rest > of the test >> won't work, right? > > These two browser_tests are both related to more than one browser window > (because of incognito windows). I feel I could put it on a better place > for this one. Let me check it. > > https://codereview.chromium.org/2371113003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/09/29 03:34:39, sky wrote:
> On Wed, Sep 28, 2016 at 5:07 PM, <mailto:warx@chromium.org> wrote:
> > The basic logic in FocusManager::SetFocusedView is changed to only when
> > |widget_| is active,
>
> I think you mean !active, right?
>
> 322 if (view && !widget_->IsActive()) {
> 323 SetStoredFocusView(view);
> 324 return;
> 325 }
>
The current code is problematic because it sets the focus on view even if it
can't/shoudn't,
and he found bugs in the tests, which are fixed in this CL.
> That early return means no implicit activation. Again, I think this is
> going to cause a lot of problems and isn't something we should do.
Do you mean we assume that requesting a focus will activate the window?
I think that's not generally true because the window may not be activatable.
If requesting focus should activate the window, how about:
if (view && !widget->IsActive()) {
SetStoredFocuedView(view);
widget->Activate();
return;
}
If the window can be activated, the focus will be set upon activation.
Otherwise, it will be set
when it becomes activatable and activated.
>
> -Scott
>
> the |view| can be focused. Thus, the setfocusedview
> > call is
> > rejected when nonactive and restored only when |widget_| becomes active. The
> > existing tests with non active top level window but calling either
> > requestfocus
> > or setfocusedview are all failed.
> > I think in actual user cases, window activation should not be implicitly
> > caused
> > by ime requesting focus, instead ime requesting focus should depend on an
> > activated window.
> > This low level change makes me worried too. However, just from the broken
> > tests,
> > I could feel they are all because |widget_| is not activated. I don't think
> > it
> > may break other stuffs.
> >
> >
> >
>
https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio...
> > File
> > chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio...
> > chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:476:
> > EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(current_browser()));
> > On 2016/09/28 21:56:53, sky wrote:
> >> Why is this necessary? In process browser tests already do this. If it
> > is
> >> necessary, presumably it should be an ASSERT as if it fails the rest
> > of the test
> >> won't work, right?
> >
> > These two browser_tests are both related to more than one browser window
> > (because of incognito windows). I feel I could put it on a better place
> > for this one. Let me check it.
> >
> > https://codereview.chromium.org/2371113003/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
c
On Thu, Sep 29, 2016 at 11:48 AM, <oshima@chromium.org> wrote: > On 2016/09/29 03:34:39, sky wrote: >> On Wed, Sep 28, 2016 at 5:07 PM, <mailto:warx@chromium.org> wrote: >> > The basic logic in FocusManager::SetFocusedView is changed to only when >> > |widget_| is active, >> >> I think you mean !active, right? >> >> 322 if (view && !widget_->IsActive()) { >> 323 SetStoredFocusView(view); >> 324 return; >> 325 } >> > > The current code is problematic because it sets the focus on view even if it > can't/shoudn't, > and he found bugs in the tests, which are fixed in this CL. > >> That early return means no implicit activation. Again, I think this is >> going to cause a lot of problems and isn't something we should do. > > Do you mean we assume that requesting a focus will activate the window? My concern is that there a bunch of calls assuming the current behavior, and to change it is likely to break behavior. If we had no code and where starting from scratch than I completely agree with this change. Focus shouldn't imply activation. > I think that's not generally true because the window may not be activatable. > If requesting focus should activate the window, how about: > > if (view && !widget->IsActive()) { > SetStoredFocuedView(view); > widget->Activate(); > return; > } > > If the window can be activated, the focus will be set upon activation. > Otherwise, it will be set > when it becomes activatable and activated. This seems reasonable to me. -Scott > > >> >> -Scott >> >> the |view| can be focused. Thus, the setfocusedview >> > call is >> > rejected when nonactive and restored only when |widget_| becomes active. >> > The >> > existing tests with non active top level window but calling either >> > requestfocus >> > or setfocusedview are all failed. >> > I think in actual user cases, window activation should not be implicitly >> > caused >> > by ime requesting focus, instead ime requesting focus should depend on >> > an >> > activated window. >> > This low level change makes me worried too. However, just from the >> > broken >> > tests, >> > I could feel they are all because |widget_| is not activated. I don't >> > think >> > it >> > may break other stuffs. >> > >> > >> > >> > https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... >> > File >> > chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc >> > (right): >> > >> > >> > https://codereview.chromium.org/2371113003/diff/40001/chrome/browser/extensio... >> > >> > chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:476: >> > >> > EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(current_browser())); >> > On 2016/09/28 21:56:53, sky wrote: >> >> Why is this necessary? In process browser tests already do this. If it >> > is >> >> necessary, presumably it should be an ASSERT as if it fails the rest >> > of the test >> >> won't work, right? >> > >> > These two browser_tests are both related to more than one browser window >> > (because of incognito windows). I feel I could put it on a better place >> > for this one. Let me check it. >> > >> > https://codereview.chromium.org/2371113003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > c > > https://codereview.chromium.org/2371113003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We bailed out the focus set when widget_ is not active. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) Move geolocation_browsertest.cc and downloads_api_browsertest.cc to interactive_ui_tests as they are relying on browser window activation. (5) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. BUG=621791 BUG=650776 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We bailed out the focus set when widget_ is not active. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. (5) Besides SetStoredFocusView in focus_manager.cc when widget_ is not active, add a ClearNativeFocus call, to allow receiving keyboard inputs, and windows platform setfocus. BUG=621791 BUG=650776 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
Hi all, updates include: (1) basically a ClearNativeFocus call is added. (2) description of this issue is updated. (3) downloads_api_browsertest.cc and geolocation_browsertest.cc are restored to no change as they are failed on keyboard inputs input text type check and failed on windows platform only. It is related to the description of ClearNativeFocus. ClearNativeFocus fixes this. Thanks for reviewing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you outline why you still need the changes to the other tests?
On 2016/09/30 15:26:39, sky wrote: > Can you outline why you still need the changes to the other tests? The other tests are trying to set focus but the widget is not active. Since we reject view->Focus() in those cases and there are no reactivation of widget to restore the stored focus view, thus any following view->HasFocus() check will fail. (1) changing TYPE_POPUP to TYPE_WINDOW_FRAMELESS is to allow widget activatable, and if there is a missing widget Show call, then we add it. (2) tray_details_view_unittest is trying to focus the content but the bubble is not active. (3) In widget_interactive_uitest.cc, the order is wrong. (4) In focus_manager_unittest.cc is to make top level widget active. (5) In toolbar_view_browsertest.cc, which is failed on linux only. ToolbarCycleFocusWithBookmarkBar creates a second browser, and I think it makes sense to set the second browser active and wait since linux window activation is async.
Sorry for the run around this. Focus is always tricky. https://codereview.chromium.org/2371113003/diff/80001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2371113003/diff/80001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:325: ClearNativeFocus(); I'm wrong. It's the call to Focus() on 346 that triggers activation. So, can this line be a call to widget_->Activate()? That way if the activation fails nothing weird has happened, but if the activation succeeds we get the old logic?
https://codereview.chromium.org/2371113003/diff/80001/ui/views/focus/focus_ma... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2371113003/diff/80001/ui/views/focus/focus_ma... ui/views/focus/focus_manager.cc:325: ClearNativeFocus(); On 2016/10/03 15:56:18, sky wrote: > I'm wrong. It's the call to Focus() on 346 that triggers activation. So, can > this line be a call to widget_->Activate()? That way if the activation fails > nothing weird has happened, but if the activation succeeds we get the old logic? I agree with it. In this way, activatable ones remain the behaviors when restoring the focus. Non-activatable ones just silently store the view. I need to change the unit test I added. There are also some tests failing. Let me check them.
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. Updates include: (1) using !defined(OS_MACOSX) to allow a separate issue to track crbug.com/650859. (2) based on the description of this cl: https://codereview.chromium.org/23702017. We bailed out the focus set when widget_ is not active. We should correctly restore the stored focus in this case if there is one. (3) Fix in toolbar_view_browsertest.cc, and remove flakiness on win. (4) tray_details_view_unittest.cc is a new fix targeting for crbug.com/650776. (5) Besides SetStoredFocusView in focus_manager.cc when widget_ is not active, add a ClearNativeFocus call, to allow receiving keyboard inputs, and windows platform setfocus. BUG=621791 BUG=650776 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
This is a tentative CL for review. As I am not sure why some tests are still failing on (3) and (4). Thanks for reviewing! Updates include: (1) replace ClearNativeFocus with widget_->Activate(). I think this is to set widget activation if it is activatable, when a focus request comes. (2) In tray_details_view_unittest.cc, tray->ActivateBubble() is changed to tray->GetSystemBubble()->bubble_view()->set_can_activate(true). Since activation is implied by FocusTitleRow() call. (3) In ax_tree_source_aura_unittest.cc and app_info_dialog_ash_unittest.cc, I am not sure why the change is needed now. (4) In toolbar_view_browsertest.cc, it is still needed to pass on linux. Not sure why. (5) The DCHECK in native_widget_mus.cc is modified, Activate no longer gurantees window_->HasFocus(). (6) native_widget_aura_unittest.cc is updated. (7) In widget_interactive_uitest.cc, (7-1) From the description "Test input method focus changes affected by top window activaction.", I assume the ShowSync should be before textfield->RequestFocus(). (7-2) 1748 line ActivateSync is no longer needed, since RequestFocus() implies that.
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give focus if a view's toplevelwidget is not active and not activatable Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Please make sure you understand why 3 and 4 are necessary. https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... ui/views/focus/focus_manager.cc:320: // If |widget_| is not active, focus is not allowed to set within |widget_| I think you want something like: If the widget isn't active store the focused view and then attempt to activate the widget. If activation succeeds |view| will be focused. If activation fails |view| will be focused the next time the widget is made active.
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active and not activatable Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give instant focus when setting focus if a view's toplevelwidget is not active When setting focus is called, If the toplevelwidget isn't active, store the focused view and then attempt to activate the widget. If activation succeeds, focus is set by restoring focused view. If activation fails, focus will be set the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Scott, This patch has the following updates: (1) For app_info_dialog_ash_unittest.cc, widget_->Show() is needed because it needs to be IsVisible for CanActivate https://cs.chromium.org/chromium/src/ui/wm/core/base_focus_rules.cc?q=IsWindo.... This is because in ash test, FocusController::FocusAndActivateWindow is called where the above focus rules are applied. I have seen other places in the codebase having CreateDialogWidget and then having a Show() call. So it should be fine. widget_->Show() can also be widget_->ShowInactive(), just need to make sure it is visible. (2) For the reason (1), combobox_unittest, view_targeter_unittest and root_view_unittest are all failed on views_mus_unittests. I think mus is using ash. Adding widget.Show() makes them pass the test. (3) For ax_tree_source_aura_unittest, widget needs to be CanActivate, which leads the change. (4) For toolbar_view_browsertest.cc, I make the change base::RunLoop().RunUntilIdle() just after the first setting focus call. This allows linux platfom async window activation. (5) In widget_interactive_uitest.cc, I add the #define(OS_MACOSX) since there is a EXPECT_NE check in WidgetActivationWaiter ctor. Thanks for reviewing! https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... ui/views/focus/focus_manager.cc:320: // If |widget_| is not active, focus is not allowed to set within |widget_| On 2016/10/04 15:47:03, sky wrote: > I think you want something like: > > If the widget isn't active store the focused view and then attempt to activate > the widget. If activation succeeds |view| will be focused. If activation fails > |view| will be focused the next time the widget is made active. Done.
On 2016/10/10 16:40:01, Qiang (Joe) Xu wrote: > Hi Scott, > > This patch has the following updates: > (1) For app_info_dialog_ash_unittest.cc, widget_->Show() is needed because it > needs to be IsVisible for CanActivate > https://cs.chromium.org/chromium/src/ui/wm/core/base_focus_rules.cc?q=IsWindo.... > This is because in ash test, FocusController::FocusAndActivateWindow is called > where the above focus rules are applied. > I have seen other places in the codebase having CreateDialogWidget and then > having a Show() call. So it should be fine. widget_->Show() can also be > widget_->ShowInactive(), just need to make sure it is visible. This sounds reasonable, but why is this different with your patch? By that I mean how is the test passing without your change, and what in your change causes the failure. Same comment for the remaining ones too. > (2) For the reason (1), combobox_unittest, view_targeter_unittest and > root_view_unittest are all failed on views_mus_unittests. I think mus is using > ash. Adding widget.Show() makes them pass the test. > > (3) For ax_tree_source_aura_unittest, widget needs to be CanActivate, which > leads the change. > > (4) For toolbar_view_browsertest.cc, I make the change > base::RunLoop().RunUntilIdle() just after the first setting focus call. This > allows linux platfom async window activation. > > (5) In widget_interactive_uitest.cc, I add the #define(OS_MACOSX) since there is > a EXPECT_NE check in WidgetActivationWaiter ctor. > > Thanks for reviewing! > > https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... > File ui/views/focus/focus_manager.cc (right): > > https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... > ui/views/focus/focus_manager.cc:320: // If |widget_| is not active, focus is not > allowed to set within |widget_| > On 2016/10/04 15:47:03, sky wrote: > > I think you want something like: > > > > If the widget isn't active store the focused view and then attempt to activate > > the widget. If activation succeeds |view| will be focused. If activation fails > > |view| will be focused the next time the widget is made active. > > Done.
Description was changed from ========== Do not give focus if a view's toplevelwidget is not active and not activatable Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
On 2016/10/10 19:47:08, sky wrote: > On 2016/10/10 16:40:01, Qiang (Joe) Xu wrote: > > Hi Scott, > > > > This patch has the following updates: > > (1) For app_info_dialog_ash_unittest.cc, widget_->Show() is needed because it > > needs to be IsVisible for CanActivate > > > https://cs.chromium.org/chromium/src/ui/wm/core/base_focus_rules.cc?q=IsWindo.... > > This is because in ash test, FocusController::FocusAndActivateWindow is called > > where the above focus rules are applied. > > I have seen other places in the codebase having CreateDialogWidget and then > > having a Show() call. So it should be fine. widget_->Show() can also be > > widget_->ShowInactive(), just need to make sure it is visible. > > This sounds reasonable, but why is this different with your patch? By that I > mean how is the test passing without your change, and what in your change causes > the failure. Same comment for the remaining ones too. > > > (2) For the reason (1), combobox_unittest, view_targeter_unittest and > > root_view_unittest are all failed on views_mus_unittests. I think mus is using > > ash. Adding widget.Show() makes them pass the test. > > > > (3) For ax_tree_source_aura_unittest, widget needs to be CanActivate, which > > leads the change. > > > > (4) For toolbar_view_browsertest.cc, I make the change > > base::RunLoop().RunUntilIdle() just after the first setting focus call. This > > allows linux platfom async window activation. > > > > (5) In widget_interactive_uitest.cc, I add the #define(OS_MACOSX) since there > is > > a EXPECT_NE check in WidgetActivationWaiter ctor. > > > > Thanks for reviewing! > > > > > https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... > > File ui/views/focus/focus_manager.cc (right): > > > > > https://codereview.chromium.org/2371113003/diff/100001/ui/views/focus/focus_m... > > ui/views/focus/focus_manager.cc:320: // If |widget_| is not active, focus is > not > > allowed to set within |widget_| > > On 2016/10/04 15:47:03, sky wrote: > > > I think you want something like: > > > > > > If the widget isn't active store the focused view and then attempt to > activate > > > the widget. If activation succeeds |view| will be focused. If activation > fails > > > |view| will be focused the next time the widget is made active. > > > > Done. Without my change, the test passes because we didn't check widget_->IsActive() in the test. If https://cs.chromium.org/chromium/src/chrome/browser/ui/views/apps/app_info_di... is modified to pin_button->RequestFocus(); EXPECT_TRUE(pin_button->visible()); EXPECT_FALSE(unpin_button->visible()); EXPECT_TRUE(pin_button->HasFocus()); EXPECT_TRUE(widget_->IsActive()); Test will fail on the last line. This is almost the same case for the bug crbug.com/621791. Widget is not activatable (because not visible here), RequestFocus can grab the focus (but widget_ is still not active).
Thanks for the clarification. Only one issue. Also, is the ifdef still needed for mac in focus_manager? https://codereview.chromium.org/2371113003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view_browsertest.cc (right): https://codereview.chromium.org/2371113003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view_browsertest.cc:105: IN_PROC_BROWSER_TEST_F(ToolbarViewTest, ToolbarCycleFocusWithBookmarkBar) { It seems as though this test relies on focus. If that's the case, then it shouldn't in browser_tests, rather interactive_ui_tests. That's likely why it's flaky. I don't think your patch helps it. Also, you should document why the RunUntilIdle is necessary.
On 2016/10/10 22:47:22, sky wrote: > Thanks for the clarification. Only one issue. > > Also, is the ifdef still needed for mac in focus_manager? > > https://codereview.chromium.org/2371113003/diff/140001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/toolbar_view_browsertest.cc (right): > > https://codereview.chromium.org/2371113003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_view_browsertest.cc:105: > IN_PROC_BROWSER_TEST_F(ToolbarViewTest, ToolbarCycleFocusWithBookmarkBar) { > It seems as though this test relies on focus. If that's the case, then it > shouldn't in browser_tests, rather interactive_ui_tests. That's likely why it's > flaky. I don't think your patch helps it. > > Also, you should document why the RunUntilIdle is necessary. ifdef for mac is still needed. https://bugs.chromium.org/p/chromium/issues/detail?id=650859 comment #1 has details. Updates: (1) I put the toolbar_view_browsertest.cc to interactive_ui_tests. It makes more sense there. (2) document RunUntilIdle
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2371113003/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2371113003/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:558: "../browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc", It is confusing to have both a toolbar_view_browsertest and toolbar_view_interactive_uitest both in interactive ui tests. How about copy/paste the code from toolbar_view_browsertest.cc into interactive_uitest and nuke toolbar_view_browsertest.cc?
One followup question on Rununtilidle. Rununtilidle runs until there are no events in the runloop. That's different than waiting from a response from X. How do you know X has responded in time?
Patchset #10 (id:200001) has been deleted
On 2016/10/11 19:24:50, sky wrote: > One followup question on Rununtilidle. Rununtilidle runs until there are no > events in the runloop. That's different than waiting from a response from X. How > do you know X has responded in time? mm.. I don't know on this. I dig the code base a little bit. content::RunAllPendingInMessageLoop() is used instead in several places. (1) https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... Though the test is disabled. (2) https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_focus_uitest.c... (3) https://cs.chromium.org/chromium/src/chrome/browser/ui/views/passwords/manage... The browser window should be active for the following tests. But on linux, it depends on asynchronous update from the window server. Is content::RunAllPendingInMessageLoop enough? I run the test locally, it can pass well. Thanks!
https://codereview.chromium.org/2371113003/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2371113003/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:558: "../browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc", On 2016/10/11 19:24:09, sky wrote: > It is confusing to have both a toolbar_view_browsertest and > toolbar_view_interactive_uitest both in interactive ui tests. How about > copy/paste the code from toolbar_view_browsertest.cc into interactive_uitest and > nuke toolbar_view_browsertest.cc? Done.
On 2016/10/11 22:49:11, Qiang (Joe) Xu wrote: > On 2016/10/11 19:24:50, sky wrote: > > One followup question on Rununtilidle. Rununtilidle runs until there are no > > events in the runloop. That's different than waiting from a response from X. > How > > do you know X has responded in time? > > mm.. I don't know on this. I dig the code base a little bit. > > content::RunAllPendingInMessageLoop() is used instead in several places. > (1) > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... > Though the test is disabled. > (2) > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_focus_uitest.c... > (3) > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/passwords/manage... > > The browser window should be active for the following tests. But on linux, it > depends on asynchronous update from the window server. > Is content::RunAllPendingInMessageLoop enough? I run the test locally, it can > pass well. > > Thanks! I'm skeptical that is good enough. Running locally is one thing, running on the bots 1000 of times a day with varying loads is another thing. We don't want more flake. You should *wait* for activation, and then continue on.
On 2016/10/11 23:14:58, sky wrote: > On 2016/10/11 22:49:11, Qiang (Joe) Xu wrote: > > On 2016/10/11 19:24:50, sky wrote: > > > One followup question on Rununtilidle. Rununtilidle runs until there are no > > > events in the runloop. That's different than waiting from a response from X. > > How > > > do you know X has responded in time? > > > > mm.. I don't know on this. I dig the code base a little bit. > > > > content::RunAllPendingInMessageLoop() is used instead in several places. > > (1) > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... > > Though the test is disabled. > > (2) > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_focus_uitest.c... > > (3) > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/passwords/manage... > > > > The browser window should be active for the following tests. But on linux, it > > depends on asynchronous update from the window server. > > Is content::RunAllPendingInMessageLoop enough? I run the test locally, it can > > pass well. > > > > Thanks! > > I'm skeptical that is good enough. Running locally is one thing, running on the > bots 1000 of times a day with varying loads is another thing. We don't want more > flake. You should *wait* for activation, and then continue on. We use WidgetActivationWaiter in a couple of tests. Sounds like it'd be a good idea to make it available in test_support and use it here too.
+1 On Tue, Oct 11, 2016 at 4:19 PM, <sadrul@chromium.org> wrote: > On 2016/10/11 23:14:58, sky wrote: >> On 2016/10/11 22:49:11, Qiang (Joe) Xu wrote: >> > On 2016/10/11 19:24:50, sky wrote: >> > > One followup question on Rununtilidle. Rununtilidle runs until there >> > > are > no >> > > events in the runloop. That's different than waiting from a response >> > > from > X. >> > How >> > > do you know X has responded in time? >> > >> > mm.. I don't know on this. I dig the code base a little bit. >> > >> > content::RunAllPendingInMessageLoop() is used instead in several places. >> > (1) >> > >> > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... >> > Though the test is disabled. >> > (2) >> > >> > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_focus_uitest.c... >> > (3) >> > >> > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/passwords/manage... >> > >> > The browser window should be active for the following tests. But on >> > linux, > it >> > depends on asynchronous update from the window server. >> > Is content::RunAllPendingInMessageLoop enough? I run the test locally, >> > it > can >> > pass well. >> > >> > Thanks! >> >> I'm skeptical that is good enough. Running locally is one thing, running >> on > the >> bots 1000 of times a day with varying loads is another thing. We don't >> want > more >> flake. You should *wait* for activation, and then continue on. > > We use WidgetActivationWaiter in a couple of tests. Sounds like it'd be a > good > idea to make it available in test_support and use it here too. > > https://codereview.chromium.org/2371113003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/11 23:27:32, sky wrote: > +1 > > On Tue, Oct 11, 2016 at 4:19 PM, <mailto:sadrul@chromium.org> wrote: > > On 2016/10/11 23:14:58, sky wrote: > >> On 2016/10/11 22:49:11, Qiang (Joe) Xu wrote: > >> > On 2016/10/11 19:24:50, sky wrote: > >> > > One followup question on Rununtilidle. Rununtilidle runs until there > >> > > are > > no > >> > > events in the runloop. That's different than waiting from a response > >> > > from > > X. > >> > How > >> > > do you know X has responded in time? > >> > > >> > mm.. I don't know on this. I dig the code base a little bit. > >> > > >> > content::RunAllPendingInMessageLoop() is used instead in several places. > >> > (1) > >> > > >> > > > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... > >> > Though the test is disabled. > >> > (2) > >> > > >> > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_focus_uitest.c... > >> > (3) > >> > > >> > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/passwords/manage... > >> > > >> > The browser window should be active for the following tests. But on > >> > linux, > > it > >> > depends on asynchronous update from the window server. > >> > Is content::RunAllPendingInMessageLoop enough? I run the test locally, > >> > it > > can > >> > pass well. > >> > > >> > Thanks! > >> > >> I'm skeptical that is good enough. Running locally is one thing, running > >> on > > the > >> bots 1000 of times a day with varying loads is another thing. We don't > >> want > > more > >> flake. You should *wait* for activation, and then continue on. > > > > We use WidgetActivationWaiter in a couple of tests. Sounds like it'd be a > > good > > idea to make it available in test_support and use it here too. > > > > https://codereview.chromium.org/2371113003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > We use WidgetActivationWaiter in a couple of tests. Sounds like it'd be good idea to make it available in test_support and use it here too. Is it good to use a separate bug to track it?
You shouldn't enable this test until you use something like WidgetActivationWaiter. Two options: . leave the test disabled in your patch, separate out WidgetActivationWaiter in a separate patch, then update test and reenable. . separate out WidgetActivationWaiter first, then update this patch. I'm fine with either approach. -Scott On Tue, Oct 11, 2016 at 4:31 PM, <warx@chromium.org> wrote: > On 2016/10/11 23:27:32, sky wrote: >> +1 >> >> On Tue, Oct 11, 2016 at 4:19 PM, <mailto:sadrul@chromium.org> wrote: >> > On 2016/10/11 23:14:58, sky wrote: >> >> On 2016/10/11 22:49:11, Qiang (Joe) Xu wrote: >> >> > On 2016/10/11 19:24:50, sky wrote: >> >> > > One followup question on Rununtilidle. Rununtilidle runs until >> >> > > there >> >> > > are >> > no >> >> > > events in the runloop. That's different than waiting from a >> >> > > response >> >> > > from >> > X. >> >> > How >> >> > > do you know X has responded in time? >> >> > >> >> > mm.. I don't know on this. I dig the code base a little bit. >> >> > >> >> > content::RunAllPendingInMessageLoop() is used instead in several >> >> > places. >> >> > (1) >> >> > >> >> >> > >> > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... >> >> > Though the test is disabled. >> >> > (2) >> >> > >> >> >> > >> > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_focus_uitest.c... >> >> > (3) >> >> > >> >> >> > >> > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/passwords/manage... >> >> > >> >> > The browser window should be active for the following tests. But on >> >> > linux, >> > it >> >> > depends on asynchronous update from the window server. >> >> > Is content::RunAllPendingInMessageLoop enough? I run the test >> >> > locally, >> >> > it >> > can >> >> > pass well. >> >> > >> >> > Thanks! >> >> >> >> I'm skeptical that is good enough. Running locally is one thing, >> >> running >> >> on >> > the >> >> bots 1000 of times a day with varying loads is another thing. We don't >> >> want >> > more >> >> flake. You should *wait* for activation, and then continue on. >> > >> > We use WidgetActivationWaiter in a couple of tests. Sounds like it'd be >> > a >> > good >> > idea to make it available in test_support and use it here too. >> > >> > https://codereview.chromium.org/2371113003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > We use WidgetActivationWaiter in a couple of tests. Sounds like it'd be good > idea to make it available in test_support and use it here too. > > Is it good to use a separate bug to track it? > > https://codereview.chromium.org/2371113003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Scott, updates include: (1) using WidgetActivationWaiter in toolbar_view_interactive_uitest.cc (2) restoring WidgetInputMethodInteractiveTest.OneWindow test. I think it is OK to have duplicate activation (one by requestfocus and one by activationsync), otherwise if needs #if !defined(OS_MACOSX) and #else.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #13 (id:280001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2371113003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc:26: #include "chrome/browser/ui/views/toolbar/toolbar_view.h" This include should the first include, then newline (see style guide for details). https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:161: std::unique_ptr<Widget> widget1(new Widget()); widget1 and widget2 can be defined on the stack (no need to wrap in unique_ptr). https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:174: textfield2->RequestFocus(); Will this trigger activation at the native level? Say when this test is run on linux or windows non-chromeos? If so, it should move to interactive tests. https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1658: ShowSync(widget); Why does the order here matter?
Hi Scott, I have a question regarding tests on other non-chromeos platforms. https://codereview.chromium.org/2371113003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc:26: #include "chrome/browser/ui/views/toolbar/toolbar_view.h" On 2016/10/13 16:21:21, sky wrote: > This include should the first include, then newline (see style guide for > details). Done. https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:161: std::unique_ptr<Widget> widget1(new Widget()); On 2016/10/13 16:21:21, sky wrote: > widget1 and widget2 can be defined on the stack (no need to wrap in unique_ptr). Done. https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:174: textfield2->RequestFocus(); On 2016/10/13 16:21:21, sky wrote: > Will this trigger activation at the native level? Say when this test is run on > linux or windows non-chromeos? If so, it should move to interactive tests. Because I put it in NativeWidgetAuraTest. So it should only run in ChromeOS. Yeah, I should put it in widget_interactive_uitest to support more platforms. However, it is a little tricky here. widget_interactive_uitest uses ViewsTestBase, which eventually set up this: https://cs.chromium.org/chromium/src/ui/views/test/views_test_helper_aura.cc?... The DefaultActivationClient activates window whenever requested. However, we need something that widget is non-activatable first and then activatable. On chromeos, widget will always be activatable in widget_interactive_uitest. (1) We properly want an overridden focus rules. (2) split the test into NativeWidgetAuraTest and DesktopNativeWidgetAuraTest (which can be put in widget_interactive_uitest). What do you think? https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1658: ShowSync(widget); On 2016/10/13 16:21:22, sky wrote: > Why does the order here matter? Oh, the master doesn't matter any more. I will restore it. Thanks for pointing out.
https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:174: textfield2->RequestFocus(); On 2016/10/14 01:42:39, Qiang (Joe) Xu wrote: > On 2016/10/13 16:21:21, sky wrote: > > Will this trigger activation at the native level? Say when this test is run on > > linux or windows non-chromeos? If so, it should move to interactive tests. > > Because I put it in NativeWidgetAuraTest. So it should only run in ChromeOS. NativeWidgetAuraTest runs everywhere but mac (mac doesn't support aura, so it isn't applicable). > Yeah, I should put it in widget_interactive_uitest to support more platforms. I'm fine keeping the test specific to NativeWidgetAura, but you need to make sure it doesn't try to do activation at the native level. If it does, then it needs to be moved to interactive ui tests. > However, it is a little tricky here. widget_interactive_uitest uses > ViewsTestBase, which eventually set up this: > https://cs.chromium.org/chromium/src/ui/views/test/views_test_helper_aura.cc?... > > The DefaultActivationClient activates window whenever requested. However, we > need something that widget is non-activatable first and then activatable. On > chromeos, widget will always be activatable in widget_interactive_uitest. > > (1) We properly want an overridden focus rules. > (2) split the test into NativeWidgetAuraTest and DesktopNativeWidgetAuraTest > (which can be put in widget_interactive_uitest). > What do you think?
Patchset #14 (id:320001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #15 (id:360001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #15 (id:380001) has been deleted
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #15 (id:400001) has been deleted
Patchset #14 (id:340001) has been deleted
Description was changed from ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, WidgetInputMethodInteractiveTest.NonActiveWindowRequestImeFocus ==========
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Scott, new patch is uploaded. Change is moving the added test to interactive_ui_test. However, I don't know why the added test is failing on OS_WIN. It fails on line 1885 and 1886. So set_can_activate(false) doesn't work on OS_WIN. Thanks! https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2371113003/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:174: textfield2->RequestFocus(); On 2016/10/14 15:41:36, sky wrote: > On 2016/10/14 01:42:39, Qiang (Joe) Xu wrote: > > On 2016/10/13 16:21:21, sky wrote: > > > Will this trigger activation at the native level? Say when this test is run > on > > > linux or windows non-chromeos? If so, it should move to interactive tests. > > > > Because I put it in NativeWidgetAuraTest. So it should only run in ChromeOS. > > NativeWidgetAuraTest runs everywhere but mac (mac doesn't support aura, so it > isn't applicable). > > > Yeah, I should put it in widget_interactive_uitest to support more platforms. > > I'm fine keeping the test specific to NativeWidgetAura, but you need to make > sure it doesn't try to do activation at the native level. If it does, then it > needs to be moved to interactive ui tests. > > > However, it is a little tricky here. widget_interactive_uitest uses > > ViewsTestBase, which eventually set up this: > > > https://cs.chromium.org/chromium/src/ui/views/test/views_test_helper_aura.cc?... > > > > The DefaultActivationClient activates window whenever requested. However, we > > need something that widget is non-activatable first and then activatable. On > > chromeos, widget will always be activatable in widget_interactive_uitest. > > > > (1) We properly want an overridden focus rules. > > (2) split the test into NativeWidgetAuraTest and DesktopNativeWidgetAuraTest > > (which can be put in widget_interactive_uitest). > > What do you think? > Since it does cause OnNativeWidgetActivationChanged, I then move the test to interactive_ui_test. Since the DefaultActivationClient activates window whenever requested, the test sets its own ActivationClient (reference: NativeWidgetAuraTest). I don't know why the test is failing on OS_WIN. Right now, I just disable it. Scott, do you have an idea on why test is failing on OS_WIN?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Your test is creating a DesktopNativeWidgetAura on windows, not a NativeWidgetAura. That's why it fails on windows. I would expect it to fail on a linux (non-chromeos) build as well. https://codereview.chromium.org/2371113003/diff/440001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/440001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1860: TEST_F(WidgetInputMethodInteractiveTest, MAYBE_NonActiveWindowRequestImeFocus) { How come you're grouping this in WidgetInputMethodInteractiveTest? Isn't this a test of NativeWidgetAura? I would expect a test name of NativeWidgetAuraTest and this to be in native_widget_aura_interactive_uitest.cc.
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Scott, I put the test in native_widget_aura_interactive_uitest.cc now. Thanks for clarifying. https://codereview.chromium.org/2371113003/diff/440001/ui/views/widget/widget... File ui/views/widget/widget_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/440001/ui/views/widget/widget... ui/views/widget/widget_interactive_uitest.cc:1860: TEST_F(WidgetInputMethodInteractiveTest, MAYBE_NonActiveWindowRequestImeFocus) { On 2016/10/19 16:23:32, sky wrote: > How come you're grouping this in WidgetInputMethodInteractiveTest? Isn't this a > test of NativeWidgetAura? I would expect a test name of NativeWidgetAuraTest and > this to be in native_widget_aura_interactive_uitest.cc. done
Description was changed from ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add a unittest, WidgetInputMethodInteractiveTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add an interactive_ui_test, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2371113003/diff/480001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2371113003/diff/480001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:592: if (!is_mac) { I would rather you add an use_aura case. Having a conditional like: if (foo || bar) { if (!bar) {} } Is just confusing. https://codereview.chromium.org/2371113003/diff/480001/ui/views/widget/native... File ui/views/widget/native_widget_aura_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/480001/ui/views/widget/native... ui/views/widget/native_widget_aura_interactive_uitest.cc:51: gl::GLSurfaceTestSupport::InitializeOneOff(); Ugh, we should really move this to a common base class so it isn't copied every where. Please file a bug on this (I don't want to hold your patch up any longer). https://codereview.chromium.org/2371113003/diff/480001/ui/views/widget/native... ui/views/widget/native_widget_aura_interactive_uitest.cc:69: // (crbug.com/621791) Please add a descriptive comment. You shouldn't need to chase down a bug report to figure out what the test is covering.
Hi Scott, new patch is uploaded. Thanks! https://codereview.chromium.org/2371113003/diff/480001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2371113003/diff/480001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:592: if (!is_mac) { On 2016/10/21 03:14:18, sky wrote: > I would rather you add an use_aura case. Having a conditional like: > > if (foo || bar) { > if (!bar) {} > } > > Is just confusing. Done. https://codereview.chromium.org/2371113003/diff/480001/ui/views/widget/native... File ui/views/widget/native_widget_aura_interactive_uitest.cc (right): https://codereview.chromium.org/2371113003/diff/480001/ui/views/widget/native... ui/views/widget/native_widget_aura_interactive_uitest.cc:51: gl::GLSurfaceTestSupport::InitializeOneOff(); On 2016/10/21 03:14:18, sky wrote: > Ugh, we should really move this to a common base class so it isn't copied every > where. Please file a bug on this (I don't want to hold your patch up any > longer). Done. https://codereview.chromium.org/2371113003/diff/480001/ui/views/widget/native... ui/views/widget/native_widget_aura_interactive_uitest.cc:69: // (crbug.com/621791) On 2016/10/21 03:14:18, sky wrote: > Please add a descriptive comment. You shouldn't need to chase down a bug report > to figure out what the test is covering. Done.
LGTM - yay! Thanks for your patience and diligence in seeing this patch through.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2371113003/#ps500001 (title: "based on comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add an interactive_ui_test, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add an interactive_ui_test, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ==========
Message was sent while issue was closed.
Committed patchset #18 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add an interactive_ui_test, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus ========== to ========== Do not give instant focus if a view's toplevelwidget is not active If the top level widget isn't active during setting focus view, we store the focused view and then attempt to activate the widget. If activation succeeds view will be focused. If activation fails |view| will be focused the next time the widget is made active. Based on this rule, a set of unittests related are modified. This is a following up CL for 2113163002 as there are already too many patches there. BUG=621791 BUG=650776 BUG=152938 TEST=add an interactive_ui_test, NativeWidgetAuraTest.NonActiveWindowRequestImeFocus Committed: https://crrev.com/5c16e0e849526c1c627e08a05351ab38a9cfbcf1 Cr-Commit-Position: refs/heads/master@{#426905} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/5c16e0e849526c1c627e08a05351ab38a9cfbcf1 Cr-Commit-Position: refs/heads/master@{#426905}
Message was sent while issue was closed.
On 2016/10/21 20:01:50, sky wrote:
> LGTM - yay! Thanks for your patience and diligence in seeing this patch
through.
Hi guys!
I work at company "Cliqz GmbH", where we do some experiments with Chromium
sources.
After merging with v56 we encountered an issue, stemming from this patch.
The scenario in short is: when clicking into WebView placed inside
OmniboxPopupContentsView, FocusManager activates the Widget under the popup,
which causes main window to loose focus and close the popup, preventing the
content from properly handling the click event.
We fixed it by adding another check, whether Widget even can be activated,
making it look like this:
if (view && !widget_->IsActive() && widget_->CanActivate()) {
SetStoredFocusView(view);
widget_->Activate();
return;
}
It looks valid to me, that a Widget should not be activated, if it says it
can't.
This patch also doesn't seem to break any tests or easily observed behaviour.
Question is: is it possible for you to accept this fix to your repo. It seems
valid, and we also want to keep our changes compared to upstream to a minimum of
course.
If it looks wrong, and there should be a better way to do it, I'll be glad to
hear about it :)
Best,
Max Breev
Message was sent while issue was closed.
On 2017/01/20 14:25:37, Knuckster wrote:
> On 2016/10/21 20:01:50, sky wrote:
> > LGTM - yay! Thanks for your patience and diligence in seeing this patch
> through.
>
> Hi guys!
> I work at company "Cliqz GmbH", where we do some experiments with Chromium
> sources.
> After merging with v56 we encountered an issue, stemming from this patch.
> The scenario in short is: when clicking into WebView placed inside
> OmniboxPopupContentsView, FocusManager activates the Widget under the popup,
> which causes main window to loose focus and close the popup, preventing the
> content from properly handling the click event.
> We fixed it by adding another check, whether Widget even can be activated,
> making it look like this:
>
> if (view && !widget_->IsActive() && widget_->CanActivate()) {
> SetStoredFocusView(view);
> widget_->Activate();
> return;
> }
>
> It looks valid to me, that a Widget should not be activated, if it says it
> can't.
> This patch also doesn't seem to break any tests or easily observed behaviour.
>
> Question is: is it possible for you to accept this fix to your repo. It seems
> valid, and we also want to keep our changes compared to upstream to a minimum
of
> course.
>
> If it looks wrong, and there should be a better way to do it, I'll be glad to
> hear about it :)
>
> Best,
> Max Breev
I think a better fix is so make Activate() do nothing if CanActivate() returns
false.
|
