Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(314)

Issue 2684403002: Views: Don't advance focus in Widgets when the stored focus view is not null. (Closed)

Created:
3 years, 10 months ago by Patti Lor
Modified:
3 years, 10 months ago
Reviewers:
sky
CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -3 lines) Patch
M ui/views/widget/widget.cc View 1 2 3 1 chunk +5 lines, -2 lines 0 comments Download
M ui/views/window/dialog_delegate_unittest.cc View 1 2 chunks +39 lines, -1 line 0 comments Download

Messages

Total messages: 32 (24 generated)
Patti Lor
Hi sky@, please take a look, see https://codereview.chromium.org/2604303002/ for the CL that this one is ...
3 years, 10 months ago (2017-02-10 06:10:35 UTC) #6
sky
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#newcode1311 ui/views/widget/widget.cc:1311: focus_manager->GetStoredFocusView() == nullptr) { Can you clarify why we ...
3 years, 10 months ago (2017-02-10 18:18:18 UTC) #7
Patti Lor
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 ...
3 years, 10 months ago (2017-02-15 03:15:21 UTC) #20
sky
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#newcode1311 ui/views/widget/widget.cc:1311: focus_manager->GetStoredFocusView() == nullptr) { On 2017/02/15 03:15:21, Patti Lor ...
3 years, 10 months ago (2017-02-15 16:28:28 UTC) #21
Patti Lor
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#newcode1311 ui/views/widget/widget.cc:1311: focus_manager->GetStoredFocusView() == nullptr) { On 2017/02/15 16:28:28, ...
3 years, 10 months ago (2017-02-16 22:58:49 UTC) #26
sky
LGTM
3 years, 10 months ago (2017-02-17 16:47:26 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2684403002/60001
3 years, 10 months ago (2017-02-19 23:10:27 UTC) #29
commit-bot: I haz the power
3 years, 10 months ago (2017-02-20 00:58:41 UTC) #32
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3033684819d40bb35117382a522c...

Powered by Google App Engine
This is Rietveld 408576698