Re-land: Send Windows accessibility events based on tree updates.

Re-land: Send Windows accessibility events based on tree updates.

The idea of this change is to fire Win AX events
based on the AX tree changing, rather than based
on Blink firing the right event and mapping it to a
Win event.

Store the Win-specific AX attributes (ia role,
ia state, name, value, etc.) in a struct, and when a
node is updated, compare the old value of each attribute
to the new value and fire an appropriate event for each
one (name changed, state changed, etc.).

Handle hypertext_ similarly, and properly diff the old and new hypertext in get_oldText and get_newText.

This enables all of the "event" tests that were
previously added to pass now, and it makes live regions
work correctly in NVDA and JAWS.

BUG=447962
Committed: https://crrev.com/eb691a960bef3c2aefa38627baa7cbdde480b81e
Cr-Commit-Position: refs/heads/master@{#313052}

Committed: https://crrev.com/9b381f3e36dfb3dc1fd93c46573222601b31d3d7
Cr-Commit-Position: refs/heads/master@{#313179}

[dmazzoni, 2015-01-16 19:58:23]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, dtseng@chromium.org

[dmazzoni, 2015-01-16 20:01:43]

Ready for review!

Sorry this is so large, but a lot of the lines of code changed are just
mechanical because I moved some attributes into a struct.

[dmazzoni, 2015-01-21 00:07:40]

Friendly ping

[aboxhall, 2015-01-22 21:22:10]

Argh, forgot to publish comments (too used to GitHub!)

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

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.cc:94: bool
BrowserAccessibility::PlatformIsChildOfLeaf() const {
Is this something we could or should reasonably cache?

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

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:121: LONG
child_id = node->ToBrowserAccessibilityWin()->unique_id_win();
Why is (was) this called child_id rather than node_id?

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:254: //
Second pass.
Why do we need to do a second pass?

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

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:2177: //if
(!old_win_attributes_)
Lingering commented-out code

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3187: // Expose
slider value.
The one feature I wish Rietveld had: move detection.

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3211: LOG(ERROR) <<
"id=" << GetId()
logspam?

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3243: if ((ia_state()
& STATE_SYSTEM_SELECTED) &&
Suggestion: pull out (ia_state() & STATE_SYSTEM_SELECTED) and
(old_win_attributes->ia_state & STATE_SYSTEM_SELECTED) as bool is_selected and
bool was_selected (or whatever) to make this easier to follow.

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3295: if (hypertext_
!= old_hypertext_) {
Maybe early-out instead?

[dmazzoni, 2015-01-23 23:26:13]

Feedback addressed, thanks!

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

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.cc:94: bool
BrowserAccessibility::PlatformIsChildOfLeaf() const {
On 2015/01/22 21:22:09, aboxhall wrote:
> Is this something we could or should reasonably cache?

Worth considering! Right now our bottlenecks are much more on the renderer side,
but we should do more profiling of the browser process while JAWS is running and
see if this shows up.

For now I think this is okay, it's not like it's adding the first function that
walks up the parent chain.

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

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:121: LONG
child_id = node->ToBrowserAccessibilityWin()->unique_id_win();
On 2015/01/22 21:22:09, aboxhall wrote:
> Why is (was) this called child_id rather than node_id?

That's the name of the ::NotifyWinEvent parameter, on the next line. For legacy
reasons, you can't call NotifyWinEvent on a MSAA object, you have to call it on
an HWND, with a child id - then the client asks the HWND for the child with that
id and we can treat that child id as a global id rather than a 1-based index.

If you want the gory details:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/Implementing_MSAA_...

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:254: //
Second pass.
On 2015/01/22 21:22:09, aboxhall wrote:
> Why do we need to do a second pass?

Because updating IAccessibleText requires the children are updated first.

I updated the comment to explain why.

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

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:2177: //if
(!old_win_attributes_)
On 2015/01/22 21:22:09, aboxhall wrote:
> Lingering commented-out code

Done.

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3187: // Expose
slider value.
On 2015/01/22 21:22:09, aboxhall wrote:
> The one feature I wish Rietveld had: move detection.

Acknowledged.

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3211: LOG(ERROR) <<
"id=" << GetId()
On 2015/01/22 21:22:09, aboxhall wrote:
> logspam?

Done.

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3243: if ((ia_state()
& STATE_SYSTEM_SELECTED) &&
On 2015/01/22 21:22:10, aboxhall wrote:
> Suggestion: pull out (ia_state() & STATE_SYSTEM_SELECTED) and
> (old_win_attributes->ia_state & STATE_SYSTEM_SELECTED) as bool is_selected and
> bool was_selected (or whatever) to make this easier to follow.

Good idea, done.

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3295: if (hypertext_
!= old_hypertext_) {
On 2015/01/22 21:22:09, aboxhall wrote:
> Maybe early-out instead?

Only reason I don't want to is because we need to clear old_hypertext_ either
way.

[aboxhall, 2015-01-23 23:39:13]

lgtm

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

https://codereview.chromium.org/848653002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3295: if (hypertext_
!= old_hypertext_) {
On 2015/01/23 23:26:13, dmazzoni wrote:
> On 2015/01/22 21:22:09, aboxhall wrote:
> > Maybe early-out instead?
> 
> Only reason I don't want to is because we need to clear old_hypertext_ either
> way.

Yeah, you could either write that line twice, or just leave it as-is; I'm happy
with either, but I can understand wanting to avoid the duplication.

[dmazzoni, 2015-01-24 00:15:01]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-01-24 00:16:12]

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

[commit-bot: I haz the power, 2015-01-24 00:24:58]

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

[commit-bot: I haz the power, 2015-01-24 00:24:59]

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

[dmazzoni, 2015-01-26 07:55:20]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-01-26 07:56:19]

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

[commit-bot: I haz the power, 2015-01-26 08:04:22]

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

[commit-bot: I haz the power, 2015-01-26 08:04:23]

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

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

[dmazzoni, 2015-01-26 08:34:27]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-01-26 08:34:57]

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

[commit-bot: I haz the power, 2015-01-26 09:32:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-26 09:34:03]

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

[pauljensen, 2015-01-26 14:56:59]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/880523002/ by pauljensen@chromium.org.

The reason for reverting is:
DumpAccessibilityEventsTest.AccessibilityEventsTextChanged failing frequently on
Windows bots.  Fails most of the time on Win8 Aura..

[dmazzoni, 2015-01-26 23:25:10]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-01-26 23:25:41]

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

[commit-bot: I haz the power, 2015-01-27 00:34:53]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-01-27 00:38:39]

Patchset 12 (id:??) landed as
https://crrev.com/9b381f3e36dfb3dc1fd93c46573222601b31d3d7
Cr-Commit-Position: refs/heads/master@{#313179}