Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398303002:
[Mac] Fix an ESC key issue with the find bar on Sierra (Closed)
|
Created:
4 years, 2 months ago by spqchan Modified:
4 years, 2 months ago Reviewers:
Robert Sesek CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Fix an ESC key issue with the find bar on Sierra
Prevent the window from handling the ESC key when the find bar text is
focused on Sierra.
BUG=628979
Committed: https://crrev.com/795ed3707fefc9d39d836545d860f216df620c66
Cr-Commit-Position: refs/heads/master@{#424279}
Patch Set 1 #Patch Set 2 : Wrote test #
Total comments: 2
Patch Set 3 : Fix for rsesek #
Messages
Total messages: 18 (7 generated)
Description was changed from ========== [Mac] Fix ESC issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG= ========== to ========== [Mac] Fix ESC issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG=628979 ==========
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
PTAL
Description was changed from ========== [Mac] Fix ESC issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG=628979 ========== to ========== [Mac] Fix an ESC key issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG=628979 ==========
Is it possible to write a regression test for this?
On 2016/10/07 16:59:09, Robert Sesek wrote: > Is it possible to write a regression test for this? I looked around and I don't see any similar the test. The most I can probably do is try to simulate a key down event for the ESC button and see if it closes. I'm not sure how I can test to see if AppKit ends up handling the ESC key. Let me know though if you have any suggestions!
On 2016/10/07 18:23:44, spqchan wrote: > On 2016/10/07 16:59:09, Robert Sesek wrote: > > Is it possible to write a regression test for this? > > I looked around and I don't see any similar the test. > The most I can probably do is try to simulate a key down event for the ESC > button and see if it closes. > I'm not sure how I can test to see if AppKit ends up handling the ESC key. Let > me know though if you have any suggestions! I was thinking an interactive_ui_test or brower_test that opens a browser window, puts it in fullscreen, opens the find bar, and sends an Esc key event. After which, the find bar should be closed and the window should still be fullscreen. (Wait, are all of our fullscreen tests flaky? Can't remember...)
On 2016/10/07 19:47:15, Robert Sesek wrote: > On 2016/10/07 18:23:44, spqchan wrote: > > On 2016/10/07 16:59:09, Robert Sesek wrote: > > > Is it possible to write a regression test for this? > > > > I looked around and I don't see any similar the test. > > The most I can probably do is try to simulate a key down event for the ESC > > button and see if it closes. > > I'm not sure how I can test to see if AppKit ends up handling the ESC key. Let > > me know though if you have any suggestions! > > I was thinking an interactive_ui_test or brower_test that opens a browser > window, puts it in fullscreen, opens the find bar, and sends an Esc key event. > After which, the find bar should be closed and the window should still be > fullscreen. (Wait, are all of our fullscreen tests flaky? Can't remember...) Ah so that's what you mean. Yeah, that would work. I'll look into this! A lot of them are flaky but I'm currently rewriting them as part of the fullscreen refactor =/
On 2016/10/07 20:52:16, spqchan wrote: > On 2016/10/07 19:47:15, Robert Sesek wrote: > > On 2016/10/07 18:23:44, spqchan wrote: > > > On 2016/10/07 16:59:09, Robert Sesek wrote: > > > > Is it possible to write a regression test for this? > > > > > > I looked around and I don't see any similar the test. > > > The most I can probably do is try to simulate a key down event for the ESC > > > button and see if it closes. > > > I'm not sure how I can test to see if AppKit ends up handling the ESC key. > Let > > > me know though if you have any suggestions! > > > > I was thinking an interactive_ui_test or brower_test that opens a browser > > window, puts it in fullscreen, opens the find bar, and sends an Esc key event. > > After which, the find bar should be closed and the window should still be > > fullscreen. (Wait, are all of our fullscreen tests flaky? Can't remember...) > > Ah so that's what you mean. Yeah, that would work. I'll look into this! > A lot of them are flaky but I'm currently rewriting them as part of the > fullscreen refactor =/ I added a test to find_bar_browsertest. Let me know if you prefer that I move it to interactive_ui_test. Thanks!
Awesome, thanks for the test! LGTM https://codereview.chromium.org/2398303002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/find_bar/find_bar_browsertest.mm (right): https://codereview.chromium.org/2398303002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/find_bar/find_bar_browsertest.mm:76: base::RunLoop runLoop; naming: run_loop
thanks! https://codereview.chromium.org/2398303002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/find_bar/find_bar_browsertest.mm (right): https://codereview.chromium.org/2398303002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/find_bar/find_bar_browsertest.mm:76: base::RunLoop runLoop; On 2016/10/10 22:19:13, Robert Sesek wrote: > naming: run_loop Done.
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2398303002/#ps40001 (title: "Fix for rsesek")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Mac] Fix an ESC key issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG=628979 ========== to ========== [Mac] Fix an ESC key issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG=628979 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Mac] Fix an ESC key issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG=628979 ========== to ========== [Mac] Fix an ESC key issue with the find bar on Sierra Prevent the window from handling the ESC key when the find bar text is focused on Sierra. BUG=628979 Committed: https://crrev.com/795ed3707fefc9d39d836545d860f216df620c66 Cr-Commit-Position: refs/heads/master@{#424279} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/795ed3707fefc9d39d836545d860f216df620c66 Cr-Commit-Position: refs/heads/master@{#424279} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
