Prune invisible views from the desktop automation tree

Prune invisible views from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree. Do the same for the desktop tree.

TEST=navigate shelf; observe you don't get to the invisible buttons->staticText nodes like sms messages.

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

[David Tseng, 2016-01-28 23:21:51]

Description was changed from

==========
Match against invisible nodes when navigating by element.
==========

to

==========
Match against invisible nodes when navigating by element.

The element predicate is sometimes used as a leaf node predicate. As a result,
we should match against the invisible state so that traversal doesn't continue.
In AutomationTreeWalker, if a leaf predicate isn't given, it uses the primary
predicate also as the leaf predicate.

shouldIgnoreLeaf takes care of skipping the entire invisible subtree of the node
(descendants don't usually hyave their invisible state set to true).


TEST=navigate shelf; observe you don't get to the invisible buttons like sms
messages.
==========

[David Tseng, 2016-01-28 23:21:51]

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

[David Tseng, 2016-02-04 22:25:14]

Description was changed from

==========
Match against invisible nodes when navigating by element.

The element predicate is sometimes used as a leaf node predicate. As a result,
we should match against the invisible state so that traversal doesn't continue.
In AutomationTreeWalker, if a leaf predicate isn't given, it uses the primary
predicate also as the leaf predicate.

shouldIgnoreLeaf takes care of skipping the entire invisible subtree of the node
(descendants don't usually hyave their invisible state set to true).


TEST=navigate shelf; observe you don't get to the invisible buttons like sms
messages.
==========

to

==========
Match against invisible nodes when navigating by element.

The element predicate is sometimes used as a leaf node predicate. As a result,
we should match against the invisible state so that traversal doesn't continue
(downward). In AutomationTreeWalker, if a leaf predicate isn't given, it uses
the primary predicate also as the leaf predicate.

shouldIgnoreLeaf takes care of skipping the entire invisible subtree of the node
(descendants don't usually have their invisible state set to true).

Example:
sms button with state invisible -> sms messages staticText with no state

results in us putting range on the child static text.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

[David Tseng, 2016-02-04 22:25:36]

On 2016/01/28 23:22:09, David Tseng wrote:

Friendly ping.

[dmazzoni, 2016-02-04 23:01:04]

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

[dmazzoni, 2016-02-04 23:01:05]

I understand why this works, but it feels like the wrong place to put the code.
It just seems counterintuitive to return true for a node that you don't actually
want to land on.

Is there any way we could have the tree walker check for this directly? Or make
it into a separate predicate like shouldSkipSubtree?

[David Tseng, 2016-02-04 23:40:04]

Description was changed from

==========
Match against invisible nodes when navigating by element.

The element predicate is sometimes used as a leaf node predicate. As a result,
we should match against the invisible state so that traversal doesn't continue
(downward). In AutomationTreeWalker, if a leaf predicate isn't given, it uses
the primary predicate also as the leaf predicate.

shouldIgnoreLeaf takes care of skipping the entire invisible subtree of the node
(descendants don't usually have their invisible state set to true).

Example:
sms button with state invisible -> sms messages staticText with no state

results in us putting range on the child static text.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

to

==========
Prune invisible nodes from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree.
Do the same for the desktop tree.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

[David Tseng, 2016-02-04 23:40:20]

On 2016/02/04 23:01:05, dmazzoni wrote:
> I understand why this works, but it feels like the wrong place to put the
code.
> It just seems counterintuitive to return true for a node that you don't
actually
> want to land on.
> 
> Is there any way we could have the tree walker check for this directly? Or
make
> it into a separate predicate like shouldSkipSubtree?

PTAL. Went with another solution.

[dmazzoni, 2016-02-04 23:57:39]

lgtm

Awesome, this solution works great.

There are probably a couple of places we need CHILDREN_CHANGED events that we
don't have them currently...

[David Tseng, 2016-02-05 00:31:46]

On Thu, Feb 4, 2016 at 3:57 PM, <dmazzoni@chromium.org> wrote:

> lgtm
>
> Awesome, this solution works great.
>
> There are probably a couple of places we need CHILDREN_CHANGED events that we
> don't have them currently...
>
>
Uploaded a new patchset with potentials. PTAL.



> https://codereview.chromium.org/1644863003/
>
> --
> 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, 2016-02-05 00:32:11]

Description was changed from

==========
Prune invisible nodes from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree.
Do the same for the desktop tree.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

to

==========
Prune invisible views from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree.
Do the same for the desktop tree.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

[David Tseng, 2016-02-05 00:43:35]

dtseng@chromium.org changed reviewers:
- dmazzoni@chromium.rog

[David Tseng, 2016-02-05 01:15:27]

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

[David Tseng, 2016-02-09 23:54:40]

+ sky for OWNERS

[sky, 2016-02-10 16:19:41]

LGTM assuming CHILDREN_CHANGED is right for changes to visibility, and some
comments would be nice.

https://codereview.chromium.org/1644863003/diff/60001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1644863003/diff/60001/ui/views/view.cc#newcod...
ui/views/view.cc:414:
parent_->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false);
I don't see any documentation for the AXEvent constants. Is CHILDREN_CHANGED
used for changes to visibility? It would be good if there was a comment
indicating this.

[David Tseng, 2016-02-11 22:56:23]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-02-11 22:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1644863003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-02-11 22:57:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1644863003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1644863003/60001

[commit-bot: I haz the power, 2016-02-12 00:10:16]

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

[commit-bot: I haz the power, 2016-02-12 00:10:17]

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

[David Tseng, 2016-02-12 00:48:00]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-02-12 00:48:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1644863003/#ps80001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-02-12 00:48:13]

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

[commit-bot: I haz the power, 2016-02-12 02:14:09]

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

[commit-bot: I haz the power, 2016-02-12 02:14:10]

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

[David Tseng, 2016-02-12 17:18:03]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-02-12 17:18:16]

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

[commit-bot: I haz the power, 2016-02-12 18:35:37]

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

[commit-bot: I haz the power, 2016-02-12 18:35:38]

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

[David Tseng, 2016-02-12 19:21:26]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-02-12 19:21:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1644863003/#ps100001
(title: "Try hitting space (behavior changes when a tab is open).")

[commit-bot: I haz the power, 2016-02-12 19:23:46]

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

[commit-bot: I haz the power, 2016-02-12 20:07:07]

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

[commit-bot: I haz the power, 2016-02-12 20:07:08]

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

[dtseng, 2016-02-12 21:52:38]

The CQ bit was checked by dtseng@google.com

[commit-bot: I haz the power, 2016-02-12 21:53:09]

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

[commit-bot: I haz the power, 2016-02-12 23:17:03]

Description was changed from

==========
Prune invisible views from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree.
Do the same for the desktop tree.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

to

==========
Prune invisible views from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree.
Do the same for the desktop tree.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

[commit-bot: I haz the power, 2016-02-12 23:17:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:45:33]

Description was changed from

==========
Prune invisible views from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree.
Do the same for the desktop tree.


TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.
==========

to

==========
Prune invisible views from the desktop automation tree

In the Blink accessibility tree, invisible nodes are not included in the tree.
Do the same for the desktop tree.

TEST=navigate shelf; observe you don't get to the invisible buttons->staticText
nodes like sms messages.

Committed: https://crrev.com/e84d40fb1f37d32dc8b384e26dac19d59011d99f
Cr-Commit-Position: refs/heads/master@{#375273}
==========

[commit-bot: I haz the power, 2016-02-16 22:45:34]

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