Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491213002:
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.
This change also adds the ripple effect of the toggle button when changing the state from repeat-all to repeat-one-track.
This change is a renewed version of 2482063002.
BUG=662829
TEST=browser_test OpenAudioFiles/FileManagerBrowserTest.Test, manual test by hitting [TAB] key several times
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix duplicated ripple effect. The button should be toggle type. #Patch Set 3 : Fix duplicated ripple effect. #
Total comments: 5
Messages
Total messages: 31 (12 generated)
Description was changed from ========== Make the repeat button accessible by keyboard. This change is a renewed version of 2482063002. BUG=662829 TEST=browser_test OpenAudioFiles/FileManagerBrowserTest.Test, manual test by hitting [TAB] key several times ========== to ========== Make the repeat button accessible by keyboard. This change is a renewed version of 2482063002. BUG=662829 TEST=browser_test OpenAudioFiles/FileManagerBrowserTest.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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yamaguchi@chromium.org changed reviewers: + oka@chromium.org
ptal
fukino@chromium.org changed reviewers: + fukino@chromium.org
https://codereview.chromium.org/2491213002/diff/1/ui/file_manager/audio_playe... File ui/file_manager/audio_player/elements/repeat_button.html (right): https://codereview.chromium.org/2491213002/diff/1/ui/file_manager/audio_playe... ui/file_manager/audio_player/elements/repeat_button.html:12: <!-- TODO(yamaguchi): Eliminate extra ripple effect upon mouse click. --> I'm sorry for jumping in this already-agreed approach! If you don't have a plan to fix this regression at this moment, I think it is better to stick with the previous implementation by copying necessary logic/style from files-icon-button. You can factor out the common parts to shared style and behavior later.
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...
https://codereview.chromium.org/2491213002/diff/1/ui/file_manager/audio_playe... File ui/file_manager/audio_player/elements/repeat_button.html (right): https://codereview.chromium.org/2491213002/diff/1/ui/file_manager/audio_playe... ui/file_manager/audio_player/elements/repeat_button.html:12: <!-- TODO(yamaguchi): Eliminate extra ripple effect upon mouse click. --> On 2016/11/10 07:09:35, fukino wrote: > I'm sorry for jumping in this already-agreed approach! > > If you don't have a plan to fix this regression at this moment, I think it is > better to stick with the previous implementation by copying necessary > logic/style from files-icon-button. > You can factor out the common parts to shared style and behavior later. Resolved the TODO now. I'm sorry, this was very simple to fix.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looking good. Just one question. https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... File ui/file_manager/audio_player/elements/repeat_button.html (right): https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... ui/file_manager/audio_player/elements/repeat_button.html:13: repeat-mode$="{{repeatMode}}" toggles> What is the intention to change "class$=" to "repeat-mode$="? I guess class$= still works and we won't need to modify tests with it?
On 2016/11/10 08:30:19, fukino wrote: > looking good. Just one question. > > https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... > File ui/file_manager/audio_player/elements/repeat_button.html (right): > > https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... > ui/file_manager/audio_player/elements/repeat_button.html:13: > repeat-mode$="{{repeatMode}}" toggles> > What is the intention to change "class$=" to "repeat-mode$="? > I guess class$= still works and we won't need to modify tests with it? lgtm. Please get LGTM from fukino-san.
https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... File ui/file_manager/audio_player/elements/repeat_button.html (right): https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... ui/file_manager/audio_player/elements/repeat_button.html:13: repeat-mode$="{{repeatMode}}" toggles> On 2016/11/10 08:30:19, fukino wrote: > What is the intention to change "class$=" to "repeat-mode$="? > I guess class$= still works and we won't need to modify tests with it? To avoid that keyboard focus indicator disappears when hitting Enter key on the button. https://codereview.chromium.org/2482063002/#ps40001 When we put the toggle status as a part of class attribute, I found the script will remove the "keyboard-focus" value in class attribute here: https://cs.chromium.org/chromium/src/third_party/catapult/third_party/polymer... which should have been added by files-icon-button: https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... I am not yet 100% sure if there's a better solution, though.
Description was changed from ========== Make the repeat button accessible by keyboard. This change is a renewed version of 2482063002. BUG=662829 TEST=browser_test OpenAudioFiles/FileManagerBrowserTest.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. This change also adds the ripple effect of the toggle button when changing the state from repeat-all to repeat-one-track. This change is a renewed version of 2482063002. BUG=662829 TEST=browser_test OpenAudioFiles/FileManagerBrowserTest.Test, manual test by hitting [TAB] key several times CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Updated the CL description. This change also adds the ripple effect on transition from repeat-all to repeat-one-track. Before this change, the icon was silently changed upon this transition without the ripple effect by click/tap. Would this require more discussion with UX?
On 2016/11/10 08:48:07, yamaguchi wrote: > Updated the CL description. > This change also adds the ripple effect on transition from repeat-all to > repeat-one-track. Before this change, the icon was silently changed upon this > transition without the ripple effect by click/tap. > Would this require more discussion with UX? How about taking screencast and ask UX for LGTM?
On 2016/11/10 08:55:53, oka wrote: > On 2016/11/10 08:48:07, yamaguchi wrote: > > Updated the CL description. > > This change also adds the ripple effect on transition from repeat-all to > > repeat-one-track. Before this change, the icon was silently changed upon this > > transition without the ripple effect by click/tap. > > Would this require more discussion with UX? > > How about taking screencast and ask UX for LGTM? Filed a bug for it. https://bugs.chromium.org/p/chromium/issues/detail?id=664064
https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... File ui/file_manager/audio_player/elements/repeat_button.css (right): https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... ui/file_manager/audio_player/elements/repeat_button.css:5: [repeat-mode=repeat-all], [repeat-mode=no-repeat] { files-icon-button { } will work. https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... ui/file_manager/audio_player/elements/repeat_button.css:11: [repeat-mode=repeat-one] { :host[repeat-mode='repeat-one'] files-icon-button { } will work. https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... File ui/file_manager/audio_player/elements/repeat_button.html (right): https://codereview.chromium.org/2491213002/diff/40001/ui/file_manager/audio_p... ui/file_manager/audio_player/elements/repeat_button.html:13: repeat-mode$="{{repeatMode}}" toggles> On 2016/11/10 08:43:16, yamaguchi wrote: > On 2016/11/10 08:30:19, fukino wrote: > > What is the intention to change "class$=" to "repeat-mode$="? > > I guess class$= still works and we won't need to modify tests with it? > > To avoid that keyboard focus indicator disappears when hitting Enter key on the > button. > https://codereview.chromium.org/2482063002/#ps40001 > > When we put the toggle status as a part of class attribute, I found the script > will remove the "keyboard-focus" value in class attribute here: > https://cs.chromium.org/chromium/src/third_party/catapult/third_party/polymer... > which should have been added by files-icon-button: > https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... > > I am not yet 100% sure if there's a better solution, though. I see. Since the repeatMode's refrectToAttribute is true, we can set background image without this binding. I left comment in the css file.
On 2016/11/10 08:48:07, yamaguchi wrote: > Updated the CL description. > This change also adds the ripple effect on transition from repeat-all to > repeat-one-track. Before this change, the icon was silently changed upon this > transition without the ripple effect by click/tap. > Would this require more discussion with UX? I saw a flicker on clicking this repeat button's active->active transition. This looks a bug to me. One theory is that the click event on the repeat-button propagates to the inner files-icon-button and the 'activated' attribute of files-toggle-ripple was changed to false unexpectedly?
If fixing this is too complicated, I'm OK with copy & paste solution. Sorry for back and forth. On Thu, Nov 10, 2016 at 6:30 PM <fukino@chromium.org> wrote: > On 2016/11/10 08:48:07, yamaguchi wrote: > > Updated the CL description. > > This change also adds the ripple effect on transition from repeat-all to > > repeat-one-track. Before this change, the icon was silently changed upon > this > > transition without the ripple effect by click/tap. > > Would this require more discussion with UX? > > I saw a flicker on clicking this repeat button's active->active transition. > This looks a bug to me. > One theory is that the click event on the repeat-button propagates to the > inner > files-icon-button and the 'activated' attribute of files-toggle-ripple was > changed to false unexpectedly? > > https://codereview.chromium.org/2491213002/ > -- 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.
On 2016/11/10 09:30:44, fukino wrote: > On 2016/11/10 08:48:07, yamaguchi wrote: > > Updated the CL description. > > This change also adds the ripple effect on transition from repeat-all to > > repeat-one-track. Before this change, the icon was silently changed upon this > > transition without the ripple effect by click/tap. > > Would this require more discussion with UX? > > I saw a flicker on clicking this repeat button's active->active transition. > This looks a bug to me. > One theory is that the click event on the repeat-button propagates to the inner > files-icon-button and the 'activated' attribute of files-toggle-ripple was > changed to false unexpectedly? I think it's because we only have OFF-to-ON and ON-to-OFF animations in files-toggle-ripple, and just reusing the OFF-to-ON animation for the transition from 2 to 3. It should actually be "ON-to-ON" animation, which we may not have in the spec now. https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/...
Having the spec sounds good to me. Thank you for creating the bug. On Thu, Nov 10, 2016 at 6:44 PM <yamaguchi@chromium.org> wrote: > On 2016/11/10 09:30:44, fukino wrote: > > On 2016/11/10 08:48:07, yamaguchi wrote: > > > Updated the CL description. > > > This change also adds the ripple effect on transition from repeat-all > to > > > repeat-one-track. Before this change, the icon was silently changed > upon > this > > > transition without the ripple effect by click/tap. > > > Would this require more discussion with UX? > > > > I saw a flicker on clicking this repeat button's active->active > transition. > > This looks a bug to me. > > One theory is that the click event on the repeat-button propagates to the > inner > > files-icon-button and the 'activated' attribute of files-toggle-ripple > was > > changed to false unexpectedly? > > I think it's because we only have OFF-to-ON and ON-to-OFF animations in > files-toggle-ripple, and just reusing the OFF-to-ON animation for the > transition > from 2 to 3. It should actually be "ON-to-ON" animation, which we may not > have > in the spec now. > > https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... > > https://codereview.chromium.org/2491213002/ > -- 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.
On 2016/11/10 09:44:16, yamaguchi wrote: > On 2016/11/10 09:30:44, fukino wrote: > > On 2016/11/10 08:48:07, yamaguchi wrote: > > > Updated the CL description. > > > This change also adds the ripple effect on transition from repeat-all to > > > repeat-one-track. Before this change, the icon was silently changed upon > this > > > transition without the ripple effect by click/tap. > > > Would this require more discussion with UX? > > > > I saw a flicker on clicking this repeat button's active->active transition. > > This looks a bug to me. > > One theory is that the click event on the repeat-button propagates to the > inner > > files-icon-button and the 'activated' attribute of files-toggle-ripple was > > changed to false unexpectedly? > > I think it's because we only have OFF-to-ON and ON-to-OFF animations in > files-toggle-ripple, and just reusing the OFF-to-ON animation for the transition > from 2 to 3. It should actually be "ON-to-ON" animation, which we may not have > in the spec now. > https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... I think it can be problematic that inner files-icon-button handles click event and the button's toggle state goes to false temporarily during ON-to-ON transition. Regarding the animation, current implementation was verified by the designer, so we should keep it as long as we don't get instruction from designers. https://bugs.chromium.org/p/chromium/issues/detail?id=524884#c10
On 2016/11/10 09:57:19, fukino wrote: > On 2016/11/10 09:44:16, yamaguchi wrote: > > On 2016/11/10 09:30:44, fukino wrote: > > > On 2016/11/10 08:48:07, yamaguchi wrote: > > > > Updated the CL description. > > > > This change also adds the ripple effect on transition from repeat-all to > > > > repeat-one-track. Before this change, the icon was silently changed upon > > this > > > > transition without the ripple effect by click/tap. > > > > Would this require more discussion with UX? > > > > > > I saw a flicker on clicking this repeat button's active->active transition. > > > This looks a bug to me. > > > One theory is that the click event on the repeat-button propagates to the > > inner > > > files-icon-button and the 'activated' attribute of files-toggle-ripple was > > > changed to false unexpectedly? > > > > I think it's because we only have OFF-to-ON and ON-to-OFF animations in > > files-toggle-ripple, and just reusing the OFF-to-ON animation for the > transition > > from 2 to 3. It should actually be "ON-to-ON" animation, which we may not have > > in the spec now. > > > https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... > > I think it can be problematic that inner files-icon-button handles click event > and the button's toggle state goes to false temporarily during ON-to-ON > transition. > Regarding the animation, current implementation was verified by the designer, so > we should keep it as long as we don't get instruction from designers. > https://bugs.chromium.org/p/chromium/issues/detail?id=524884#c10 Thanks. Now I also found that your hypothesis is right. Current code doesn't trigger animation unless the element attribute is changed. I will revise this CL to preserve existing appearance.
On 2016/11/10 10:01:54, yamaguchi wrote: > On 2016/11/10 09:57:19, fukino wrote: > > On 2016/11/10 09:44:16, yamaguchi wrote: > > > On 2016/11/10 09:30:44, fukino wrote: > > > > On 2016/11/10 08:48:07, yamaguchi wrote: > > > > > Updated the CL description. > > > > > This change also adds the ripple effect on transition from repeat-all to > > > > > repeat-one-track. Before this change, the icon was silently changed upon > > > this > > > > > transition without the ripple effect by click/tap. > > > > > Would this require more discussion with UX? > > > > > > > > I saw a flicker on clicking this repeat button's active->active > transition. > > > > This looks a bug to me. > > > > One theory is that the click event on the repeat-button propagates to the > > > inner > > > > files-icon-button and the 'activated' attribute of files-toggle-ripple was > > > > changed to false unexpectedly? > > > > > > I think it's because we only have OFF-to-ON and ON-to-OFF animations in > > > files-toggle-ripple, and just reusing the OFF-to-ON animation for the > > transition > > > from 2 to 3. It should actually be "ON-to-ON" animation, which we may not > have > > > in the spec now. > > > > > > https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... > > > > I think it can be problematic that inner files-icon-button handles click event > > and the button's toggle state goes to false temporarily during ON-to-ON > > transition. > > Regarding the animation, current implementation was verified by the designer, > so > > we should keep it as long as we don't get instruction from designers. > > https://bugs.chromium.org/p/chromium/issues/detail?id=524884#c10 > > Thanks. Now I also found that your hypothesis is right. Current code doesn't > trigger animation unless the element attribute is changed. > I will revise this CL to preserve existing appearance. Let me make the 'problematic' a bit clearer. repeat-button's click event handling does not comply with the Polymer.IronButtonState's behavior. Polymer.IronButtonState's assumption is that click on the element should always toggle the active state. So, if we reuse the repeat-button's style, we should avoid sending click events to files-icon-button.
On 2016/11/10 10:01:54, yamaguchi wrote: > On 2016/11/10 09:57:19, fukino wrote: > > On 2016/11/10 09:44:16, yamaguchi wrote: > > > On 2016/11/10 09:30:44, fukino wrote: > > > > On 2016/11/10 08:48:07, yamaguchi wrote: > > > > > Updated the CL description. > > > > > This change also adds the ripple effect on transition from repeat-all to > > > > > repeat-one-track. Before this change, the icon was silently changed upon > > > this > > > > > transition without the ripple effect by click/tap. > > > > > Would this require more discussion with UX? > > > > > > > > I saw a flicker on clicking this repeat button's active->active > transition. > > > > This looks a bug to me. > > > > One theory is that the click event on the repeat-button propagates to the > > > inner > > > > files-icon-button and the 'activated' attribute of files-toggle-ripple was > > > > changed to false unexpectedly? > > > > > > I think it's because we only have OFF-to-ON and ON-to-OFF animations in > > > files-toggle-ripple, and just reusing the OFF-to-ON animation for the > > transition > > > from 2 to 3. It should actually be "ON-to-ON" animation, which we may not > have > > > in the spec now. > > > > > > https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/... > > > > I think it can be problematic that inner files-icon-button handles click event > > and the button's toggle state goes to false temporarily during ON-to-ON > > transition. > > Regarding the animation, current implementation was verified by the designer, > so > > we should keep it as long as we don't get instruction from designers. > > https://bugs.chromium.org/p/chromium/issues/detail?id=524884#c10 > > Thanks. Now I also found that your hypothesis is right. Current code doesn't > trigger animation unless the element attribute is changed. > I will revise this CL to preserve existing appearance. Thanks! If the designer request us ON-to-ON transition, let's consider it separately :)
I will abandon this changelist because we resolved the original issue by antoher changelist: https://codereview.chromium.org/2495213002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
