ARIA role region is not exposed correct with MSAA+IA2 role

ARIA role region is not exposed correct with MSAA+IA2 role

According to specs, ARIA role region should be exposed as below

MSAA+IA2: ROLE_SYSTEM_PANE

AXRole:AXGroup
AXSubrole:AXDocumentRegion
AXRoleDescription:'region'

Also section tag should have AXRoleDescription as 'region'

BUG=424521
TBR=avi

Committed: https://crrev.com/ac8f5a4a56bccdaba37f393493bac0ed76253e85
Cr-Commit-Position: refs/heads/master@{#300535}

[shreeramk, 2014-10-17 13:15:41]

shreeram.k@samsung.com changed reviewers:
+ aboxhall@chromium.org, dmazzoni@chromium.org

[shreeramk, 2014-10-17 13:15:42]

Please review.

Instead of introducing a new role in blink side for section tag, I used a check
in chromium side under switch case ui::AX_ROLE_REGION for section tag.

Thanks :)

[shreeramk, 2014-10-17 13:26:29]

shreeram.k@samsung.com changed reviewers:
+ agl@chromium.org

[dmazzoni, 2014-10-17 15:46:04]

https://codereview.chromium.org/638103003/diff/40001/content/app/strings/cont...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/638103003/diff/40001/content/app/strings/cont...
content/app/strings/content_strings.grd:353: <message name="IDS_AX_ROLE_REGION"
desc="accessibility role description for section">
the description should say "region", not "section"

https://codereview.chromium.org/638103003/diff/40001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/638103003/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3694: ia_role_ =
ROLE_SYSTEM_PANE;
Did you mean to have an "else" here? This will always override setting ia_role_
inside the if block above.

[shreeramk, 2014-10-17 16:08:22]

https://codereview.chromium.org/638103003/diff/40001/content/app/strings/cont...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/638103003/diff/40001/content/app/strings/cont...
content/app/strings/content_strings.grd:353: <message name="IDS_AX_ROLE_REGION"
desc="accessibility role description for section">
On 2014/10/17 15:46:04, dmazzoni wrote:
> the description should say "region", not "section"

I am adding this for section tag. Still I need to write its as "region" ??

For ARIA role region, AXRoleDescription is working fine.

https://codereview.chromium.org/638103003/diff/40001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/638103003/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3694: ia_role_ =
ROLE_SYSTEM_PANE;
On 2014/10/17 15:46:04, dmazzoni wrote:
> Did you mean to have an "else" here? This will always override setting
ia_role_
> inside the if block above.

Ohh...yes...it would be better if its "else"

[dmazzoni, 2014-10-17 16:17:18]

On Fri, Oct 17, 2014 at 9:08 AM, <shreeram.k@samsung.com> wrote:

>
> https://codereview.chromium.org/638103003/diff/40001/
> content/app/strings/content_strings.grd
> File content/app/strings/content_strings.grd (right):
>
> https://codereview.chromium.org/638103003/diff/40001/
> content/app/strings/content_strings.grd#newcode353
> content/app/strings/content_strings.grd:353: <message
> name="IDS_AX_ROLE_REGION" desc="accessibility role description for
> section">
> On 2014/10/17 15:46:04, dmazzoni wrote:
>
>> the description should say "region", not "section"
>>
>
> I am adding this for section tag. Still I need to write its as "region"
> ??
>

Look again - I just mean the desc="" doesn't match the message name.

<message name="IDS_AX_ROLE_REGION" desc="accessibility role description for
section">

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shreeramk, 2014-10-17 16:36:51]

On 2014/10/17 16:17:18, dmazzoni wrote:
> On Fri, Oct 17, 2014 at 9:08 AM, <mailto:shreeram.k@samsung.com> wrote:
> 
> >
> > https://codereview.chromium.org/638103003/diff/40001/
> > content/app/strings/content_strings.grd
> > File content/app/strings/content_strings.grd (right):
> >
> > https://codereview.chromium.org/638103003/diff/40001/
> > content/app/strings/content_strings.grd#newcode353
> > content/app/strings/content_strings.grd:353: <message
> > name="IDS_AX_ROLE_REGION" desc="accessibility role description for
> > section">
> > On 2014/10/17 15:46:04, dmazzoni wrote:
> >
> >> the description should say "region", not "section"
> >>
> >
> > I am adding this for section tag. Still I need to write its as "region"
> > ??
> >
> 
> Look again - I just mean the desc="" doesn't match the message name.


