Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1305153004:
Focus comes on the 'close button' in find bar when selecting 'Enter' key. (Closed)
DescriptionFocus comes on the 'close button' in find bar when selecting 'Enter' key.
Now NotifyClick() takes ui::Event from CustomButton::OnKeyPressed() directly
instead of synthetic mouse event, So We need to change the foucs to text box when
user select 'Enter' from the keyboard when navigation key is focused in FindViewBar.
BUG=524373
Committed: https://crrev.com/8afc21e1982fa5f0d2a01d8a9c3a3e692d3f23d3
Cr-Commit-Position: refs/heads/master@{#346311}
Patch Set 1 #Patch Set 2 : Adding test case. #
Total comments: 16
Patch Set 3 : Changes as per review comments. #
Total comments: 12
Patch Set 4 : Changes as per review comments. #
Messages
Total messages: 14 (3 generated)
deepak.m1@samsung.com changed reviewers: + ajith.v@chromium.org
PTAL
ajith.v@chromium.org changed reviewers: + msw@chromium.org
peer review looks good to me!
On 2015/08/26 10:18:09, AKV wrote: > peer review looks good to me! @msw PTAL
https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:413: .key_code() == ui::VKEY_RETURN)) { The same defect occurs for the space key, and perhaps others, so this isn't complete, and likely the wrong fix. If we just want the old behavior, remove the |event| conditions and always focus the textfield. But I think it should do: A) If it's a mouse event, focus the textfield (as the code already did). B) If it's a key event (Enter, Space, whatever...) leave the focus on the same button (up or down), so users can focus the '^' (find previous) button and repeatedly hit [RETURN] or [SPACE] to repeatedly find previous instances of the search phrase. Otherwise, pressing a key to invoke '^' (find previous) will focus the textfield and the following [ENTER] key event will find *next* (switching direction from the last key press), or the following [SPACE] key press will replace the textfield text with a space character (unexpected). https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right): https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:99: GURL url = test_server()->GetURL(kSimplePage); nit: inline test_server()->GetURL(kSimplePage) in NavigateToURL below. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:102: chrome::FocusLocationBar(browser()); Focusing the location bar seems unnecessary here; remove this and the EXPECT_TRUE directly below. (otherwise fix the indentation here) https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:108: ui_test_utils::FindInPage( Fix indentation. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:112: ASSERT_TRUE(ui_test_utils::SendKeyPressSync( Add a similar check for the 'v' (previous) button (pressing tab again). https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:118: EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), Fix indentation... You'll also need to update this expectation if you fix the behavior as I suggested in the other file. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:120: } Please add a similar test for mouse events.
Thanks for review. PTAL https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:413: .key_code() == ui::VKEY_RETURN)) { On 2015/08/26 18:44:32, msw wrote: > The same defect occurs for the space key, and perhaps others, so this isn't > complete, and likely the wrong fix. If we just want the old behavior, remove the > |event| conditions and always focus the textfield. But I think it should do: > A) If it's a mouse event, focus the textfield (as the code already did). > B) If it's a key event (Enter, Space, whatever...) leave the focus on the same > button (up or down), so users can focus the '^' (find previous) button and > repeatedly hit [RETURN] or [SPACE] to repeatedly find previous instances of the > search phrase. Otherwise, pressing a key to invoke '^' (find previous) will > focus the textfield and the following [ENTER] key event will find *next* > (switching direction from the last key press), or the following [SPACE] key > press will replace the textfield text with a space character (unexpected). Thanks for detailed explanation. As the current issue have release blocker tag, so I am restoring it to the old behavior i.e moving the focus to text field for the mouse and key events. I will make separate patch for your suggestion for keeping the focus on the same button so that user's [Return] or [Space] key will search previous/next instance of the search phrase for key events. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right): https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:99: GURL url = test_server()->GetURL(kSimplePage); On 2015/08/26 18:44:32, msw wrote: > nit: inline test_server()->GetURL(kSimplePage) in NavigateToURL below. Done. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:102: chrome::FocusLocationBar(browser()); On 2015/08/26 18:44:32, msw wrote: > Focusing the location bar seems unnecessary here; remove this and the > EXPECT_TRUE directly below. (otherwise fix the indentation here) Done. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:108: ui_test_utils::FindInPage( On 2015/08/26 18:44:32, msw wrote: > Fix indentation. Done. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:112: ASSERT_TRUE(ui_test_utils::SendKeyPressSync( On 2015/08/26 18:44:32, msw wrote: > Add a similar check for the 'v' (previous) button (pressing tab again). Done. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:118: EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), On 2015/08/26 18:44:32, msw wrote: > Fix indentation... You'll also need to update this expectation if you fix the > behavior as I suggested in the other file. Done. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:120: } On 2015/08/26 18:44:32, msw wrote: > Please add a similar test for mouse events. I have tried to add mouse event test case, But for that I need to have '/\' and '\/' button position to send mouse click position. Currently these values are not available as these buttons are private in find_bar_view.h. I will add functions to get these while making clean up and making patch for your suggestion mentioned in other file.
https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:413: .key_code() == ui::VKEY_RETURN)) { On 2015/08/27 05:31:04, Deepak wrote: > On 2015/08/26 18:44:32, msw wrote: > > The same defect occurs for the space key, and perhaps others, so this isn't > > complete, and likely the wrong fix. If we just want the old behavior, remove > the > > |event| conditions and always focus the textfield. But I think it should do: > > A) If it's a mouse event, focus the textfield (as the code already did). > > B) If it's a key event (Enter, Space, whatever...) leave the focus on the same > > button (up or down), so users can focus the '^' (find previous) button and > > repeatedly hit [RETURN] or [SPACE] to repeatedly find previous instances of > the > > search phrase. Otherwise, pressing a key to invoke '^' (find previous) will > > focus the textfield and the following [ENTER] key event will find *next* > > (switching direction from the last key press), or the following [SPACE] key > > press will replace the textfield text with a space character (unexpected). > > Thanks for detailed explanation. As the current issue have release blocker tag, > so I am restoring it to the old behavior i.e moving the focus to text field for > the mouse and key events. > I will make separate patch for your suggestion for keeping the focus on the same > button so that user's [Return] or [Space] key will search previous/next instance > of the search phrase for key events. Acknowledged. https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right): https://codereview.chromium.org/1305153004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:120: } On 2015/08/27 05:31:04, Deepak wrote: > On 2015/08/26 18:44:32, msw wrote: > > Please add a similar test for mouse events. > > I have tried to add mouse event test case, But for that I need to have '/\' and > '\/' button position to send mouse click position. Currently these values are > not available as these buttons are private in find_bar_view.h. > I will add functions to get these while making clean up and making patch for > your suggestion mentioned in other file. Acknowledged. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:409: // NotifyClick() now takes ui::Event so event can be mouse event or the nit: There is no function named "NotifyClick". Consider removing the first sentence and fixing the grammar in the second sentence, so this comment just says: "Move focus to the find textfield." https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right): https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:101: chrome::FocusLocationBar(browser()); Remove this unnecessary step, as I previously asked. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:102: EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX)); Remove this unnecessary step, as I previously asked. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:103: // Ensure the creation of the find bar controller. Remove this comment (redundant with the one on line 100) https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:111: // Verifying that TextField will get focused when we select Enter key with nit: "// The textfield should be focused after pressing [Enter] on the find button." https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:120: // Verifying that TextField will get focused when we select Enter key with ditto nit: "// The textfield should be focused after pressing [Enter] on the find button."
@msw Thanks for review and providing comments. Changes done as suggested. PTAL https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:409: // NotifyClick() now takes ui::Event so event can be mouse event or the On 2015/08/27 18:09:02, msw wrote: > nit: There is no function named "NotifyClick". Consider removing the first > sentence and fixing the grammar in the second sentence, so this comment just > says: "Move focus to the find textfield." Done. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right): https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:101: chrome::FocusLocationBar(browser()); On 2015/08/27 18:09:02, msw wrote: > Remove this unnecessary step, as I previously asked. Done. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:102: EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX)); On 2015/08/27 18:09:02, msw wrote: > Remove this unnecessary step, as I previously asked. Done. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:103: // Ensure the creation of the find bar controller. On 2015/08/27 18:09:02, msw wrote: > Remove this comment (redundant with the one on line 100) Done. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:111: // Verifying that TextField will get focused when we select Enter key with On 2015/08/27 18:09:02, msw wrote: > nit: "// The textfield should be focused after pressing [Enter] on the find > button." Done. https://codereview.chromium.org/1305153004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:120: // Verifying that TextField will get focused when we select Enter key with On 2015/08/27 18:09:02, msw wrote: > ditto nit: "// The textfield should be focused after pressing [Enter] on the > find button." Done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305153004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305153004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8afc21e1982fa5f0d2a01d8a9c3a3e692d3f23d3 Cr-Commit-Position: refs/heads/master@{#346311} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
