Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
420073003:
Two Finger Tap (Closed)
|
Created:
6 years, 4 months ago by lisayin Modified:
6 years, 4 months ago CC:
chromium-reviews, kalyank, stevenjb+watch_chromium.org, sadrul, oshima+watch_chromium.org, ben+ash_chromium.org, evy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionTwo Finger Tap
If spoken feedback is reading and a user performs a two
finger tap anywhere on the screen, it will silence spoken feedback.
BUG=399380
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289035
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed Comments and Added Tests #Patch Set 3 : Improved descriptions #
Total comments: 6
Patch Set 4 : Removed LOG(ERROR) line #Patch Set 5 : Rebase off master #
Total comments: 7
Patch Set 6 : Addressed Comments #Patch Set 7 : Added Slop Checking #Patch Set 8 : Fixed determining swipe gestures #
Total comments: 2
Patch Set 9 : Added more tests #
Total comments: 31
Patch Set 10 : Addressed Comments #
Total comments: 2
Patch Set 11 : Nit Fix #Patch Set 12 : Rebase off Master #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/420073003/diff/1/ash/ash_touch_exploration_ma... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/420073003/diff/1/ash/ash_touch_exploration_ma... ash/ash_touch_exploration_manager_chromeos.cc:62: void AshTouchExplorationManager::SilenceSpokenFeedback() { This is the code that accelerator_controller uses to silence spoken feedback. I'm not sure why this only pauses spoken feedback. However, I did notice that when spoken feedback was silenced natively using control, it had the same issues where it only paused it and skipped over a few words before continuing spoken feedback.
So I take it the ctrl key events are a stop gap until you figure out what's going on with the SilenceSpokenFeedback() method? https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:576: if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_MOVED) { How could you get ET_TOUCH_MOVED here if current_touch_ids_.size() == 0? https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:1548: EXPECT_TRUE(IsInTwoFingerTapState()); Test for the kTwoFingerTap timeout elapsing?
On 2014/07/31 16:49:26, aboxhall wrote: > So I take it the ctrl key events are a stop gap until you figure out what's > going on with the SilenceSpokenFeedback() method? > > https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... > File ui/chromeos/touch_exploration_controller.cc (right): > > https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... > ui/chromeos/touch_exploration_controller.cc:576: if (type == > ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_MOVED) { > How could you get ET_TOUCH_MOVED here if current_touch_ids_.size() == 0? > > https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... > File ui/chromeos/touch_exploration_controller_unittest.cc (right): > > https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... > ui/chromeos/touch_exploration_controller_unittest.cc:1548: > EXPECT_TRUE(IsInTwoFingerTapState()); > Test for the kTwoFingerTap timeout elapsing? Yeah. I'm going to ask Dominic about the weird incongruities between how native Chrome OS and chromevox handles key events and processes silencing spoken feedback.
https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:576: if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_MOVED) { On 2014/07/31 16:49:26, aboxhall wrote: > How could you get ET_TOUCH_MOVED here if current_touch_ids_.size() == 0? I moved up the current_touch_ids_.size() and forgot to remove the second check. Will change.
https://codereview.chromium.org/420073003/diff/40001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/420073003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.cc:429: LOG(ERROR) << "In HandleSilenceSpokenFeedback"; Delete this? https://codereview.chromium.org/420073003/diff/40001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/420073003/diff/40001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:38: virtual void SilenceSpokenFeedback() OVERRIDE; You're not actually using this, right? You're just sending a control key now. https://codereview.chromium.org/420073003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:568: state_ = WAIT_FOR_ONE_FINGER; Do we really want to wait for one finger? I'd prefer we avoid situations were a user could do some action for a long time, then release fingers and suddenly end up entering touch exploration. I think we should either enter touch exploration mode early, or never.
https://codereview.chromium.org/420073003/diff/40001/ash/ash_touch_exploratio... File ash/ash_touch_exploration_manager_chromeos.h (right): https://codereview.chromium.org/420073003/diff/40001/ash/ash_touch_exploratio... ash/ash_touch_exploration_manager_chromeos.h:38: virtual void SilenceSpokenFeedback() OVERRIDE; On 2014/07/31 20:44:43, dmazzoni wrote: > You're not actually using this, right? You're just sending a control key now. Yeah. It's set up to send it natively though once it works. https://codereview.chromium.org/420073003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:568: state_ = WAIT_FOR_ONE_FINGER; On 2014/07/31 20:44:43, dmazzoni wrote: > Do we really want to wait for one finger? > > I'd prefer we avoid situations were a user could do some action for a long time, > then release fingers and suddenly end up entering touch exploration. I think we > should either enter touch exploration mode early, or never. I agree, but Evy is writing WAIT_FOR_NO_FINGERS_DOWN so I don't want to make another method that conflicts with that one.
https://codereview.chromium.org/420073003/diff/40001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/420073003/diff/40001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.cc:429: LOG(ERROR) << "In HandleSilenceSpokenFeedback"; On 2014/07/31 20:44:43, dmazzoni_ooo_until_aug_11 wrote: > Delete this? Done.
https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:180: return EVENT_REWRITE_DISCARD; Does this have any implications for multi-finger swipes? https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:567: if (type == ui::ET_TOUCH_PRESSED) { And, does this have any implications for multi-finger swipes? https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:144: // Once touch exploration mode has been activated, Not from this change, but this paragraph should probably be moved higher up, with the rest of the touch exploration mode information. https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1557: ASSERT_EQ(2U, captured_events.size()); Can you assert what these events were? They should both be key events, right? You can assert IsKeyEvent and then static_cast to ui::KeyEvent and check what the actual keys pressed were.
https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:567: if (type == ui::ET_TOUCH_PRESSED) { On 2014/08/06 18:30:00, aboxhall wrote: > And, does this have any implications for multi-finger swipes? Yeah, whoever finishes last gets to add the change :) We're thinking that if it leaves the slop before a tap timer fire, it will then go into gesture mode. If a third finger is added, we can go into gesture mode too, since there's nothing else that would lead to at this point.
https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:180: return EVENT_REWRITE_DISCARD; On 2014/08/06 18:30:00, aboxhall wrote: > Does this have any implications for multi-finger swipes? Once we need to merge the two, we'll have a slop check. https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/420073003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:144: // Once touch exploration mode has been activated, On 2014/08/06 18:30:01, aboxhall wrote: > Not from this change, but this paragraph should probably be moved higher up, > with the rest of the touch exploration mode information. Done.
lgtm
https://codereview.chromium.org/420073003/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:580: // movement a swipe. Does this logic need extra testing?
https://codereview.chromium.org/420073003/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:580: // movement a swipe. On 2014/08/07 22:45:19, aboxhall wrote: > Does this logic need extra testing? Done.
+jamescook PTAL I added methods in the ash files so that in the future, we can switch to handling the events natively rather than through forced key presses. Currently, the code to silence ChromeVox natively is buggy, so until that is fixed, touch exploration will be sending key events.
Also, please manually wrap the commit description to 80 columns -- it makes the codereview page easier to read. https://codereview.chromium.org/420073003/diff/160001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/420073003/diff/160001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.cc:7: #include "ash/accelerators/accelerator_controller.h" Is this used? https://codereview.chromium.org/420073003/diff/160001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.cc:14: #include "base/metrics/user_metrics.h" Is this used? https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:596: DispatchEvent(&control_down); I'm not that familiar with a11y -- maybe a comment here explaining why toggling the control key down and up is a desirable thing to do? To the naive reader that seems like a no-op. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:860: initial_presses_.clear(); Hooray for clearing out data structures! https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:123: // Once touch exploration mode has been activated, nit: rewrap comment https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:371: // Map of touch ids to where its initial press occurred. Document the coordinate system of the Point. Relative to the whole screen? The current window? https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1541: TEST_F(TouchExplorationTest, TwoFingerTap) { Comment above what you are testing and why. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1557: ASSERT_EQ(2U, captured_events.size()); Probably EXPECT_EQ is better -- the test should fail, not crash, if this isn't true. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1560: TEST_F(TouchExplorationTest, TwoFingerTapWithDelay) { Comment please. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1580: ASSERT_EQ(2U, captured_events.size()); EXPECT_EQ I think https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1588: simulated_clock_->Advance(base::TimeDelta::FromMilliseconds(100)); I would use a much larger delay (like 1000) just in case the original delay is adjusted slightly. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1593: ASSERT_EQ(0U, captured_events.size()); EXPECT_EQ ? https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1596: TEST_F(TouchExplorationTest, TwoFingerTapAndHold) { Comment please https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1613: TEST_F(TouchExplorationTest, TwoFingerTapAndMoveFirstFinger) { ditto https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1619: ui::ET_TOUCH_PRESSED, gfx::Point(100, 200), 1, Now()); I like how you use 100 vs. 200 for x vs. y -- much clearer than 100 for both. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1640: generator_->Dispatch(&first_press_id_1); Comment each of these little sections of code with what you are testing. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1657: TEST_F(TouchExplorationTest, TwoFingerTapAndMoveSecondFinger) { comment
https://codereview.chromium.org/420073003/diff/160001/ash/ash_touch_explorati... File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/420073003/diff/160001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.cc:7: #include "ash/accelerators/accelerator_controller.h" On 2014/08/08 18:06:30, James Cook wrote: > Is this used? Removed. https://codereview.chromium.org/420073003/diff/160001/ash/ash_touch_explorati... ash/ash_touch_exploration_manager_chromeos.cc:14: #include "base/metrics/user_metrics.h" On 2014/08/08 18:06:30, James Cook wrote: > Is this used? Removed. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:596: DispatchEvent(&control_down); On 2014/08/08 18:06:30, James Cook wrote: > I'm not that familiar with a11y -- maybe a comment here explaining why toggling > the control key down and up is a desirable thing to do? To the naive reader > that seems like a no-op. Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:860: initial_presses_.clear(); On 2014/08/08 18:06:30, James Cook wrote: > Hooray for clearing out data structures! :) https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:123: // Once touch exploration mode has been activated, On 2014/08/08 18:06:30, James Cook wrote: > nit: rewrap comment Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:371: // Map of touch ids to where its initial press occurred. On 2014/08/08 18:06:30, James Cook wrote: > Document the coordinate system of the Point. Relative to the whole screen? The > current window? Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1541: TEST_F(TouchExplorationTest, TwoFingerTap) { On 2014/08/08 18:06:31, James Cook wrote: > Comment above what you are testing and why. Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1557: ASSERT_EQ(2U, captured_events.size()); On 2014/08/08 18:06:30, James Cook wrote: > Probably EXPECT_EQ is better -- the test should fail, not crash, if this isn't > true. Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1560: TEST_F(TouchExplorationTest, TwoFingerTapWithDelay) { On 2014/08/08 18:06:31, James Cook wrote: > Comment please. Removed the timing check for two finger tap, so this test is obsolete. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1596: TEST_F(TouchExplorationTest, TwoFingerTapAndHold) { On 2014/08/08 18:06:30, James Cook wrote: > Comment please Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1613: TEST_F(TouchExplorationTest, TwoFingerTapAndMoveFirstFinger) { On 2014/08/08 18:06:31, James Cook wrote: > ditto Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1619: ui::ET_TOUCH_PRESSED, gfx::Point(100, 200), 1, Now()); On 2014/08/08 18:06:31, James Cook wrote: > I like how you use 100 vs. 200 for x vs. y -- much clearer than 100 for both. Acknowledged. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1640: generator_->Dispatch(&first_press_id_1); On 2014/08/08 18:06:31, James Cook wrote: > Comment each of these little sections of code with what you are testing. Done. https://codereview.chromium.org/420073003/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1657: TEST_F(TouchExplorationTest, TwoFingerTapAndMoveSecondFinger) { On 2014/08/08 18:06:30, James Cook wrote: > comment Done.
LGTM with nit https://codereview.chromium.org/420073003/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/420073003/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:371: // Map of touch ids to where its initial press occured relative to the screen. occured -> occurred
https://codereview.chromium.org/420073003/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/420073003/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:371: // Map of touch ids to where its initial press occured relative to the screen. On 2014/08/08 20:07:23, James Cook wrote: > occured -> occurred Done.
lgtm
The CQ bit was checked by lisayin@chromium.org
The CQ bit was unchecked by lisayin@chromium.org
The CQ bit was checked by lisayin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/420073003/220001
Message was sent while issue was closed.
Change committed as 289035 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
