A treegrid should expose its rows in MSAA/IA2 as outlineitems.

A treegrid should expose its rows in MSAA/IA2 as outlineitems.

This makes NVDA work slightly better -- announces levels at the beginning of items.

BUG=710046
Review-Url: https://codereview.chromium.org/2812003004
Cr-Commit-Position: refs/heads/master@{#464732}
Committed: https://chromium.googlesource.com/chromium/src/+/0e81c42f0794ac126f6414861e8632df8907d913

[aleventhal, 2017-04-11 20:57:25]

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

[aleventhal, 2017-04-11 20:58:54]

Description was changed from

==========
A treegrid should expose its rows in MSAA/IA2 as outlineitems.

For now, just change treegrid test to use ROLE_SYSTEM_OUTLINEITEM -- should fail
as the new code is still commented out.

BUG=710046
==========

to

==========
A treegrid should expose its rows in MSAA/IA2 as outlineitems.

BUG=710046
==========

[commit-bot: I haz the power, 2017-04-11 20:59:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-11 21:59:39]

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

[commit-bot: I haz the power, 2017-04-11 22:00:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-11 22:30:16]

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

[commit-bot: I haz the power, 2017-04-11 22:31:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-11 22:32:16]

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

[commit-bot: I haz the power, 2017-04-11 22:33:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-11 23:10:33]

The CQ bit was checked by aleventhal@chromium.org

[aleventhal, 2017-04-11 23:10:35]

The CQ bit was unchecked by aleventhal@chromium.org

[aleventhal, 2017-04-11 23:10:42]

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

[commit-bot: I haz the power, 2017-04-11 23:11:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-11 23:12:14]

The CQ bit was unchecked by aleventhal@chromium.org

[aleventhal, 2017-04-11 23:22:30]

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

[commit-bot: I haz the power, 2017-04-11 23:23:34]

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-04-11 23:57:35]

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

[commit-bot: I haz the power, 2017-04-11 23:57:35]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[aleventhal, 2017-04-12 03:27:02]

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

[commit-bot: I haz the power, 2017-04-12 03:27:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-12 03:59:32]

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

[commit-bot: I haz the power, 2017-04-12 03:59:42]

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-04-12 04:54:30]

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

[commit-bot: I haz the power, 2017-04-12 04:54:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleventhal, 2017-04-12 14:18:36]

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

[aleventhal, 2017-04-12 14:18:45]

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

[commit-bot: I haz the power, 2017-04-12 14:18:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aleventhal, 2017-04-12 15:36:31]

Ready for review.

https://codereview.chromium.org/2812003004/diff/30003/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/2812003004/diff/30003/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.h:923: static bool
IsInTreeGrid(const BrowserAccessibility* item);
Not sure if better as static, or does it not really matter?

[aleventhal, 2017-04-12 15:39:15]

Description was changed from

==========
A treegrid should expose its rows in MSAA/IA2 as outlineitems.

BUG=710046
==========

to

==========
A treegrid should expose its rows in MSAA/IA2 as outlineitems.

This makes NVDA work slightly better -- announces levels at the beginning of
items.

BUG=710046
==========

[commit-bot: I haz the power, 2017-04-12 16:15:52]

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

[commit-bot: I haz the power, 2017-04-12 16:15:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleventhal, 2017-04-12 16:24:22]

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

[commit-bot: I haz the power, 2017-04-12 16:25:09]

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-04-12 17:30:29]

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

[commit-bot: I haz the power, 2017-04-12 17:30:29]

Dry run: This issue passed the CQ dry run.

[aleventhal, 2017-04-12 18:13:19]

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

[dmazzoni, 2017-04-12 20:35:12]

lgtm

https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.cc:5615: (role ==
ui::AX_ROLE_TABLE &&
Can you add a comment explaining why we'd have something
with a role of TABLE even though the ARIA role was "treegrid"?
Is it a bug or expected behavior?

Also, do we really want an OUTLINE_ITEM inside a role of TABLE?

https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.cc:5616:
container->GetString16Attribute(ui::AX_ATTR_ROLE) == L"treegrid");
nit: call GetStringAttribute(ui::AX_ATTR_ROLE) == "treegrid" instead - when
you're just doing a string comparison, that will return an existing object
reference, while GetString16Attribute needs to convert.

[aleventhal, 2017-04-14 14:16:56]

On 2017/04/12 20:35:12, dmazzoni_ooo_until_april_17 wrote:
> lgtm
> 
>
https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
> 
>
https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
> content/browser/accessibility/browser_accessibility_win.cc:5615: (role ==
> ui::AX_ROLE_TABLE &&
> Can you add a comment explaining why we'd have something
> with a role of TABLE even though the ARIA role was "treegrid"?
> Is it a bug or expected behavior?
> 
> Also, do we really want an OUTLINE_ITEM inside a role of TABLE?
> 
>
https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
> content/browser/accessibility/browser_accessibility_win.cc:5616:
> container->GetString16Attribute(ui::AX_ATTR_ROLE) == L"treegrid");
> nit: call GetStringAttribute(ui::AX_ATTR_ROLE) == "treegrid" instead - when
> you're just doing a string comparison, that will return an existing object
> reference, while GetString16Attribute needs to convert.

