Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2078953003:
Remove find bar focus redirection on mouse events. (Closed)
|
Created:
4 years, 6 months ago by msw Modified:
4 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove find bar focus redirection on mouse events.
Pressing next/previous buttons shouldn't change focus.
Focus was redirected to the textfield for Issue 524373.
That defect is no longer reproducible on ToT @ #400257.
Update and enable the interactive ui test.
BUG=621050, 584043, 524373
TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button.
R=pkasting@chromium.org
Committed: https://crrev.com/098645c6ee78757f03819207645ff151b8edf01d
Cr-Commit-Position: refs/heads/master@{#401079}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove focus redirection altogether; update FindInPageTest. #
Total comments: 2
Patch Set 3 : Remove const. #Patch Set 4 : Got windows test passing with workaround; exploring options on other platform. #Patch Set 5 : Cleanup. #Patch Set 6 : Use OnMousePressed and OnMouseReleased on all platforms. #Patch Set 7 : Add tap testing; make test more rigorous. #
Messages
Total messages: 36 (17 generated)
Hey Peter, please take a look; thanks!
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078953003/1
LGTM. Bonus if there's a framework that lets us fake touch events so this can be unittested. https://codereview.chromium.org/2078953003/diff/1/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2078953003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:431: // That lets the user continuously press SPACE/ENTER on next/previous. Nit: We might want to note why we'd want to move focus back to the textfield on any events to begin with.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix find bar touch focus issue. Move focus to the find bar textfield on next/previous button touch. This restores the behavior prior to a recent change: https://codereview.chromium.org/1660273003 (meant to keep focus on next/previous when pressing [SPACE]/[ENTER]) BUG=621050 TEST='chrome://version', CTRL+F, 'g', TAB, touch next button; focus moves to find text (not previous button). R=pkasting@chromium.org ========== to ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update the interactive ui test. BUG=621050 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ==========
Description was changed from ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update the interactive ui test. BUG=621050 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ========== to ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update the interactive ui test. BUG=621050,584043,524373 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ==========
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078953003/20001
Hey Peter, please take another look; thanks! (I'll try adding a touch test, perhaps as a followup) https://codereview.chromium.org/2078953003/diff/1/chrome/browser/ui/views/fin... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2078953003/diff/1/chrome/browser/ui/views/fin... chrome/browser/ui/views/find_bar_view.cc:431: // That lets the user continuously press SPACE/ENTER on next/previous. On 2016/06/17 20:55:47, Peter Kasting wrote: > Nit: We might want to note why we'd want to move focus back to the textfield on > any events to begin with. As discussed, I'm removing this workaround added for http://crbug.com/524373, which no longer repros anyway. There's no reason to explicitly change focus.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM https://codereview.chromium.org/2078953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right): https://codereview.chromium.org/2078953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:54: FindBarView* GetFindBarView() const { Nit: Make pointer const or remove const qualifier
Patchset #3 (id:40001) has been deleted
I'll have to look at the win test failures next week... sigh. https://codereview.chromium.org/2078953003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_views_interactive_uitest.cc (right): https://codereview.chromium.org/2078953003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_views_interactive_uitest.cc:54: FindBarView* GetFindBarView() const { On 2016/06/18 01:13:25, Peter Kasting wrote: > Nit: Make pointer const or remove const qualifier Done.
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078953003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update the interactive ui test. BUG=621050,584043,524373 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ========== to ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update and enable the interactive ui test. BUG=621050,584043,524373 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ==========
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by msw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078953003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey Peter, please take another look; thanks!
The latest changes mostly just make the test more rigorous, landing.
The CQ bit was checked by msw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2078953003/#ps160001 (title: "Add tap testing; make test more rigorous.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078953003/160001
Message was sent while issue was closed.
Description was changed from ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update and enable the interactive ui test. BUG=621050,584043,524373 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ========== to ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update and enable the interactive ui test. BUG=621050,584043,524373 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update and enable the interactive ui test. BUG=621050,584043,524373 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org ========== to ========== Remove find bar focus redirection on mouse events. Pressing next/previous buttons shouldn't change focus. Focus was redirected to the textfield for Issue 524373. That defect is no longer reproducible on ToT @ #400257. Update and enable the interactive ui test. BUG=621050,584043,524373 TEST=CTRL+F, 'a', TAB to focus previous button, touch/click next button, focus remains on previous button. R=pkasting@chromium.org Committed: https://crrev.com/098645c6ee78757f03819207645ff151b8edf01d Cr-Commit-Position: refs/heads/master@{#401079} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/098645c6ee78757f03819207645ff151b8edf01d Cr-Commit-Position: refs/heads/master@{#401079}
Message was sent while issue was closed.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
