Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604303002:
(Mac)Views: Widgets focus first View in traversal order if initial focus fails. (Closed)
Description(Mac)Views: Widgets focus first View in traversal order if initial focus fails.
views::Widgets use SetInitialFocus(), which calls
WidgetDelegate::GetInitiallyFocusedView() to set focus to a View on first
opening the Widget. In cases where GetInitiallyFocusedView() returns an
unfocusable View, the call to RequestFocus() silently fails and the Widget is
opened without focus on anything.
In particular, this is happening on MacViews when full keyboard access is turned
off. Support for full keyboard access was implemented in r391744, but
DialogDelegate::GetInitiallyFocusedView() still returns the dialog's default
button regardless of full keyboard access. This results in the dialog opening
with nothing focused.
Fix this in general for all views::Widgets trying to focus an unfocusable View -
if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to
become focused and no other View has focus, manually advance focus on the focus
manager instead.
BUG=657850
TEST=On Mac with #secondary-ui-md flag enabled and full keyboard access turned
off (System Preferences > Keyboard > Shortcuts, make sure 'All controls' radio
button is not selected), open the collected cookies dialog by going to
google.com, clicking 'Secure' on the left of the omnibox, and clicking 'X in
use' under 'Cookies'. The TreeView should have focus.
Review-Url: https://codereview.chromium.org/2604303002
Cr-Commit-Position: refs/heads/master@{#443827}
Committed: https://chromium.googlesource.com/chromium/src/+/7d47709bf1d00beaa556804b25300ebdceecca07
Patch Set 1 #Patch Set 2 : Use views:: everywhere for consistency (?) #
Total comments: 5
Patch Set 3 : Revert most changes and move fix to Widget::SetInitialFocus() instead. #
Total comments: 4
Patch Set 4 : Add test. #
Total comments: 9
Patch Set 5 : Review comments. #Patch Set 6 : Fix compile error. #
Total comments: 2
Patch Set 7 : Revert a bunch of changes. #
Total comments: 10
Patch Set 8 : Review comments. #Messages
Total messages: 66 (46 generated)
The CQ bit was checked by patricialor@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 checked by patricialor@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.
patricialor@chromium.org changed reviewers: + karandeepb@chromium.org
Hi Karan, PTAL! Thanks :)
https://codereview.chromium.org/2604303002/diff/20001/ui/views/controls/focus... File ui/views/controls/focus_ring.cc (left): https://codereview.chromium.org/2604303002/diff/20001/ui/views/controls/focus... ui/views/controls/focus_ring.cc:39: DCHECK(parent->HasFocus()); If it's ok, can you send a separate CL for this, since this is unrelated? https://codereview.chromium.org/2604303002/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2604303002/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:167: if (!dcv->GetFocusManager()->keyboard_accessible()) null-check the focus manager? Did you test this on other platforms? keyboard accessibility is off/false on all platforms except Mac. Hence won't this always return NULL on say Windows? We can #ifdef OS_MACOSX this, but I am not sure if this is the correct way to solve this. (doesn't look generic enough). Another option may be to do something more sensible in Widget::SetInitialFocus when the initially-focused-view retrieved can't take focus. For example, what does FocusManager::AdvanceFocus do, if the focus manager has no focused view?
The CQ bit was checked by patricialor@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 ========== MacViews: Dialogs shouldn't focus buttons when full keyboard access is off. Support for full keyboard access in Mac for Views was implemented in r391744, but DialogDelegate still returns the dialog's default button as the View to focus on first opening the dialog regardless of whether full keyboard access is on or off. This results in buttons being returned as the initially focused view when full keyboard access is off, resulting in nothing being focused since they're unfocusable. Fix this in general for all dialogs by checking for full keyboard access before returning a button in DialogDelegate::GetInitiallyFocusedView(), and fix the collected cookies dialog in particular by setting the initially focused view to its views::TreeView when full keyboard access is off. BUG=657850, 564912, 676244 ========== to ========== MacViews: Widgets focus first View in traversal order when initial focus fails. Support for full keyboard access in Mac for Views was implemented in r391744, but DialogDelegate still returns the dialog's default button as the View to focus on first opening the dialog regardless of whether full keyboard access is on or off. This results in buttons being returned as the initially focused view when full keyboard access is off, resulting in nothing being focused since they're unfocusable. Fix this in general for all views::Widgets trying to focus an unfocusable View - if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to become focused, manually advance focus on the focus manager instead. BUG=657850 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Karan, PTAL. https://codereview.chromium.org/2604303002/diff/20001/ui/views/controls/focus... File ui/views/controls/focus_ring.cc (left): https://codereview.chromium.org/2604303002/diff/20001/ui/views/controls/focus... ui/views/controls/focus_ring.cc:39: DCHECK(parent->HasFocus()); On 2016/12/30 21:16:04, karandeepb wrote: > If it's ok, can you send a separate CL for this, since this is unrelated? No worries, see https://codereview.chromium.org/2612463002/. I'll wait til that one lands before landing this one. https://codereview.chromium.org/2604303002/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2604303002/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:167: if (!dcv->GetFocusManager()->keyboard_accessible()) On 2016/12/30 21:16:04, karandeepb wrote: > null-check the focus manager? > > Did you test this on other platforms? keyboard accessibility is off/false on all > platforms except Mac. Hence won't this always return NULL on say Windows? > > We can #ifdef OS_MACOSX this, but I am not sure if this is the correct way to > solve this. (doesn't look generic enough). Another option may be to do something > more sensible in Widget::SetInitialFocus when the initially-focused-view > retrieved can't take focus. For example, what does FocusManager::AdvanceFocus > do, if the focus manager has no focused view? Oops, that's a mistake. I originally had the #ifdefs around this hunk but removed them for whatever reason. Your suggestion sounds reasonable though - looks like FocusManager::AdvanceFocus() works when focus is cleared. I've changed the fix to be in Widget::SetInitialFocus() instead.
LGTM. But please update the CL description to clarify that this affects all platforms, and as a result the bug on Mac is fixed. For example, maybe change the CL heading to something like- "views::Widget: Advance focus if WidgetDelegate::GetInitiallyFocusedView() returns an unfocusable view" https://codereview.chromium.org/2604303002/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2604303002/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_delegate.cc:167: if (!dcv->GetFocusManager()->keyboard_accessible()) On 2017/01/03 03:48:26, Patti Lor wrote: > On 2016/12/30 21:16:04, karandeepb wrote: > > null-check the focus manager? > > > > Did you test this on other platforms? keyboard accessibility is off/false on > all > > platforms except Mac. Hence won't this always return NULL on say Windows? > > > > We can #ifdef OS_MACOSX this, but I am not sure if this is the correct way to > > solve this. (doesn't look generic enough). Another option may be to do > something > > more sensible in Widget::SetInitialFocus when the initially-focused-view > > retrieved can't take focus. For example, what does FocusManager::AdvanceFocus > > do, if the focus manager has no focused view? > > Oops, that's a mistake. I originally had the #ifdefs around this hunk but > removed them for whatever reason. Your suggestion sounds reasonable though - > looks like FocusManager::AdvanceFocus() works when focus is cleared. I've > changed the fix to be in Widget::SetInitialFocus() instead. Yeah this seems reasonable to me as well, though owners might have a different take.
Description was changed from ========== MacViews: Widgets focus first View in traversal order when initial focus fails. Support for full keyboard access in Mac for Views was implemented in r391744, but DialogDelegate still returns the dialog's default button as the View to focus on first opening the dialog regardless of whether full keyboard access is on or off. This results in buttons being returned as the initially focused view when full keyboard access is off, resulting in nothing being focused since they're unfocusable. Fix this in general for all views::Widgets trying to focus an unfocusable View - if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to become focused, manually advance focus on the focus manager instead. BUG=657850 ========== to ========== (Mac)Views: Widgets focus first View in traversal order if initial focus fails. views::Widgets use SetInitialFocus(), which calls WidgetDelegate::GetInitiallyFocusedView() to set focus to a View on first opening the Widget. In cases where GetInitiallyFocusedView() returns an unfocusable View, the call to RequestFocus() silently fails and the Widget is opened without focus on anything. In particular, this is happening on MacViews when full keyboard access is turned off. Support for full keyboard access was implemented in r391744, but DialogDelegate::GetInitiallyFocusedView() still returns the dialog's default button regardless of full keyboard access. This results in the dialog opening with nothing focused. Fix this in general for all views::Widgets trying to focus an unfocusable View - if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to become focused, manually advance focus on the focus manager instead. BUG=657850 ==========
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, are you able to do owner's review for this change? Thanks.
This should have a test - something in dialog_delegate_unittest.cc is probably appropriate. https://codereview.chromium.org/2604303002/diff/40001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2604303002/diff/40001/ui/views/widget/widget.... ui/views/widget/widget.cc:1316: v->RequestFocus(); This is a bit inconsistent. View::RequestFocus() uses Widget::GetFocusManager() which may return something other than |focus_manager_|. Very little in Widget uses focus_manager_ directly - and it all has a comment. I think we want something at the top of the function like FocusManager* focus_manager = GetFocusManager() if (!focus_manager) return; the use on line 1311-2 can be updated too. https://codereview.chromium.org/2604303002/diff/40001/ui/views/widget/widget.... ui/views/widget/widget.cc:1319: if (focus_manager_ && v != focus_manager_->GetFocusedView()) Is it enough just to call focus_manager->AdvanceFocusIfNecessary()?
The CQ bit was checked by patricialor@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.
Thanks Trent, PTAL. https://codereview.chromium.org/2604303002/diff/40001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2604303002/diff/40001/ui/views/widget/widget.... ui/views/widget/widget.cc:1316: v->RequestFocus(); On 2017/01/04 03:59:38, tapted wrote: > This is a bit inconsistent. View::RequestFocus() uses Widget::GetFocusManager() > which may return something other than |focus_manager_|. Very little in Widget > uses focus_manager_ directly - and it all has a comment. I think we want > something at the top of the function like > > FocusManager* focus_manager = GetFocusManager() > if (!focus_manager) > return; > > the use on line 1311-2 can be updated too. Done. https://codereview.chromium.org/2604303002/diff/40001/ui/views/widget/widget.... ui/views/widget/widget.cc:1319: if (focus_manager_ && v != focus_manager_->GetFocusedView()) On 2017/01/04 03:59:38, tapted wrote: > Is it enough just to call focus_manager->AdvanceFocusIfNecessary()? No - AdvanceFocusIfNecessary is only meant to make sure no unfocusable views have focus. When the focus is cleared, that's considered a valid state and it doesn't do anything.
There's a few subtleties here - please loop in sky when it's ready (I feel as though I should keep my training wheels on for widget.cc changes ;). It all seems reasonable to me though. https://codereview.chromium.org/2604303002/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2604303002/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:1308: return false; oh drat - I missed that this early exit would change the return value for that codepath :/ I think HWNDMessageHandler::ShowWindowWithState(..) is the only thing using the return value. I don't know enough about that code to know if we can change the return value in this case. We probably shouldn't (but sky might know better) https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:52: bool ShouldShowCloseButton() const override { return false; } This makes the test a bit "unauthentic". That is, the real reason the changes is needed is that view that should be focused depends on the users's current `Full Keyboard Access` setting. That limits the options for fixing it (e.g. we can't just #ifdef(mac), and we don't want to litter knowledge of that setting throughout the codebase). I think we want something like #if defined(OS_MACOSX) #include "ui/base/test/scoped_fake_full_keyboard_access.h" #endif .. #if defined(OS_MACOSX) ScopedFakeFullKeyboardAccess::GetInstance()->set_full_keyboard_access_state(false); #endif (alternatively we could update ViewsTestBase with a method to tweak it and just have a no-op implementation in ViewsTestHelperAura, but I'm slightly in favour of the above for now) Of course then the test will have different results on other platforms. I think that's OK (more #ifdefs..) https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:289: dialog()->input()->SetFocusBehavior(View::FocusBehavior::NEVER); My approach here would be something like do this line _only_ on not-mac (and do it for the close button too). Then remove the ShouldShowCloseButton() override. That lets us check something on each platform -- the authentic scenario on Mac, and a contrived one on other platforms that gives us some cross-platform coverage.
The CQ bit was checked by patricialor@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@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.
Hi Trent, PTAL - I'm kind of doubtful about the changes to ViewsTestBase/etc, was this what you intended? https://codereview.chromium.org/2604303002/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2604303002/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:1308: return false; On 2017/01/06 11:45:15, tapted wrote: > oh drat - I missed that this early exit would change the return value for that > codepath :/ > > I think HWNDMessageHandler::ShowWindowWithState(..) is the only thing using the > return value. I don't know enough about that code to know if we can change the > return value in this case. We probably shouldn't (but sky might know better) Yeah, I read the comment description for this method and it says // Returns true if the initial focus has been set or the window should // not set the initial focus, or false if the caller should set the initial // focus (if any). Which kind of suggests that we shouldn't change it, but then I feel like it also suggests the "fallback" implemented in this patch shouldn't be in the scope of this method either. :/ I've reverted the early return here but will link sky these comments. https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:52: bool ShouldShowCloseButton() const override { return false; } On 2017/01/06 11:45:15, tapted wrote: > This makes the test a bit "unauthentic". That is, the real reason the changes is > needed is that view that should be focused depends on the users's current `Full > Keyboard Access` setting. That limits the options for fixing it (e.g. we can't > just #ifdef(mac), and we don't want to litter knowledge of that setting > throughout the codebase). > > I think we want something like > > #if defined(OS_MACOSX) > #include "ui/base/test/scoped_fake_full_keyboard_access.h" > #endif > > .. > > #if defined(OS_MACOSX) > ScopedFakeFullKeyboardAccess::GetInstance()->set_full_keyboard_access_state(false); > #endif > > (alternatively we could update ViewsTestBase with a method to tweak it and just > have a no-op implementation in ViewsTestHelperAura, but I'm slightly in favour > of the above for now) > > Of course then the test will have different results on other platforms. I think > that's OK (more #ifdefs..) Apparently you have to call [bridge_->ns_view() updateFullKeyboardAccess] after setting the full keyboard access state, according to NativeWidgetMacFullKeyboardAccessTest.FullKeyboardToggle. I think the reason for that was here: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_mac_unitte..., but you or Karan will have to confirm this. I had a look at your second suggestion of adding a method to ViewsTestBase, but it's really gross. There seems like there are too many layers between ViewsTestBase and ViewsTestHelper* - I had to add a method to ScopedViewsTestHelper which will call the proper implementation on ViewsTestHelperMac? I might be misunderstanding something though. I went ahead and did this anyway, but I'm not sure if you would prefer, something else, e.g. an approach similar to https://cs.chromium.org/chromium/src/ui/base/test/user_interactive_test_case.... https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:289: dialog()->input()->SetFocusBehavior(View::FocusBehavior::NEVER); On 2017/01/06 11:45:15, tapted wrote: > My approach here would be something like do this line _only_ on not-mac (and do > it for the close button too). Then remove the ShouldShowCloseButton() override. > That lets us check something on each platform -- the authentic scenario on Mac, > and a contrived one on other platforms that gives us some cross-platform > coverage. With keyboard-focus off on Mac, I think we don't need to worry about the close button now. It turns out the close button isn't part of the focus traversal order on non-Mac - I'm not sure why this is the case, any ideas? If that's not intentional, though, I'm not sure if using ShouldShowCloseButton() can be avoided, because there doesn't seem to be a way to retrieve the close button out of the dialog, AFAICT.
https://codereview.chromium.org/2604303002/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2604303002/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:1308: return false; On 2017/01/12 02:16:22, Patti Lor wrote: > On 2017/01/06 11:45:15, tapted wrote: > > oh drat - I missed that this early exit would change the return value for that > > codepath :/ > > > > I think HWNDMessageHandler::ShowWindowWithState(..) is the only thing using > the > > return value. I don't know enough about that code to know if we can change the > > return value in this case. We probably shouldn't (but sky might know better) > > Yeah, I read the comment description for this method and it says > > // Returns true if the initial focus has been set or the window should > // not set the initial focus, or false if the caller should set the initial > // focus (if any). > > Which kind of suggests that we shouldn't change it, but then I feel like it also > suggests the "fallback" implemented in this patch shouldn't be in the scope of > this method either. :/ I've reverted the early return here but will link sky > these comments. yep - this seems good/safest https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:289: dialog()->input()->SetFocusBehavior(View::FocusBehavior::NEVER); On 2017/01/12 02:16:22, Patti Lor wrote: > On 2017/01/06 11:45:15, tapted wrote: > > My approach here would be something like do this line _only_ on not-mac (and > do > > it for the close button too). Then remove the ShouldShowCloseButton() > override. > > That lets us check something on each platform -- the authentic scenario on > Mac, > > and a contrived one on other platforms that gives us some cross-platform > > coverage. > > With keyboard-focus off on Mac, I think we don't need to worry about the close > button now. It turns out the close button isn't part of the focus traversal > order on non-Mac - I'm not sure why this is the case, any ideas? yep - that makes sense - buttons are skipped with full keyboard access off, so that's the right behaviour. > If that's not > intentional, though, I'm not sure if using ShouldShowCloseButton() can be > avoided, because there doesn't seem to be a way to retrieve the close button out > of the dialog, AFAICT. ah, i see. Can we add #ifdefs around ShouldShowCloseButton() then, so it's only overridden on not-mac? (the override should have a comment too) https://codereview.chromium.org/2604303002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:296: SetFullKeyboardAccessState(dialog_widget, false); if you do this before creating the widget, I think you can avoid having to do updateFullKeyboardAccess (and, not have to pass in a pointer to widget -- i.e. just use ScopedFakeFullKeyboardAccess::GetInstance()->set_full_keyboard_access_state(false);
The CQ bit was checked by patricialor@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@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...
https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_delegate_unittest.cc:289: dialog()->input()->SetFocusBehavior(View::FocusBehavior::NEVER); On 2017/01/12 02:25:43, tapted wrote: > On 2017/01/12 02:16:22, Patti Lor wrote: > > On 2017/01/06 11:45:15, tapted wrote: > > > My approach here would be something like do this line _only_ on not-mac (and > > do > > > it for the close button too). Then remove the ShouldShowCloseButton() > > override. > > > That lets us check something on each platform -- the authentic scenario on > > Mac, > > > and a contrived one on other platforms that gives us some cross-platform > > > coverage. > > > > With keyboard-focus off on Mac, I think we don't need to worry about the close > > button now. It turns out the close button isn't part of the focus traversal > > order on non-Mac - I'm not sure why this is the case, any ideas? > > yep - that makes sense - buttons are skipped with full keyboard access off, so > that's the right behaviour. > > > If that's not > > intentional, though, I'm not sure if using ShouldShowCloseButton() can be > > avoided, because there doesn't seem to be a way to retrieve the close button > out > > of the dialog, AFAICT. > > ah, i see. Can we add #ifdefs around ShouldShowCloseButton() then, so it's only > overridden on not-mac? (the override should have a comment too) Sorry, by the close button I meant the little 'x' in the top-right. You can get to it pressing tab in MacViews but not on Linux (dunno about Windows, I'm assuming it's the same). Originally I put in the ShouldShowCloseButton() override because the behaviour was the same as Mac with keyboard access on, which would always hit the close button instead of |textfield|. But this test passes on non-Mac without hiding the 'x'. I wasn't sure if this was a bug. If yes, then we might need the ifdefs. https://codereview.chromium.org/2604303002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:296: SetFullKeyboardAccessState(dialog_widget, false); On 2017/01/12 02:25:43, tapted wrote: > if you do this before creating the widget, I think you can avoid having to do > updateFullKeyboardAccess (and, not have to pass in a pointer to widget -- i.e. > just use > ScopedFakeFullKeyboardAccess::GetInstance()->set_full_keyboard_access_state(false); Oh, that works! Thanks, reverted stuff back.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tapted@chromium.org changed reviewers: + sky@chromium.org
lgtm % nits. +sky to ensure the widget.cc changes (and general concept) are sensible https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:10: #include "ui/base/test/scoped_fake_full_keyboard_access.h" This should be between #if defined(OS_MACOSX) https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:279: // On Mac, make all buttons unfocusable by turning off full keyboard access. add "This is the more common configuration, and if a dialog has a focusable textfield, tree or table, that should obtain focus instead." https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:291: // For non-Mac, turn off focusability on all the dialog's buttons manually. add "This achieves the same effect as disabling full keyboard access."
General idea SGTM https://codereview.chromium.org/2604303002/diff/120001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2604303002/diff/120001/ui/views/widget/widget... ui/views/widget/widget.cc:1320: if (focus_manager && v != focus_manager->GetFocusedView()) Rather than checking |v|, how about checking for null? If focus ended up on some random view I think we shouldn't try to advance. https://codereview.chromium.org/2604303002/diff/120001/ui/views/widget/widget... ui/views/widget/widget.cc:1323: return !!v; The return value should be true if any view got focus.
The CQ bit was checked by patricialor@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 ========== (Mac)Views: Widgets focus first View in traversal order if initial focus fails. views::Widgets use SetInitialFocus(), which calls WidgetDelegate::GetInitiallyFocusedView() to set focus to a View on first opening the Widget. In cases where GetInitiallyFocusedView() returns an unfocusable View, the call to RequestFocus() silently fails and the Widget is opened without focus on anything. In particular, this is happening on MacViews when full keyboard access is turned off. Support for full keyboard access was implemented in r391744, but DialogDelegate::GetInitiallyFocusedView() still returns the dialog's default button regardless of full keyboard access. This results in the dialog opening with nothing focused. Fix this in general for all views::Widgets trying to focus an unfocusable View - if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to become focused, manually advance focus on the focus manager instead. BUG=657850 ========== to ========== (Mac)Views: Widgets focus first View in traversal order if initial focus fails. views::Widgets use SetInitialFocus(), which calls WidgetDelegate::GetInitiallyFocusedView() to set focus to a View on first opening the Widget. In cases where GetInitiallyFocusedView() returns an unfocusable View, the call to RequestFocus() silently fails and the Widget is opened without focus on anything. In particular, this is happening on MacViews when full keyboard access is turned off. Support for full keyboard access was implemented in r391744, but DialogDelegate::GetInitiallyFocusedView() still returns the dialog's default button regardless of full keyboard access. This results in the dialog opening with nothing focused. Fix this in general for all views::Widgets trying to focus an unfocusable View - if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to become focused and no other View has focus, manually advance focus on the focus manager instead. BUG=657850 TEST=On Mac with #secondary-ui-md flag enabled and full keyboard access turned off (System Preferences > Keyboard > Shortcuts, make sure 'All controls' radio button is not selected), open the collected cookies dialog by going to google.com, clicking 'Secure' on the left of the omnibox, and clicking 'X in use' under 'Cookies'. The TreeView should have focus. ==========
Thanks both! https://codereview.chromium.org/2604303002/diff/120001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2604303002/diff/120001/ui/views/widget/widget... ui/views/widget/widget.cc:1320: if (focus_manager && v != focus_manager->GetFocusedView()) On 2017/01/12 16:34:13, sky wrote: > Rather than checking |v|, how about checking for null? If focus ended up on some > random view I think we shouldn't try to advance. Done, thanks. https://codereview.chromium.org/2604303002/diff/120001/ui/views/widget/widget... ui/views/widget/widget.cc:1323: return !!v; On 2017/01/12 16:34:13, sky wrote: > The return value should be true if any view got focus. Done. https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... File ui/views/window/dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:10: #include "ui/base/test/scoped_fake_full_keyboard_access.h" On 2017/01/12 15:30:50, tapted wrote: > This should be between #if defined(OS_MACOSX) Done. https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:279: // On Mac, make all buttons unfocusable by turning off full keyboard access. On 2017/01/12 15:30:51, tapted wrote: > add "This is the more common configuration, and if a dialog has a focusable > textfield, tree or table, that should obtain focus instead." Done. https://codereview.chromium.org/2604303002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_delegate_unittest.cc:291: // For non-Mac, turn off focusability on all the dialog's buttons manually. On 2017/01/12 15:30:51, tapted wrote: > add "This achieves the same effect as disabling full keyboard access." Done.
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_...)
lgtm but wait for sky (I'm pretty sure that's what he had in mind though)
LGTM
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from karandeepb@chromium.org Link to the patchset: https://codereview.chromium.org/2604303002/#ps140001 (title: "Review comments.")
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
Internal error: failed to checkout. Please try again.
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484531312198440,
"parent_rev": "a8f5781bcfc7c13294895aa12205d81c3e102a82", "commit_rev":
"7d47709bf1d00beaa556804b25300ebdceecca07"}
Message was sent while issue was closed.
Description was changed from ========== (Mac)Views: Widgets focus first View in traversal order if initial focus fails. views::Widgets use SetInitialFocus(), which calls WidgetDelegate::GetInitiallyFocusedView() to set focus to a View on first opening the Widget. In cases where GetInitiallyFocusedView() returns an unfocusable View, the call to RequestFocus() silently fails and the Widget is opened without focus on anything. In particular, this is happening on MacViews when full keyboard access is turned off. Support for full keyboard access was implemented in r391744, but DialogDelegate::GetInitiallyFocusedView() still returns the dialog's default button regardless of full keyboard access. This results in the dialog opening with nothing focused. Fix this in general for all views::Widgets trying to focus an unfocusable View - if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to become focused and no other View has focus, manually advance focus on the focus manager instead. BUG=657850 TEST=On Mac with #secondary-ui-md flag enabled and full keyboard access turned off (System Preferences > Keyboard > Shortcuts, make sure 'All controls' radio button is not selected), open the collected cookies dialog by going to google.com, clicking 'Secure' on the left of the omnibox, and clicking 'X in use' under 'Cookies'. The TreeView should have focus. ========== to ========== (Mac)Views: Widgets focus first View in traversal order if initial focus fails. views::Widgets use SetInitialFocus(), which calls WidgetDelegate::GetInitiallyFocusedView() to set focus to a View on first opening the Widget. In cases where GetInitiallyFocusedView() returns an unfocusable View, the call to RequestFocus() silently fails and the Widget is opened without focus on anything. In particular, this is happening on MacViews when full keyboard access is turned off. Support for full keyboard access was implemented in r391744, but DialogDelegate::GetInitiallyFocusedView() still returns the dialog's default button regardless of full keyboard access. This results in the dialog opening with nothing focused. Fix this in general for all views::Widgets trying to focus an unfocusable View - if the View returned by WidgetDelegate::GetInitiallyFocusedView() isn't able to become focused and no other View has focus, manually advance focus on the focus manager instead. BUG=657850 TEST=On Mac with #secondary-ui-md flag enabled and full keyboard access turned off (System Preferences > Keyboard > Shortcuts, make sure 'All controls' radio button is not selected), open the collected cookies dialog by going to google.com, clicking 'Secure' on the left of the omnibox, and clicking 'X in use' under 'Cookies'. The TreeView should have focus. Review-Url: https://codereview.chromium.org/2604303002 Cr-Commit-Position: refs/heads/master@{#443827} Committed: https://chromium.googlesource.com/chromium/src/+/7d47709bf1d00beaa556804b2530... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7d47709bf1d00beaa556804b2530... |
