Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805623003:
Move the clear button to the right (Closed)
DescriptionMove the clear button to the right and reduce its touch target to avoid
accidential clicks when the user actually wanted to press the back or home button.
https://screenshot.googleplex.com/evyLBUaowaG.png
BUG=681523
Review-Url: https://codereview.chromium.org/2805623003
Cr-Commit-Position: refs/heads/master@{#462803}
Committed: https://chromium.googlesource.com/chromium/src/+/ea3bd60b2f5f5b394bde861c15d3c865e47caf87
Patch Set 1 #
Total comments: 1
Patch Set 2 : increase button height #
Depends on Patchset: Messages
Total messages: 14 (5 generated)
Description was changed from ========== Move the clear button to the right and reduce its touch target to avoid accidential clicks when the user actually wanted to press the back or home button. https://screenshot.googleplex.com/evyLBUaowaG.png BUG=681523 ========== to ========== Move the clear button to the right and reduce its touch target to avoid accidential clicks when the user actually wanted to press the back or home button. https://screenshot.googleplex.com/evyLBUaowaG.png BUG=681523 ==========
dullweber@chromium.org changed reviewers: + twellington@chromium.org
twellington@chromium.org: I have another small cl with a request from Max. I also tested the code with RTL turned on and it looks fine.
https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/clear_browsing_data_tab_content.xml (right): https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:29: android:minHeight="36dp" Did you run into issues where you accidentally tapped the clear data button when trying to access the Android controls? It's a bit unconventional to add top/bottom margin to reduce the touch target. Typically a button's touch target gets the full height of the View its displayed in.
On 2017/04/06 15:08:05, Theresa wrote: > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > File chrome/android/java/res/layout/clear_browsing_data_tab_content.xml (right): > > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:29: > android:minHeight="36dp" > Did you run into issues where you accidentally tapped the clear data button when > trying to access the Android controls? It's a bit unconventional to add > top/bottom margin to reduce the touch target. Typically a button's touch target > gets the full height of the View its displayed in. I didn't have any problem with it but Max suggested that it could be a problem. The specs for this are here: https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20V2/Specs#%2FSpe.... I can change it to full height and talk to him about this.
On 2017/04/06 15:37:18, dullweber wrote: > On 2017/04/06 15:08:05, Theresa wrote: > > > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > > File chrome/android/java/res/layout/clear_browsing_data_tab_content.xml > (right): > > > > > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > > chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:29: > > android:minHeight="36dp" > > Did you run into issues where you accidentally tapped the clear data button > when > > trying to access the Android controls? It's a bit unconventional to add > > top/bottom margin to reduce the touch target. Typically a button's touch > target > > gets the full height of the View its displayed in. > > I didn't have any problem with it but Max suggested that it could be a problem. > The specs for this are here: > https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20V2/Specs#%2FSpe.... > I can change it to full height and talk to him about this. The minimum Android touch target is 48dp for accessibility, so the spec'ed 36dp height is in direct contrast to that. Will you please discuss with Max? https://support.google.com/accessibility/android/answer/7101858?hl=en
On 2017/04/06 15:40:09, Theresa wrote: > On 2017/04/06 15:37:18, dullweber wrote: > > On 2017/04/06 15:08:05, Theresa wrote: > > > > > > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > > > File chrome/android/java/res/layout/clear_browsing_data_tab_content.xml > > (right): > > > > > > > > > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > > > chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:29: > > > android:minHeight="36dp" > > > Did you run into issues where you accidentally tapped the clear data button > > when > > > trying to access the Android controls? It's a bit unconventional to add > > > top/bottom margin to reduce the touch target. Typically a button's touch > > target > > > gets the full height of the View its displayed in. > > > > I didn't have any problem with it but Max suggested that it could be a > problem. > > The specs for this are here: > > > https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20V2/Specs#%2FSpe.... > > I can change it to full height and talk to him about this. > > The minimum Android touch target is 48dp for accessibility, so the spec'ed 36dp > height is in direct contrast to that. Will you please discuss with Max? > > https://support.google.com/accessibility/android/answer/7101858?hl=en Yes, I will to that. Thanks!
On 2017/04/06 15:42:00, dullweber wrote: > On 2017/04/06 15:40:09, Theresa wrote: > > On 2017/04/06 15:37:18, dullweber wrote: > > > On 2017/04/06 15:08:05, Theresa wrote: > > > > > > > > > > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > > > > File chrome/android/java/res/layout/clear_browsing_data_tab_content.xml > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2805623003/diff/1/chrome/android/java/res/lay... > > > > chrome/android/java/res/layout/clear_browsing_data_tab_content.xml:29: > > > > android:minHeight="36dp" > > > > Did you run into issues where you accidentally tapped the clear data > button > > > when > > > > trying to access the Android controls? It's a bit unconventional to add > > > > top/bottom margin to reduce the touch target. Typically a button's touch > > > target > > > > gets the full height of the View its displayed in. > > > > > > I didn't have any problem with it but Max suggested that it could be a > > problem. > > > The specs for this are here: > > > > > > https://folio.googleplex.com/clear-browsing-data/Dialog%20-%20V2/Specs#%2FSpe.... > > > I can change it to full height and talk to him about this. > > > > The minimum Android touch target is 48dp for accessibility, so the spec'ed > 36dp > > height is in direct contrast to that. Will you please discuss with Max? > > > > https://support.google.com/accessibility/android/answer/7101858?hl=en > > Yes, I will to that. Thanks! I send him a message and changed the button height.
lgtm
The CQ bit was checked by dullweber@chromium.org
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": 20001, "attempt_start_ts": 1491549579926010,
"parent_rev": "679c2b37f46e5542996e26304218bf922a4eb43e", "commit_rev":
"ea3bd60b2f5f5b394bde861c15d3c865e47caf87"}
Message was sent while issue was closed.
Description was changed from ========== Move the clear button to the right and reduce its touch target to avoid accidential clicks when the user actually wanted to press the back or home button. https://screenshot.googleplex.com/evyLBUaowaG.png BUG=681523 ========== to ========== Move the clear button to the right and reduce its touch target to avoid accidential clicks when the user actually wanted to press the back or home button. https://screenshot.googleplex.com/evyLBUaowaG.png BUG=681523 Review-Url: https://codereview.chromium.org/2805623003 Cr-Commit-Position: refs/heads/master@{#462803} Committed: https://chromium.googlesource.com/chromium/src/+/ea3bd60b2f5f5b394bde861c15d3... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ea3bd60b2f5f5b394bde861c15d3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
