Add an accessibility description for notification

Add an accessibility description for notification

Previously, Chromevox speaks "button" on notification. With this CL, it changes to "notification".

BUG=728489
TEST=manual (chromevox speaks "notification" at notifications)

Review-Url: https://codereview.chromium.org/2918483002
Cr-Commit-Position: refs/heads/master@{#481120}
Committed: https://chromium.googlesource.com/chromium/src/+/99d72b12adfe2db83d7f7dbd409794b73c265a10

[yoshiki, 2017-05-31 06:00:01]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 06:00:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 06:31:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 06:31:34]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[yoshiki, 2017-06-01 03:53:26]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 03:53:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 03:56:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 03:56:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yoshiki, 2017-06-01 05:03:23]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 05:03:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-06-01 05:14:32]

Patchset #2 (id:20001) has been deleted

[yoshiki, 2017-06-01 05:17:51]

Description was changed from

==========
wip

.

BUG=
==========

to

==========
Add "notification" accessibility role

BUG=728489
==========

[yoshiki, 2017-06-01 05:17:51]

yoshiki@chromium.org changed reviewers:
+ yawano@chromium.org

[yoshiki, 2017-06-01 05:18:11]

yoshiki@chromium.org changed reviewers:
+ yhanada@chromium.org

[yoshiki, 2017-06-01 05:18:12]

yoshiki@chromium.org changed required reviewers:
+ yawano@chromium.org

[yoshiki, 2017-06-01 05:18:35]

Description was changed from

==========
Add "notification" accessibility role

BUG=728489
==========

to

==========
Add "notification" accessibility role

BUG=728489
TEST=manual (chromevox speaks "notification" at notifications)
==========

[yoshiki, 2017-06-01 05:18:54]

Awano-san, could you take an initial look? Thanks

[commit-bot: I haz the power, 2017-06-01 06:42:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 06:42:10]

Dry run: This issue passed the CQ dry run.

[yawano, 2017-06-02 04:56:52]

Mostly lgtm. Thank you!

What happens if you run ChromeVox in other language? Would the "notification" be
translated?

https://codereview.chromium.org/2918483002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2918483002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:148: notification,
Please update the extern file by running the script. Note that there is already
some diff between the script output and current extern file. It's up to you to
fix other diffs or not.

https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/aut...

[David Tseng, 2017-06-02 17:56:11]

dtseng@chromium.org changed reviewers:
+ dtseng@chromium.org

[David Tseng, 2017-06-02 17:56:12]

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_content_view.cc (left):

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_content_view.cc:640: node_data->role =
ui::AX_ROLE_BUTTON;
I would keep this.

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_content_view.cc (right):

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_content_view.cc:640: node_data->role =
ui::AX_ROLE_NOTIFICATION;
And add this as role description string attribute.

[David Tseng, 2017-06-02 18:50:12]

See
https://codereview.chromium.org/2917123002/
which supports the role description attribute in ChromeVox.


On Fri, Jun 2, 2017 at 10:56 AM, <dtseng@chromium.org> wrote:

>
> https://codereview.chromium.org/2918483002/diff/40001/ui/
> arc/notification/arc_notification_content_view.cc
> File ui/arc/notification/arc_notification_content_view.cc (left):
>
> https://codereview.chromium.org/2918483002/diff/40001/ui/
> arc/notification/arc_notification_content_view.cc#oldcode640
> ui/arc/notification/arc_notification_content_view.cc:640:
> node_data->role = ui::AX_ROLE_BUTTON;
> I would keep this.
>
> https://codereview.chromium.org/2918483002/diff/40001/ui/
> arc/notification/arc_notification_content_view.cc
> File ui/arc/notification/arc_notification_content_view.cc (right):
>
> https://codereview.chromium.org/2918483002/diff/40001/ui/
> arc/notification/arc_notification_content_view.cc#newcode640
> ui/arc/notification/arc_notification_content_view.cc:640:
> node_data->role = ui::AX_ROLE_NOTIFICATION;
> And add this as role description string attribute.
>
> https://codereview.chromium.org/2918483002/
>
> --
> 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.
>

-- 
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.

[yoshiki, 2017-06-05 05:06:03]

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_content_view.cc (left):

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_content_view.cc:640: node_data->role =
ui::AX_ROLE_BUTTON;
On 2017/06/02 17:56:11, David Tseng wrote:
> I would keep this.

Thank you for comment. if you don't think this change is necessary, I don't do.

[dmazzoni, 2017-06-05 05:16:52]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2017-06-05 05:16:53]

Just to make sure I understand the notification role,
this is to represent a single notification card on Android,
like when you pull down the list of notifications from the
top?

If we organized all of our roles hierarchically, what would
this role inherit from? See the ARIA categorization of
roles, for example:

https://rawgit.com/w3c/aria/master/aria/aria.html#roles_categorization

A notification seems to be something that's kind of like a list
item, it's often actionable, and usually deletable. Should it be
a live region too, i.e. does it imply that we should announce it
when it appears?

[yoshiki, 2017-06-05 05:24:50]

On 2017/06/05 05:16:53, dmazzoni wrote:
> Just to make sure I understand the notification role,
> this is to represent a single notification card on Android,
> like when you pull down the list of notifications from the
> top?

This is for a single notification. Not for the list.

> If we organized all of our roles hierarchically, what would
> this role inherit from? See the ARIA categorization of
> roles, for example:
> 
> https://rawgit.com/w3c/aria/master/aria/aria.html#roles_categorization
> 
> A notification seems to be something that's kind of like a list
> item, it's often actionable, and usually deletable. Should it be
> a live region too, i.e. does it imply that we should announce it
> when it appears?