I'm checking this in for now and we can work out these details when you return.

[aleventhal, 2017-04-14 14:16:59]

On 2017/04/12 20:35:12, dmazzoni_ooo_until_april_17 wrote:
> lgtm
> 
>
https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
> 
>
https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
> content/browser/accessibility/browser_accessibility_win.cc:5615: (role ==
> ui::AX_ROLE_TABLE &&
> Can you add a comment explaining why we'd have something
> with a role of TABLE even though the ARIA role was "treegrid"?
> Is it a bug or expected behavior?
> 
> Also, do we really want an OUTLINE_ITEM inside a role of TABLE?
> 
>
https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
> content/browser/accessibility/browser_accessibility_win.cc:5616:
> container->GetString16Attribute(ui::AX_ATTR_ROLE) == L"treegrid");
> nit: call GetStringAttribute(ui::AX_ATTR_ROLE) == "treegrid" instead - when
> you're just doing a string comparison, that will return an existing object
> reference, while GetString16Attribute needs to convert.

I'm checking this in for now and we can work out these details when you return.

[aleventhal, 2017-04-14 14:17:08]

The CQ bit was checked by aleventhal@chromium.org

[commit-bot: I haz the power, 2017-04-14 14:17:18]

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-04-14 16:03:34]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1492179428832920,
"parent_rev": "4ced9f3c7e9534b6b730d3b265e39a0fe5b17dae", "commit_rev":
"0e81c42f0794ac126f6414861e8632df8907d913"}

[commit-bot: I haz the power, 2017-04-14 16:04:21]

Description was changed from

==========
A treegrid should expose its rows in MSAA/IA2 as outlineitems.

This makes NVDA work slightly better -- announces levels at the beginning of
items.

BUG=710046
==========

to

==========
A treegrid should expose its rows in MSAA/IA2 as outlineitems.

This makes NVDA work slightly better -- announces levels at the beginning of
items.

BUG=710046

Review-Url: https://codereview.chromium.org/2812003004
Cr-Commit-Position: refs/heads/master@{#464732}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e81c42f0794ac126f6414861e86...
==========

[commit-bot: I haz the power, 2017-04-14 16:04:22]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/0e81c42f0794ac126f6414861e86...

[aleventhal, 2017-04-17 22:06:42]

Questions for your questions.

https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.cc:5615: (role ==
ui::AX_ROLE_TABLE &&
On 2017/04/12 20:35:12, dmazzoni_ooo_until_april_17 wrote:
> Can you add a comment explaining why we'd have something
> with a role of TABLE even though the ARIA role was "treegrid"?
> Is it a bug or expected behavior?
> 
> Also, do we really want an OUTLINE_ITEM inside a role of TABLE?
> 

I was following the code in InitRoleAndState() here:
https://cs.chromium.org/chromium/src/content/browser/accessibility/browser_ac...

When I just checked for AX_ROLE_TREE_GRID my new tests failed. I took the lines
above as an indication that it was the correct way to check for a treegrid. Is
it not?

https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.cc:5616:
container->GetString16Attribute(ui::AX_ATTR_ROLE) == L"treegrid");
On 2017/04/12 20:35:12, dmazzoni_ooo_until_april_17 wrote:
> nit: call GetStringAttribute(ui::AX_ATTR_ROLE) == "treegrid" instead - when
> you're just doing a string comparison, that will return an existing object
> reference, while GetString16Attribute needs to convert.

Assuming we keep this code, should I change it in the other place referenced
above as well?

[dmazzoni, 2017-04-17 22:43:35]

On 2017/04/17 22:06:42, aleventhal wrote:
> I was following the code in InitRoleAndState() here:
>
https://cs.chromium.org/chromium/src/content/browser/accessibility/browser_ac...

Yeah, let's fix both as a follow-up.

> When I just checked for AX_ROLE_TREE_GRID my new tests failed. I took the
lines
> above as an indication that it was the correct way to check for a treegrid. Is
> it not?

It was a quick hack, not completely horrible but it'd be better to fix it.

We tolerated that a lot more back before we forked WebKit, because to add
another WebKit role we'd have to convince Apple we needed it too. Now we have no
excuse not to just add a new role and it's not that much work, so we should try
to clean stuff like that up when we see it.

>
https://codereview.chromium.org/2812003004/diff/170001/content/browser/access...
> content/browser/accessibility/browser_accessibility_win.cc:5616:
> container->GetString16Attribute(ui::AX_ATTR_ROLE) == L"treegrid");
> On 2017/04/12 20:35:12, dmazzoni_ooo_until_april_17 wrote:
> > nit: call GetStringAttribute(ui::AX_ATTR_ROLE) == "treegrid" instead - when
> > you're just doing a string comparison, that will return an existing object
> > reference, while GetString16Attribute needs to convert.
> 
> Assuming we keep this code, should I change it in the other place referenced
> above as well?

Yes. Also historical in that at one point the string16 was the "natural" form of
the
string, but we switched it to std::string, so we should use std::string whenever
they're otherwise equally good.

All of the Windows APIs want string16 so most of that file uses string16.