Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747423005:
cros: Allow hotrod remote to check/uncheck a11y checkboxes for CFM oobe (Closed)
|
Created:
3 years, 9 months ago by Qiang(Joe) Xu Modified:
3 years, 9 months ago Reviewers:
xiyuan CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Allow hotrod remote to check/uncheck a11y checkboxes for CFM oobe
Changes:
allow hotrod remote controller to check/uncheck a11y checkboxes for oobe a11y checkboxes.
BUG=700128
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2747423005
Cr-Commit-Position: refs/heads/master@{#457862}
Committed: https://chromium.googlesource.com/chromium/src/+/29994c2a7c27139cb3d6410f8f4c06a071b0225d
Patch Set 1 #Patch Set 2 : improvement #Patch Set 3 : simulate click event #Messages
Total messages: 20 (11 generated)
Description was changed from ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes BUG=700128 ========== to ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes BUG=700128 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes BUG=700128 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes Changes: allow hotrod remote controller to check/uncheck a11y checkboxes for oobe a11y checkboxes. BUG=700128 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
warx@chromium.org changed reviewers: + xiyuan@chromium.org
Hi xiyuan@, PTAL, thanks! This patch may have some code duplication. We probably can add a second parameter for each a11y handler, maybe is_click? We need to distinguish between click and enter, because click is to send checked bool and enter is to send !checked bool. Another way is, to implement Toggle operation on C++ side. What do you think?
The CQ bit was checked by warx@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.
On 2017/03/17 03:08:48, Qiang(Joe) Xu wrote: > Hi xiyuan@, PTAL, thanks! > > This patch may have some code duplication. We probably can add a second > parameter for each a11y handler, maybe is_click? We need to distinguish between > click and enter, because click is to send checked bool and enter is to send > !checked bool. > > Another way is, to implement Toggle operation on C++ side. What do you think? Does the remote has a SPACE key? If it does, could the SPACE key be used to toggle the checkboxes? If we want to add ENTER key to toggle, we can probably do it by handle 'keypress' event and on ENTER key, create and dispatch a 'click' event on the checkbox. Could you give that a try?
Description was changed from ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes Changes: allow hotrod remote controller to check/uncheck a11y checkboxes for oobe a11y checkboxes. BUG=700128 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes for CFM oobe Changes: allow hotrod remote controller to check/uncheck a11y checkboxes for oobe a11y checkboxes. BUG=700128 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/03/17 04:56:19, xiyuan wrote:
> On 2017/03/17 03:08:48, Qiang(Joe) Xu wrote:
> > Hi xiyuan@, PTAL, thanks!
> >
> > This patch may have some code duplication. We probably can add a second
> > parameter for each a11y handler, maybe is_click? We need to distinguish
> between
> > click and enter, because click is to send checked bool and enter is to send
> > !checked bool.
> >
> > Another way is, to implement Toggle operation on C++ side. What do you
think?
>
> Does the remote has a SPACE key? If it does, could the SPACE key be used to
> toggle the checkboxes?
>
> If we want to add ENTER key to toggle, we can probably do it by handle
> 'keypress' event and on ENTER key, create and dispatch a 'click' event on the
> checkbox. Could you give that a try?
Space key works just like mouse click event, but I don't know why yet.
Changing to 'keypress' event doesn't help. In the new patch I am doing
$('..').checked = !$('..').checked before chrome.send. PTAL, thanks!
On 2017/03/17 16:04:20, Qiang(Joe) Xu wrote:
> On 2017/03/17 04:56:19, xiyuan wrote:
> > On 2017/03/17 03:08:48, Qiang(Joe) Xu wrote:
> > > Hi xiyuan@, PTAL, thanks!
> > >
> > > This patch may have some code duplication. We probably can add a second
> > > parameter for each a11y handler, maybe is_click? We need to distinguish
> > between
> > > click and enter, because click is to send checked bool and enter is to
send
> > > !checked bool.
> > >
> > > Another way is, to implement Toggle operation on C++ side. What do you
> think?
> >
> > Does the remote has a SPACE key? If it does, could the SPACE key be used to
> > toggle the checkboxes?
> >
> > If we want to add ENTER key to toggle, we can probably do it by handle
> > 'keypress' event and on ENTER key, create and dispatch a 'click' event on
the
> > checkbox. Could you give that a try?
>
> Space key works just like mouse click event, but I don't know why yet.
>
> Changing to 'keypress' event doesn't help. In the new patch I am doing
> $('..').checked = !$('..').checked before chrome.send. PTAL, thanks!
Space key should be the default keyboard toggle supported by blink. It is a
feature. If we have space key, why do we need to add ENTER key support?
Think you can get rid of the '!' hack and implement handleA11yKeyPress like
this:
handleA11yKeyPress: function(e) {
if (e.key != 'Enter')
return;
// Optionally ensures this happens on a checkbox.
// if (e.target.tagName != 'INPUT' || e.target.type != 'checkbox')
// return;
// Simulate click on the checkbox.
e.target.click();
),
On 2017/03/17 16:28:10, xiyuan wrote:
> On 2017/03/17 16:04:20, Qiang(Joe) Xu wrote:
> > On 2017/03/17 04:56:19, xiyuan wrote:
> > > On 2017/03/17 03:08:48, Qiang(Joe) Xu wrote:
> > > > Hi xiyuan@, PTAL, thanks!
> > > >
> > > > This patch may have some code duplication. We probably can add a second
> > > > parameter for each a11y handler, maybe is_click? We need to distinguish
> > > between
> > > > click and enter, because click is to send checked bool and enter is to
> send
> > > > !checked bool.
> > > >
> > > > Another way is, to implement Toggle operation on C++ side. What do you
> > think?
> > >
> > > Does the remote has a SPACE key? If it does, could the SPACE key be used
to
> > > toggle the checkboxes?
> > >
> > > If we want to add ENTER key to toggle, we can probably do it by handle
> > > 'keypress' event and on ENTER key, create and dispatch a 'click' event on
> the
> > > checkbox. Could you give that a try?
> >
> > Space key works just like mouse click event, but I don't know why yet.
> >
> > Changing to 'keypress' event doesn't help. In the new patch I am doing
> > $('..').checked = !$('..').checked before chrome.send. PTAL, thanks!
>
> Space key should be the default keyboard toggle supported by blink. It is a
> feature. If we have space key, why do we need to add ENTER key support?
>
> Think you can get rid of the '!' hack and implement handleA11yKeyPress like
> this:
>
> handleA11yKeyPress: function(e) {
> if (e.key != 'Enter')
> return;
>
> // Optionally ensures this happens on a checkbox.
> // if (e.target.tagName != 'INPUT' || e.target.type != 'checkbox')
> // return;
>
> // Simulate click on the checkbox.
> e.target.click();
> ),
Thanks for the suggestion. It is much better now. PTAL.
lgtm
The CQ bit was checked by warx@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": 40001, "attempt_start_ts": 1489769271222960,
"parent_rev": "fb07ed0749ddbb4791c03e9a1b996ec1b0fe191b", "commit_rev":
"29994c2a7c27139cb3d6410f8f4c06a071b0225d"}
Message was sent while issue was closed.
Description was changed from ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes for CFM oobe Changes: allow hotrod remote controller to check/uncheck a11y checkboxes for oobe a11y checkboxes. BUG=700128 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow hotrod remote to check/uncheck a11y checkboxes for CFM oobe Changes: allow hotrod remote controller to check/uncheck a11y checkboxes for oobe a11y checkboxes. BUG=700128 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2747423005 Cr-Commit-Position: refs/heads/master@{#457862} Committed: https://chromium.googlesource.com/chromium/src/+/29994c2a7c27139cb3d6410f8f4c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/29994c2a7c27139cb3d6410f8f4c... |