I didn't much consider about categorization of role. The initial motivation was
to speak "notification" instead of "button" when the accessibility focus gets on
a single notification. Speaking "button" on a notification sounded strange a bit
for me.

[dmazzoni, 2017-06-05 05:30:17]

One option would be to just set the role description. If the semantics are
the same as button, you can leave the role as button, and add a string
attribute AX_ATTR_ROLE_DESCRIPTION of "notification" (localized). I think
David recently added support for ChromeVox to speak that.

In general I'm leaning towards fewer roles and more role descriptions.

However, a notification is a pretty important UI element on Android so it
might be okay to have one if we decide what it inherits from so we can
categorize it.


On Sun, Jun 4, 2017 at 10:24 PM <yoshiki@chromium.org> wrote:

> On 2017/06/05 05:16:53, dmazzoni wrote:
> > Just to make sure I understand the notification role,
> > this is to represent a single notification card on Android,
> > like when you pull down the list of notifications from the
> > top?
>
> This is for a single notification. Not for the list.
>
>
> > If we organized all of our roles hierarchically, what would
> > this role inherit from? See the ARIA categorization of
> > roles, for example:
> >
> > https://rawgit.com/w3c/aria/master/aria/aria.html#roles_categorization
> >
> > A notification seems to be something that's kind of like a list
> > item, it's often actionable, and usually deletable. Should it be
> > a live region too, i.e. does it imply that we should announce it
> > when it appears?
>
> I didn't much consider about categorization of role. The initial
> motivation was
> to speak "notification" instead of "button" when the accessibility focus
> gets on
> a single notification. Speaking "button" on a notification sounded strange
> a bit
> for me.
>
> https://codereview.chromium.org/2918483002/
>
> --
> 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.
>

-- 
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.

[David Tseng, 2017-06-05 16:27:17]

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_content_view.cc (left):

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_content_view.cc:640: node_data->role =
ui::AX_ROLE_BUTTON;
On 2017/06/05 05:06:03, yoshiki wrote:
> On 2017/06/02 17:56:11, David Tseng wrote:
> > I would keep this.
> 
> Thank you for comment. if you don't think this change is necessary, I don't
do.

No problem. It's is needed...see the other comment.

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_content_view.cc (right):

https://codereview.chromium.org/2918483002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_content_view.cc:640: node_data->role =
ui::AX_ROLE_NOTIFICATION;
On 2017/06/02 17:56:11, David Tseng wrote:
> And add this as role description string attribute.

A role is a programmatic identifier (string) that describes the essential usage
of the widget e.g. button, checkbox, etc. A role description is a localized
string describing the widget's role e.g. push button, dial, etc.

My suggestion was simply to add this string as a role description.

To add a new role, you would have to make ChromeVox aware of that role or re-use
an existing role.

Your change prompted me to hook up support for role descriptions in ChromeVox
(which was missed whenever it was added in Blink).

[yoshiki, 2017-06-12 02:28:48]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 02:28:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-06-12 02:29:13]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-06-12 02:30:27]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[yoshiki, 2017-06-12 02:38:34]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 02:38:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 02:51:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 02:51:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2017-06-12 03:12:24]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 03:12:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 05:06:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 05:06:14]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-06-14 15:57:42]

Patchset #3 (id:80001) has been deleted

[yoshiki, 2017-06-14 15:59:03]

Description was changed from

==========
Add "notification" accessibility role

BUG=728489
TEST=manual (chromevox speaks "notification" at notifications)
==========

to

==========
Add an accessibility description for notification

Previously, Chromevox speaks "button" on notification. With this CL, it changes
to "notification".

BUG=728489
TEST=manual (chromevox speaks "notification" at notifications)
==========

[yoshiki, 2017-06-14 15:59:30]

David, Dominic, I updated the CL with a11y description. PTAL. Thanks.

[yoshiki, 2017-06-20 04:27:54]

dtseng@, dmazzoni@, ping?

[David Tseng, 2017-06-20 18:38:03]

lgtm

[yoshiki, 2017-06-21 04:35:10]

Thanks!

[yoshiki, 2017-06-21 04:35:15]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2017-06-21 04:35:15]

The patchset sent to the CQ was uploaded after l-g-t-m from yawano@chromium.org
Link to the patchset: https://codereview.chromium.org/2918483002/#ps100001
(title: "Use a11y description instead of rule")

[commit-bot: I haz the power, 2017-06-21 04:35:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 06:01:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498019715594780,
"parent_rev": "9357c7e77960c6468f9f1939ef72940bb6836be4", "commit_rev":
"99d72b12adfe2db83d7f7dbd409794b73c265a10"}

[commit-bot: I haz the power, 2017-06-21 06:01:47]

Description was changed from

==========
Add an accessibility description for notification

Previously, Chromevox speaks "button" on notification. With this CL, it changes
to "notification".

BUG=728489
TEST=manual (chromevox speaks "notification" at notifications)
==========

to

==========
Add an accessibility description for notification

Previously, Chromevox speaks "button" on notification. With this CL, it changes
to "notification".

BUG=728489
TEST=manual (chromevox speaks "notification" at notifications)

Review-Url: https://codereview.chromium.org/2918483002
Cr-Commit-Position: refs/heads/master@{#481120}
Committed:
https://chromium.googlesource.com/chromium/src/+/99d72b12adfe2db83d7f7dbd4097...
==========

[commit-bot: I haz the power, 2017-06-21 06:01:50]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/99d72b12adfe2db83d7f7dbd4097...