Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482063002:
Make the repeat button accessible by keyboard. (Closed)
|
Created:
4 years, 1 month ago by yamaguchi Modified:
4 years, 1 month ago CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the repeat button accessible by keyboard.
As a known issue, duplicated ripple effect will be shown when clicking the icon (instead of keyboard).
BUG=662829
TEST=manual test by hitting [TAB] key several times
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b5275a7dcc189413073ea961c1eb7d3768ef35b8
Cr-Commit-Position: refs/heads/master@{#430897}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Allow injecting the style for keyboard focus of files-icon-button and share the setting with repeat… #Patch Set 3 : Use separate attribute for visualizing toggle state, for avoiding that [Enter] key erases the focus cursor by "keyboard-focus" class. #Patch Set 4 : Use files-icon-button inside repeat-button. #Patch Set 5 : Remove unused styles which was originally copied from files_icon_button.html. #
Total comments: 2
Patch Set 6 : Replace tabs with spaces. #Patch Set 7 : Add TODO comment. #Patch Set 8 : Update integration test to follow the change. #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== Make the repeat button accessible by keyboard. BUG=662829 TEST=manual test by hitting [TAB] key several times ========== to ========== Make the repeat button accessible by keyboard. BUG=662829 TEST=manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@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...
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
oka@chromium.org changed reviewers: + oka@chromium.org
https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... File ui/file_manager/audio_player/elements/repeat_button.css (right): https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... ui/file_manager/audio_player/elements/repeat_button.css:40: /* TODO(yamaguchi): Share the style config with other buttons in the app. */ Can't you use files-icon-button to construct repeat-button?
https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... File ui/file_manager/audio_player/elements/repeat_button.css (right): https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... ui/file_manager/audio_player/elements/repeat_button.css:40: /* TODO(yamaguchi): Share the style config with other buttons in the app. */ On 2016/11/08 06:18:50, oka wrote: > Can't you use files-icon-button to construct repeat-button? I haven't come up with simple solution by that approach. I think that if repeat-button were to contain files-icon-button, it'd need to tweak some functions in the files-icon-button object to propagate events upwards. (assuming that the actual focus should go to the inner icon-button.) What we are doing here is to make a similar button to files-icon-button but differs in both its appearance and logic. So far I got that use of behavior: in Polymer is the way to share logic among custom elements (like composition in OOP). https://www.polymer-project.org/1.0/docs/about_10 So I think it'd be better if we can quote out some common code between the two buttons and reuse it. Fukino-san would have some more thoughts about the design around these.
On 2016/11/08 12:15:39, yamaguchi wrote: > https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... > File ui/file_manager/audio_player/elements/repeat_button.css (right): > > https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... > ui/file_manager/audio_player/elements/repeat_button.css:40: /* TODO(yamaguchi): > Share the style config with other buttons in the app. */ > On 2016/11/08 06:18:50, oka wrote: > > Can't you use files-icon-button to construct repeat-button? > > I haven't come up with simple solution by that approach. I think that if > repeat-button were to contain files-icon-button, it'd need to tweak some > functions in the files-icon-button object to propagate events upwards. (assuming > that the actual focus should go to the inner icon-button.) > > What we are doing here is to make a similar button to files-icon-button but > differs in both its appearance and logic. > So far I got that use of behavior: in Polymer is the way to share logic among > custom elements (like composition in OOP). > https://www.polymer-project.org/1.0/docs/about_10 > So I think it'd be better if we can quote out some common code between the two > buttons and reuse it. > Fukino-san would have some more thoughts about the design around these. I made a sample CL to use composition in repeat_button. https://codereview.chromium.org/2486753003 The transition between repeat to repeat-one looks a bit wierd though. Screencast: https://drive.google.com/open?id=0B7EYjkGAjRAhbnNFaUlzd0E5WGc
Description was changed from ========== Make the repeat button accessible by keyboard. BUG=662829 TEST=manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make the repeat button accessible by keyboard. As a known issue, duplicated ripple effect will be shown when clicking the icon (instead of keyboard). BUG=662829 TEST=manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/11/09 05:15:25, oka wrote: > On 2016/11/08 12:15:39, yamaguchi wrote: > > > https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... > > File ui/file_manager/audio_player/elements/repeat_button.css (right): > > > > > https://codereview.chromium.org/2482063002/diff/1/ui/file_manager/audio_playe... > > ui/file_manager/audio_player/elements/repeat_button.css:40: /* > TODO(yamaguchi): > > Share the style config with other buttons in the app. */ > > On 2016/11/08 06:18:50, oka wrote: > > > Can't you use files-icon-button to construct repeat-button? > > > > I haven't come up with simple solution by that approach. I think that if > > repeat-button were to contain files-icon-button, it'd need to tweak some > > functions in the files-icon-button object to propagate events upwards. > (assuming > > that the actual focus should go to the inner icon-button.) > > > > What we are doing here is to make a similar button to files-icon-button but > > differs in both its appearance and logic. > > So far I got that use of behavior: in Polymer is the way to share logic among > > custom elements (like composition in OOP). > > https://www.polymer-project.org/1.0/docs/about_10 > > So I think it'd be better if we can quote out some common code between the two > > buttons and reuse it. > > Fukino-san would have some more thoughts about the design around these. > > I made a sample CL to use composition in repeat_button. > https://codereview.chromium.org/2486753003 > The transition between repeat to repeat-one looks a bit wierd though. > Screencast: https://drive.google.com/open?id=0B7EYjkGAjRAhbnNFaUlzd0E5WGc Thanks, I revised taking that approach. I found it will remove the "keyboard-focus" value in class attribute here when putting the toggle status as a part of class attribute. https://cs.chromium.org/chromium/src/third_party/catapult/third_party/polymer... Instead I made it use a separate attribute for toggle status.
lgtm Thank you! https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... File ui/file_manager/audio_player/elements/repeat_button.html (right): https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... ui/file_manager/audio_player/elements/repeat_button.html:13: repeat-mode$="{{repeatMode}}"></files-icon-button> Replace tab with spaces.
On 2016/11/09 08:45:23, oka wrote: > lgtm > > Thank you! > > https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... > File ui/file_manager/audio_player/elements/repeat_button.html (right): > > https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... > ui/file_manager/audio_player/elements/repeat_button.html:13: > repeat-mode$="{{repeatMode}}"></files-icon-button> > Replace tab with spaces. I think you told some weirdness on UI effect. Could you comment about it as TODO?
The CQ bit was checked by yamaguchi@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...
TODO added to repeat-button.html. https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... File ui/file_manager/audio_player/elements/repeat_button.html (right): https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... ui/file_manager/audio_player/elements/repeat_button.html:13: repeat-mode$="{{repeatMode}}"></files-icon-button> On 2016/11/09 08:45:23, oka wrote: > Replace tab with spaces. Done.
LGTM. Thank you! On Wed, Nov 9, 2016 at 5:57 PM <yamaguchi@chromium.org> wrote: > TODO added to repeat-button.html. > > > > > https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... > File ui/file_manager/audio_player/elements/repeat_button.html (right): > > > https://codereview.chromium.org/2482063002/diff/80001/ui/file_manager/audio_p... > ui/file_manager/audio_player/elements/repeat_button.html:13: > repeat-mode$="{{repeatMode}}"></files-icon-button> > On 2016/11/09 08:45:23, oka wrote: > > Replace tab with spaces. > > Done. > > https://codereview.chromium.org/2482063002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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 yamaguchi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oka@chromium.org Link to the patchset: https://codereview.chromium.org/2482063002/#ps120001 (title: "Add TODO comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make the repeat button accessible by keyboard. As a known issue, duplicated ripple effect will be shown when clicking the icon (instead of keyboard). BUG=662829 TEST=manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make the repeat button accessible by keyboard. As a known issue, duplicated ripple effect will be shown when clicking the icon (instead of keyboard). BUG=662829 TEST=manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make the repeat button accessible by keyboard. As a known issue, duplicated ripple effect will be shown when clicking the icon (instead of keyboard). BUG=662829 TEST=manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make the repeat button accessible by keyboard. As a known issue, duplicated ripple effect will be shown when clicking the icon (instead of keyboard). BUG=662829 TEST=manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5275a7dcc189413073ea961c1eb7d3768ef35b8 Cr-Commit-Position: refs/heads/master@{#430897} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b5275a7dcc189413073ea961c1eb7d3768ef35b8 Cr-Commit-Position: refs/heads/master@{#430897}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2483243004/ by vabr@chromium.org. The reason for reverting is: Broke https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%..., more info in http://crbug.com/662829#c3. Cheers, today's sheriff. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