Yes I saw that. But(My mistake) I thought the description should be for the tag
for which we are adding.

> <message name="IDS_AX_ROLE_REGION" desc="accessibility role description for
> section">

[dmazzoni, 2014-10-17 17:00:34]

On Fri, Oct 17, 2014 at 9:36 AM, <shreeram.k@samsung.com> wrote:

> Look again - I just mean the desc="" doesn't match the message name.
>>
>
> Yes I saw that. But(My mistake) I thought the description should be for
> the tag
> for which we are adding.


The description will be seen by the translator. They just get the text to
be translated, and the description, with no context. They won't know
anything about HTML or ARIA roles. Use the description to help make sure
the translator understand the proper meaning of the word. This is
particularly important if a word has multiple meanings. For example, we
could say "this refers to a region of a page or document, such as the
header, the sidebar, or the table of contents"

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[agl, 2014-10-17 17:23:34]

agl@chromium.org changed reviewers:
- agl@chromium.org

[shreeramk, 2014-10-20 13:17:13]

@Dominic: PTAL!

Thanks :)

[dmazzoni, 2014-10-20 16:33:23]

https://codereview.chromium.org/638103003/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/638103003/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3694: ia_role_ =
ROLE_SYSTEM_PANE;
I still think we need an "else" here

[shreeramk, 2014-10-20 16:41:35]

On 2014/10/20 16:33:23, dmazzoni wrote:
>
https://codereview.chromium.org/638103003/diff/60001/content/browser/accessib...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
> 
>
https://codereview.chromium.org/638103003/diff/60001/content/browser/accessib...
> content/browser/accessibility/browser_accessibility_win.cc:3694: ia_role_ =
> ROLE_SYSTEM_PANE;
> I still think we need an "else" here

Yes correct.  I'm extremely sorry about that.

[shreeramk, 2014-10-21 12:06:06]

I updated according to comments.
Please take a look now.

Thanks :)

[dmazzoni, 2014-10-21 15:24:46]

lgtm

lgtm with one formatting nit; no need for additional review, please cq after
fixing that.

https://codereview.chromium.org/638103003/diff/80001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/638103003/diff/80001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3697: else
formatting nit: when one block of an if/else uses curly braces, we require that
the other one have them too, and please put the else on the same line. It should
look like:

if (html_tag == L"section") {
  ...
} else {
  ...
}

[shreeramk, 2014-10-21 15:38:04]

The CQ bit was checked by shreeram.k@samsung.com

[shreeramk, 2014-10-21 15:38:56]

shreeram.k@samsung.com changed reviewers:
+ jam@chromium.org

[shreeramk, 2014-10-21 15:38:57]

@jam: Please review content_strings.grd changes.

Thanks

[commit-bot: I haz the power, 2014-10-21 15:39:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/638103003/100001

[commit-bot: I haz the power, 2014-10-21 15:48:29]

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

[commit-bot: I haz the power, 2014-10-21 15:48:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2014-10-21 15:54:27]

dmazzoni@chromium.org changed reviewers:
+ avi@chromium.org
- jam@chromium.org

[dmazzoni, 2014-10-21 15:54:28]

TBR:avi for OWNERS review of content/app/strings

[dmazzoni, 2014-10-21 15:54:48]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-10-21 15:55:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/638103003/100001

[Avi (use Gerrit), 2014-10-21 17:00:45]

content/app/strings lgtm

[commit-bot: I haz the power, 2014-10-21 18:20:15]

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

[commit-bot: I haz the power, 2014-10-21 18:20:16]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shreeramk, 2014-10-21 18:47:24]

The CQ bit was checked by shreeram.k@samsung.com

[commit-bot: I haz the power, 2014-10-21 18:48:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/638103003/100001

[commit-bot: I haz the power, 2014-10-21 19:30:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-21 19:30:59]

Patchset 6 (id:??) landed as
https://crrev.com/ac8f5a4a56bccdaba37f393493bac0ed76253e85
Cr-Commit-Position: refs/heads/master@{#300535}