|
|
DescriptionViews: Don't advance focus in Widgets when the stored focus view is not null.
r443827 introduced behavior where if the initially focused View failed to
receive focus, the Widget would manually advance focus to the first View in the
traversal order. However, this introduced a bug where if there was a stored
focus view in the FocusManager, it would advance the focus anyway.
Fix this by checking the FocusManager's stored focus view as well.
BUG=682157
Review-Url: https://codereview.chromium.org/2684403002
Cr-Commit-Position: refs/heads/master@{#451536}
Committed: https://chromium.googlesource.com/chromium/src/+/3033684819d40bb35117382a522ce95c98262de8
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add a test. #Patch Set 3 : Rebase? #Patch Set 4 : Check Widget active state instead of StoredFocusView. #
Messages
Total messages: 32 (24 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
patricialor@chromium.org changed reviewers: + sky@chromium.org
Hi sky@, please take a look, see https://codereview.chromium.org/2604303002/ for the CL that this one is following up.
https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc#n... ui/views/widget/widget.cc:1311: focus_manager->GetStoredFocusView() == nullptr) { Can you clarify why we end up here? I'm specifically wondering why the RequestFocus() on 1307 fails? If the request focus fails, then we should use the stored focus view if null and if focus is till null, then try to advance focus. Please add test coverage of this.
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, 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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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.
Apologies for the delay, I had a bit of trouble with the trybots. https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc#n... ui/views/widget/widget.cc:1311: focus_manager->GetStoredFocusView() == nullptr) { On 2017/02/10 18:18:18, sky wrote: > Can you clarify why we end up here? I'm specifically wondering why the > RequestFocus() on 1307 fails? If the request focus fails, then we should use the > stored focus view if null and if focus is till null, then try to advance focus. > Please add test coverage of this. The RequestFocus() call doesn't really fail, it just got called while the Widget was deactivated. When that's the case, FocusManager::SetFocusedViewWithReason() stores the focused view instead so that it can be focused later when the Widget becomes activated. But my previous change (r443827) didn't account for this scenario, so it would advance the focus anyway to the next View. All of this happens while the Widget is still deactivated, so the focus manager gets updated with an incorrect stored focus view. The change I made should do what you've suggested (use the originally stored focus view on Widget activation).
https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc#n... ui/views/widget/widget.cc:1311: focus_manager->GetStoredFocusView() == nullptr) { On 2017/02/15 03:15:21, Patti Lor wrote: > On 2017/02/10 18:18:18, sky wrote: > > Can you clarify why we end up here? I'm specifically wondering why the > > RequestFocus() on 1307 fails? If the request focus fails, then we should use > the > > stored focus view if null and if focus is till null, then try to advance > focus. > > Please add test coverage of this. > > The RequestFocus() call doesn't really fail, it just got called while the Widget > was deactivated. When that's the case, FocusManager::SetFocusedViewWithReason() > stores the focused view instead so that it can be focused later when the Widget > becomes activated. But my previous change (r443827) didn't account for this > scenario, so it would advance the focus anyway to the next View. All of this > happens while the Widget is still deactivated, so the focus manager gets updated > with an incorrect stored focus view. > > The change I made should do what you've suggested (use the originally stored > focus view on Widget activation). In that case can you check the active status rather than the stored focused view? Checking the 'stored focus view' which is a side effect of not being active is subtle and easily missed. Also, please update the comment above.
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Thanks, PTAL https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/2684403002/diff/1/ui/views/widget/widget.cc#n... ui/views/widget/widget.cc:1311: focus_manager->GetStoredFocusView() == nullptr) { On 2017/02/15 16:28:28, sky wrote: > On 2017/02/15 03:15:21, Patti Lor wrote: > > On 2017/02/10 18:18:18, sky wrote: > > > Can you clarify why we end up here? I'm specifically wondering why the > > > RequestFocus() on 1307 fails? If the request focus fails, then we should use > > the > > > stored focus view if null and if focus is till null, then try to advance > > focus. > > > Please add test coverage of this. > > > > The RequestFocus() call doesn't really fail, it just got called while the > Widget > > was deactivated. When that's the case, > FocusManager::SetFocusedViewWithReason() > > stores the focused view instead so that it can be focused later when the > Widget > > becomes activated. But my previous change (r443827) didn't account for this > > scenario, so it would advance the focus anyway to the next View. All of this > > happens while the Widget is still deactivated, so the focus manager gets > updated > > with an incorrect stored focus view. > > > > The change I made should do what you've suggested (use the originally stored > > focus view on Widget activation). > > In that case can you check the active status rather than the stored focused > view? Checking the 'stored focus view' which is a side effect of not being > active is subtle and easily missed. Also, please update the comment above. Done.
LGTM
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": 60001, "attempt_start_ts": 1487545819759510, "parent_rev": "747aab0de56d72b41287c65befbee89dfa433739", "commit_rev": "3033684819d40bb35117382a522ce95c98262de8"}
Message was sent while issue was closed.
Description was changed from ========== Views: Don't advance focus in Widgets when the stored focus view is not null. r443827 introduced behavior where if the initially focused View failed to receive focus, the Widget would manually advance focus to the first View in the traversal order. However, this introduced a bug where if there was a stored focus view in the FocusManager, it would advance the focus anyway. Fix this by checking the FocusManager's stored focus view as well. BUG=682157 ========== to ========== Views: Don't advance focus in Widgets when the stored focus view is not null. r443827 introduced behavior where if the initially focused View failed to receive focus, the Widget would manually advance focus to the first View in the traversal order. However, this introduced a bug where if there was a stored focus view in the FocusManager, it would advance the focus anyway. Fix this by checking the FocusManager's stored focus view as well. BUG=682157 Review-Url: https://codereview.chromium.org/2684403002 Cr-Commit-Position: refs/heads/master@{#451536} Committed: https://chromium.googlesource.com/chromium/src/+/3033684819d40bb35117382a522c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3033684819d40bb35117382a522c... |