Files app.: Long-tap on header row in file list loses focus from list |
||||||||||||||
Issue descriptionChrome Version: 58.0.3029.140 Chrome OS Version: 9334.72.0 Steps To Reproduce: (1) Open a folder having 2 or more entries. (2) Select the first file in the file list view. See the selected file's background turns light blue. (3) Long tap on the "Name" header cell in the table header row. (4) Hit [down arrow] key. Expected Result: 3: The selected file remains blue. 4: The second file gets selected. Actual Result: 3: The selected file turns gray. 4: Nothing happens. This is caused by having the keyboard focus moved out of the file list. There's a logic to focus on the list when the header is clicked, but only for mouse. https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/ui/file_table.js?type=cs&q=%22cr.ui.Table%22+mousedown+%22Keep+focus+on+the+file%22&sq=package:chromium&l=432 How frequently does this problem reproduce? (Always, sometimes, hard to reproduce?) Always. What is the impact to the user, and is there a workaround? If so, what is it? There are several workarounds. - Hit [tab] 2 times to move the keyboard focus back to the list. - Select files using mouse or touchscreen. I think this is not a major issue, but just to make sure that this is not related to recent update around the touch UI.
,
Aug 28 2017
,
Aug 28 2017
I'm not sure why, but long-tap on the header moves the focus to body. Maybe we can restore the focus on touchend event, though then the color becomes gray while the header is being tapped, and then backs to blue on tap end. yamaguchi@, WDYT?
,
Aug 28 2017
I think that that should be acceptable, but also that the best is not to turn the color tentatively. It looks as if the focus is moved upon the dragstart event, despite the header element doesn't seem to be receiving that type of event as far as I investigated. I think it'd be better if you (mariannet@) could try to figure out how it is happening.
,
Aug 28 2017
Will do!
,
Aug 29 2017
Trying to figure out what event causes the change
,
Aug 29 2017
,
Aug 29 2017
,
Aug 29 2017
,
Aug 29 2017
,
Aug 30 2017
Similar to https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/ui/file_table.js?type=cs&q=%22cr.ui.Table%22+mousedown+%22Keep+focus+on+the+file%22&sq=package:chromium&l=432, can't we just listening pointerdown or touchstart to prevent focusing on the header element?
,
Aug 30 2017
We tried that, but long-taps get transformed into contextmenu event, triggering the change of focus. When listening to touchstart and touchend, the behavior I saw was: 1. select a file 2. long-tap header 3. touchstart triggers, focus is kept on the list (list item is blue) 4. contextmenu triggers, focus is changed to the header, thus turning the list item grey 5. touchend triggers, focus is changed back to the list, list item turns blue again By adjusting the contextmenu event handler, focus never changed to the header in the first place.
,
Aug 30 2017
Re: #13 Did you call event.preventDefault() on touchstart or pointerdown? IIUC, contextmenu event will not follow when default behavior on touchstart/ponterdown is prevented.
,
Aug 30 2017
I did call it on touchstart, and there it didn't prevent contextmenu from being triggered - pointerdown seems to prevent it from my manual tests. While looking through events, I haven't been able to find a good list with what causes what etc., do you happen to know one? Events also seems to be browser-specific, so I'm happy we only have to look at Chrome because of ChromeOS :D
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/128d8d0b7d28106be840701b9e3aa87fe7ae07e4 commit 128d8d0b7d28106be840701b9e3aa87fe7ae07e4 Author: mariannet <mariannet@google.com> Date: Wed Aug 30 11:20:45 2017 Prevent long-tapping on the header from changing focus. Before: Long-tapping the header caused losing focus on the list by triggering a 'contextmenu' After: Focus is no longer lost when long-tapping the header. Bug: 752389 Test: manual tests Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I9e9bb58955feefc91d5b6425d2b020aad0a2949a Reviewed-on: https://chromium-review.googlesource.com/641010 Reviewed-by: Naoki Fukino <fukino@chromium.org> Reviewed-by: Keigo Oka <oka@chromium.org> Commit-Queue: Marianne Thieffry <mariannet@google.com> Cr-Commit-Position: refs/heads/master@{#498418} [modify] https://crrev.com/128d8d0b7d28106be840701b9e3aa87fe7ae07e4/ui/file_manager/file_manager/foreground/js/ui/file_table.js
,
Aug 30 2017
,
Sep 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0c2197b56fc84a027edb276958201cef7d0952a8 commit 0c2197b56fc84a027edb276958201cef7d0952a8 Author: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Date: Fri Sep 08 12:49:43 2017 Revert "Prevent long-tapping on the header from changing focus." This reverts commit 128d8d0b7d28106be840701b9e3aa87fe7ae07e4. Reason for revert: Regression; it prevented "resize column by mouse drag" functionality. https://bugs.chromium.org/p/chromium/issues/detail?id=763316 Original change's description: > Prevent long-tapping on the header from changing focus. > > Before: > Long-tapping the header caused losing focus on the list by triggering a 'contextmenu' > After: > Focus is no longer lost when long-tapping the header. > > Bug: 752389 > Test: manual tests > Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation > Change-Id: I9e9bb58955feefc91d5b6425d2b020aad0a2949a > Reviewed-on: https://chromium-review.googlesource.com/641010 > Reviewed-by: Naoki Fukino <fukino@chromium.org> > Reviewed-by: Keigo Oka <oka@chromium.org> > Commit-Queue: Marianne Thieffry <mariannet@google.com> > Cr-Commit-Position: refs/heads/master@{#498418} TBR=fukino@chromium.org,oka@chromium.org,mariannet@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 752389 Change-Id: I79da44e2772b64fef59bdb1b6d1905fb4fb26999 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Reviewed-on: https://chromium-review.googlesource.com/657458 Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Commit-Queue: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Cr-Commit-Position: refs/heads/master@{#500573} [modify] https://crrev.com/0c2197b56fc84a027edb276958201cef7d0952a8/ui/file_manager/file_manager/foreground/js/ui/file_table.js
,
Sep 8 2017
Re-opening, as the patch was reverted.
,
Sep 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae3d372b13143030f8e0e5284d812ae9a6df6709 commit ae3d372b13143030f8e0e5284d812ae9a6df6709 Author: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Date: Mon Sep 11 10:58:06 2017 Revert "Prevent long-tapping on the header from changing focus." This reverts commit 128d8d0b7d28106be840701b9e3aa87fe7ae07e4. Reason for revert: Regression; it prevented "resize column by mouse drag" functionality. https://bugs.chromium.org/p/chromium/issues/detail?id=763316 Original change's description: > Prevent long-tapping on the header from changing focus. > > Before: > Long-tapping the header caused losing focus on the list by triggering a 'contextmenu' > After: > Focus is no longer lost when long-tapping the header. > > Bug: 752389 > Test: manual tests > Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation > Change-Id: I9e9bb58955feefc91d5b6425d2b020aad0a2949a > Reviewed-on: https://chromium-review.googlesource.com/641010 > Reviewed-by: Naoki Fukino <fukino@chromium.org> > Reviewed-by: Keigo Oka <oka@chromium.org> > Commit-Queue: Marianne Thieffry <mariannet@google.com> > Cr-Commit-Position: refs/heads/master@{#498418} TBR=fukino@chromium.org,oka@chromium.org,mariannet@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 752389 Change-Id: I79da44e2772b64fef59bdb1b6d1905fb4fb26999 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Reviewed-on: https://chromium-review.googlesource.com/657458 Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Commit-Queue: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#500573}(cherry picked from commit 0c2197b56fc84a027edb276958201cef7d0952a8) Reviewed-on: https://chromium-review.googlesource.com/659639 Reviewed-by: Naoki Fukino <fukino@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#117} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/ae3d372b13143030f8e0e5284d812ae9a6df6709/ui/file_manager/file_manager/foreground/js/ui/file_table.js
,
Sep 11 2017
fukino@, do you have idea to properly fix the issue?
,
Sep 12 2017
An idea that came to my mind was one state the cl had at some point: https://chromium-review.googlesource.com/c/chromium/src/+/641010/7 This didn't touch the existing events.
,
Sep 25 2017
,
Sep 25 2017
Listening to contextmenu event makes sense to me. Add an event around l123 in list_container.js ?
,
Sep 25 2017
Also, it's awesome if you can add a test to prevent regression.
,
Oct 4 2017
,
Oct 4 2017
Update: bug fixed, working on regression test now.
,
Oct 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19522c7631911f27e5145ee7f1a6ef5bd6bf743f commit 19522c7631911f27e5145ee7f1a6ef5bd6bf743f Author: mariannet <mariannet@google.com> Date: Tue Oct 17 06:33:09 2017 Long tapping the header will keep the focus on the file table. Keep the focus on the files table when long pressing the header. Bug: 752389 Test: manually Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: Ic9833320e728cb64767741b88c43d6c8a22fc733 Reviewed-on: https://chromium-review.googlesource.com/716077 Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Reviewed-by: Keigo Oka <oka@chromium.org> Commit-Queue: Marianne Thieffry <mariannet@google.com> Cr-Commit-Position: refs/heads/master@{#509304} [modify] https://crrev.com/19522c7631911f27e5145ee7f1a6ef5bd6bf743f/ui/file_manager/file_manager/foreground/js/ui/list_container.js
,
Oct 17 2017
Fix merged. |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by weifangsun@chromium.org
, Aug 17 2017