Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785473003:
views::TableView - request focus on tap. (Closed)
|
Created:
3 years, 8 months ago by Evan Stade Modified:
3 years, 8 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews::TableView - request focus on tap.
This matches mouse presses.
BUG=705849
Review-Url: https://codereview.chromium.org/2785473003
Cr-Commit-Position: refs/heads/master@{#462309}
Committed: https://chromium.googlesource.com/chromium/src/+/f9ddb72fe527216f17d7dccbc83ee805d4e98ff3
Patch Set 1 #
Total comments: 3
Patch Set 2 : focus on tap down #Patch Set 3 : only on down #Patch Set 4 : focus even when you don't tap a particular row #Patch Set 5 : rebase #Messages
Total messages: 29 (19 generated)
estade@chromium.org changed reviewers: + sky@chromium.org, tdanderson@chromium.org
+tdanderson is this the behavior you'd expect for tap?
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... ui/views/controls/table/table_view.cc:412: RequestFocus(); Should we only request focus on ET_GESTURE_TAP_DOWN?
https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... ui/views/controls/table/table_view.cc:412: RequestFocus(); On 2017/03/28 17:41:24, sky wrote: > Should we only request focus on ET_GESTURE_TAP_DOWN? I am hoping Terry knows what the standard behavior for focusing on gestures is. I guessed that it should go here based on OnMousePressed.
https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... File ui/views/controls/table/table_view.cc (right): https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... ui/views/controls/table/table_view.cc:412: RequestFocus(); On 2017/03/28 17:43:17, Evan Stade wrote: > On 2017/03/28 17:41:24, sky wrote: > > Should we only request focus on ET_GESTURE_TAP_DOWN? > > I am hoping Terry knows what the standard behavior for focusing on gestures is. > I guessed that it should go here based on OnMousePressed. Doing a quick audit of other surfaces it doesn't look like we have a standard way of focusing with gestures. But for consistency with mouse behavior you should call RequestFocus() on ET_GESTURE_TAP_DOWN as sky@ suggests (and also call SetHandled() on the event in this case).
On 2017/03/28 21:26:22, tdanderson wrote: > https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... > File ui/views/controls/table/table_view.cc (right): > > https://codereview.chromium.org/2785473003/diff/1/ui/views/controls/table/tab... > ui/views/controls/table/table_view.cc:412: RequestFocus(); > On 2017/03/28 17:43:17, Evan Stade wrote: > > On 2017/03/28 17:41:24, sky wrote: > > > Should we only request focus on ET_GESTURE_TAP_DOWN? > > > > I am hoping Terry knows what the standard behavior for focusing on gestures > is. > > I guessed that it should go here based on OnMousePressed. > > Doing a quick audit of other surfaces it doesn't look like we have a standard > way of focusing with gestures. But for consistency with mouse behavior you > should call RequestFocus() on ET_GESTURE_TAP_DOWN as sky@ suggests (and also > call SetHandled() on the event in this case). updated. At first I tried the most literal interpretation of this suggestion, i.e. request focus on TAP_DOWN and leave TAP behavior alone. But that made the row selection jump on tap --- the TAP_DOWN focus made the first row get selected and the subsequent TAP changed the row selection. I thought it made more sense to do everything that the same time (request focus and set row selection), so I changed the behavior to do both things on TAP_DOWN. WDYT?
LGTM
The CQ bit was checked by estade@chromium.org to run a CQ dry run
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM
On 2017/03/29 21:39:48, sky wrote: > LGTM unit tests have some problems, and when I started updating the tests I found bugs unrelated to this CL. So this is now blocked on https://codereview.chromium.org/2780833004/
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2785473003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491438350871140,
"parent_rev": "16f6f2938e2ac13dd9a20e4a643ac6e90c03d0c9", "commit_rev":
"eb73636c7bf9919c4e8bc40a7e069c544c8235f1"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491438350871140,
"parent_rev": "19a6bb4bc5c4c5e7f5a878a1f45618f86218daca", "commit_rev":
"f9ddb72fe527216f17d7dccbc83ee805d4e98ff3"}
Message was sent while issue was closed.
Description was changed from ========== views::TableView - request focus on tap. This matches mouse presses. BUG=705849 ========== to ========== views::TableView - request focus on tap. This matches mouse presses. BUG=705849 Review-Url: https://codereview.chromium.org/2785473003 Cr-Commit-Position: refs/heads/master@{#462309} Committed: https://chromium.googlesource.com/chromium/src/+/f9ddb72fe527216f17d7dccbc83e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f9ddb72fe527216f17d7dccbc83e... |
