Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
490443002:
Add test for ChromeVox keyboard commands. (Closed)
|
Created:
6 years, 4 months ago by dmazzoni Modified:
6 years, 3 months ago CC:
chromium-reviews, ben+aura_chromium.org, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, kalyank, sadrul, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd test for ChromeVox keyboard commands.
This is just a couple of quick sanity checks to prevent future regressions
like the one in the linked bug. It tests both a Search+Shift shortcut and
the corresponding Prefix Key shortcut.
Adds support for sending the meta modifier key in
aura/x11 tests.
BUG=404470
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291547
Committed: https://chromium.googlesource.com/chromium/src/+/8cab2737f1cb6e6011b0946f6bef5027868602c0
Committed: https://crrev.com/91703961c10dfa0f452999ed7cb87945ea817362
Cr-Commit-Position: refs/heads/master@{#293687}
Committed: https://crrev.com/15ea141544fedaf306c94890b5be19b7c5df885a
Cr-Commit-Position: refs/heads/master@{#296074}
Patch Set 1 #
Total comments: 5
Patch Set 2 : ASSERT_NO_FATAL_FAILURE #
Total comments: 4
Patch Set 3 : Add another test involving selection #Patch Set 4 : NULL window #Patch Set 5 : Rebase #Patch Set 6 : Get rid of outdated DCHECKs for command modifier #Patch Set 7 : Add ozone support #Patch Set 8 : Disable test on ozone for now #Patch Set 9 : Rebase #Patch Set 10 : Just for debugging #Patch Set 11 : Rebase #Patch Set 12 : Fix idea #Patch Set 13 : More debugging for trybots #Patch Set 14 : webkitHidden might be the issue? #Patch Set 15 : Test filter again #Patch Set 16 : Real fix that works #
Total comments: 2
Patch Set 17 : Add debugging back #Patch Set 18 : Fix flakiness #Patch Set 19 : Selection is causing other issues, skip that part of test for now #Patch Set 20 : Just 2 of 3 tests for now #Patch Set 21 : No changes to ui_controls_factory_aurax11 needed. #
Total comments: 5
Messages
Total messages: 41 (3 generated)
https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:70: void SendKeyPressWithControl(ui::KeyboardCode key) { You need to wrap calls to these in ASSERT_NO_FATAL_FAILURE. https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:72: ash::Shell::GetInstance()->GetPrimaryRootWindow(); Why is this sending to ash?
https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:70: void SendKeyPressWithControl(ui::KeyboardCode key) { On 2014/08/19 16:27:13, sky wrote: > You need to wrap calls to these in ASSERT_NO_FATAL_FAILURE. Done. https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:72: ash::Shell::GetInstance()->GetPrimaryRootWindow(); On 2014/08/19 16:27:13, sky wrote: > Why is this sending to ash? These are very high-level integration tests that are meant to simulate a user pressing keys, including system-level shortcuts. The keys need to get routed to whatever window has focus or whatever module would normally intercept that key, not necessarily to the main browser window.
lgtm The title doesn't seem to capture the actual fix being made to the x11 file. The ChromeVox regression seems just the manner in which we caught the regression. https://codereview.chromium.org/490443002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/490443002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:294: } Want to also add a test for search+shift+s as discussed? https://codereview.chromium.org/490443002/diff/20001/ui/aura/test/ui_controls... File ui/aura/test/ui_controls_factory_aurax11.cc (right): https://codereview.chromium.org/490443002/diff/20001/ui/aura/test/ui_controls... ui/aura/test/ui_controls_factory_aurax11.cc:97: UnmaskAndSetKeycodeThenSend(&xevent, Mod4Mask, XK_Super_L); Was this removed recently? If not, how did ChromeVox's search key detection work?
https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/490443002/diff/1/chrome/browser/chromeos/acce... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:72: ash::Shell::GetInstance()->GetPrimaryRootWindow(); On 2014/08/19 16:53:55, dmazzoni wrote: > On 2014/08/19 16:27:13, sky wrote: > > Why is this sending to ash? > > These are very high-level integration tests that are meant to simulate a user > pressing keys, including system-level shortcuts. The keys need to get routed to > whatever window has focus or whatever module would normally intercept that key, > not necessarily to the main browser window. If you're saying the window doesn't matter, then should it be removed? I suspect it's needed for some things, in which case you should pass in the right window.
On 2014/08/19 17:48:38, David Tseng wrote: > The title doesn't seem to capture the actual fix being made to the x11 file. The > ChromeVox regression seems just the manner in which we caught the regression. I updated the change description. Note that the x11 file is just a test helper.
https://codereview.chromium.org/490443002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/490443002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:294: } On 2014/08/19 17:48:38, David Tseng wrote: > Want to also add a test for search+shift+s as discussed? Done. https://codereview.chromium.org/490443002/diff/20001/ui/aura/test/ui_controls... File ui/aura/test/ui_controls_factory_aurax11.cc (right): https://codereview.chromium.org/490443002/diff/20001/ui/aura/test/ui_controls... ui/aura/test/ui_controls_factory_aurax11.cc:97: UnmaskAndSetKeycodeThenSend(&xevent, Mod4Mask, XK_Super_L); On 2014/08/19 17:48:38, David Tseng wrote: > Was this removed recently? If not, how did ChromeVox's search key detection > work? This is just a test helper. Tests that used this level of key event simulation never sent the meta key before.
On 2014/08/19 19:47:55, sky wrote: > If you're saying the window doesn't matter, then should it be removed? I suspect > it's needed for some things, in which case you should pass in the right window. I changed it to pass NULL instead of a window, is that okay? I didn't realize I could do that before.
I don't think NULL works everywhere.
It works for all of these tests. SendKeyPressNotifyWhenDone in ui_controls_factory_aurax11.cc ignores |window|. Maybe I don't understand - what did you have in mind? On Tue, Aug 19, 2014 at 2:56 PM, <sky@chromium.org> wrote: > I don't think NULL works everywhere. > > > https://codereview.chromium.org/490443002/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@sky note that the test is Chrome OS only - so NULL should be fine, right?
Sorry for the run around, LGTM
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/490443002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/490443002/100001
Message was sent while issue was closed.
Committed patchset #6 (100001) as 291547
Message was sent while issue was closed.
A revert of this CL (patchset #6) has been created in https://codereview.chromium.org/500833002/ by agable@chromium.org. The reason for reverting is: Broke interactive_ui_tests on chromium.chromiumos waterfall: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....
This was reverted due to a failure on ozone only. I made a similar fix to ui_controls_factory_ozone.cc and I'm going to try landing again.
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/490443002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 8cab2737f1cb6e6011b0946f6bef5027868602c0
Message was sent while issue was closed.
On 2014/09/04 00:24:14, I haz the power (commit-bot) wrote: > Committed patchset #7 (id:120001) as 8cab2737f1cb6e6011b0946f6bef5027868602c0 New tests are still failing on "Linux ChromiumOS Ozone Tests": http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/535283004/ by vadimsh@chromium.org. The reason for reverting is: Broke interactive_ui_tests on Linux ChromiumOS Ozone: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... Failing tests: ChromeVoxNavigateAndSelect_0 ChromeVoxPrefixKey_0 ChromeVoxPrefixKey_1 ChromeVoxShiftSearch_0 ChromeVoxShiftSearch_1 .
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/490443002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as ad123c3ac76fadc625a9dff66960743d0df253ba
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/550183002/ by arv@chromium.org. The reason for reverting is: Broke Linux ChromiumOS Tests (dbg)(3) http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes....
Patchset 8 (id:??) landed as https://crrev.com/3299e1cf26232b831441987ae90d442b21ab6def Cr-Commit-Position: refs/heads/master@{#293234}
Patchset 8 (id:??) landed as https://crrev.com/91703961c10dfa0f452999ed7cb87945ea817362 Cr-Commit-Position: refs/heads/master@{#293687}
Looks like the latest patchset still fails? https://codereview.chromium.org/490443002/diff/300001/ui/aura/test/ui_control... File ui/aura/test/ui_controls_factory_aurax11.cc (right): https://codereview.chromium.org/490443002/diff/300001/ui/aura/test/ui_control... ui/aura/test/ui_controls_factory_aurax11.cc:95: UnmaskAndSetKeycodeThenSend(&xevent, Mod4Mask, XK_Super_L); Could you explain this change? meta vs super_l and mod4? meta vs super_l did not make a difference for me.
All trybots pass now. I pulled out one of the three tests into a separate changelist because it was having strange failures on one of the trybots no matter what I did. I'm going to try to land that test separately. https://codereview.chromium.org/490443002/diff/300001/ui/aura/test/ui_control... File ui/aura/test/ui_controls_factory_aurax11.cc (right): https://codereview.chromium.org/490443002/diff/300001/ui/aura/test/ui_control... ui/aura/test/ui_controls_factory_aurax11.cc:95: UnmaskAndSetKeycodeThenSend(&xevent, Mod4Mask, XK_Super_L); On 2014/09/19 22:37:05, David Tseng wrote: > Could you explain this change? meta vs super_l and mod4? meta vs super_l did not > make a difference for me. You're right, they both work - I reverted the changes to this file.
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/490443002/390004
lgtm https://codereview.chromium.org/490443002/diff/390004/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc (right): https://codereview.chromium.org/490443002/diff/390004/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:111: // the page loading and the ChromeVox extension loading and fully Mention something about the content script being fully initialized here. https://codereview.chromium.org/490443002/diff/390004/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:131: while (speech_monitor_.GetNextUtterance() != "ready") { Should we sleep here inside the loop to avoid potentially hanging the machine? https://codereview.chromium.org/490443002/diff/390004/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:287: if (utterance == "Click me") Would be helpful to state why we're expecting this? (initial page focus)? https://codereview.chromium.org/490443002/diff/390004/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc:289: } Ditto. https://codereview.chromium.org/490443002/diff/390004/ui/aura/test/ui_control... File ui/aura/test/ui_controls_factory_ozone.cc (right): https://codereview.chromium.org/490443002/diff/390004/ui/aura/test/ui_control... ui/aura/test/ui_controls_factory_ozone.cc:58: PostKeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_LWIN, flags); Does it make sense to use meta or win consistently?
Message was sent while issue was closed.
Committed patchset #21 (id:390004) as a586f6cd22a074945b9c7241134f6bfbfb857393
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/15ea141544fedaf306c94890b5be19b7c5df885a Cr-Commit-Position: refs/heads/master@{#296074}
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:390004) has been created in https://codereview.chromium.org/603823002/ by jam@chromium.org. The reason for reverting is: flakes, see http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... failures: TestAsNormalAndGuestUser/SpokenFeedbackTest.ChromeVoxPrefixKey/0 TestAsNormalAndGuestUser/SpokenFeedbackTest.ChromeVoxPrefixKey/1 TestAsNormalAndGuestUser/SpokenFeedbackTest.ChromeVoxShiftSearch/1 TestAsNormalAndGuestUser/SpokenFeedbackTest.ChromeVoxShiftSearch/0 LoggedInSpokenFeedbackTest.AddBookmark. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
