Add a |skipInitialAncestry| option to tree walking.

Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward. For example, suppose CV focus is on a static texxt, searching backwards for a heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and |skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*
In a Google search results page, navigate by headings and links. Ensure consistency of navigation (we should land on the individual links inside of the headings).

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/391186ab1240b07335ecf82e0504441c5db3c9f7
Cr-Commit-Position: refs/heads/master@{#391870}

[David Tseng, 2016-05-04 22:58:52]

Description was changed from

==========
Add a |skipInitialAncestry| option to tree walking.
==========

to

==========
Add a |skipInitialAncestry| option to tree walking.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-05-04 23:00:56]

Description was changed from

==========
Add a |skipInitialAncestry| option to tree walking.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward.
For example, suppose CV focus is on a static texxt, searching backwards for a
heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and
|skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-05-04 23:00:56]

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

[David Tseng, 2016-05-04 23:04:52]

Description was changed from

==========
Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward.
For example, suppose CV focus is on a static texxt, searching backwards for a
heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and
|skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward.
For example, suppose CV focus is on a static texxt, searching backwards for a
heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and
|skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*
In a Google search results page, navigate by headings and links. Ensure
consistency of navigation (we should land on the individual links inside of the
headings).

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-05-04 23:30:35]

Could you add a test to the tree walker test suite?

I think there are cases where we might not want to skip ancestors, like if
they're focusable.

What happens with this html?

<div tabindex=0 aria-label="Link container">
  <a href="#">Link 1</a>
  <a href="#">Link 2</a>
</div>

We should navigate to the container and then the two links when going forward,
and the links and then the container when going backward.

Question: would you be able to solve the bug that led you here if you did some
sort of sync() (like forward / backward) before any tree walking? I think the
key is that the current behavior is incorrect if the current range is not the
exact node you'd land on if traversing at that granularity.

[David Tseng, 2016-05-04 23:49:46]

On Wed, May 4, 2016 at 4:30 PM, <dmazzoni@chromium.org> wrote:

> Could you add a test to the tree walker test suite?
>
> I think there are cases where we might not want to skip ancestors, like if
> they're focusable.
>

Note that the current cl only skips ancestors if using a jump command. This
isn't happening in general.


>
> What happens with this html?
>
> <div tabindex=0 aria-label="Link container">
> <a href="#">Link 1</a>
> <a href="#">Link 2</a>
> </div>
>
> We should navigate to the container and then the two links when going
> forward,
> and the links and then the container when going backward.
>
> Question: would you be able to solve the bug that led you here if you did
> some
> sort of sync() (like forward / backward) before any tree walking? I think
> the
> key is that the current behavior is incorrect if the current range is not
> the
> exact node you'd land on if traversing at that granularity.
>
>
> The bug is that jump navigation is a lie. We don't actually want to be on
a heading or link, but possibly a descendant of that node. The logic for
where to go is pretty vague (as it was in Classic).

But, I think the specific case of jumping should disregard ancestors of the
original node. You can see the result in Classic when we went with syncing;
I think it's a lot more messy and confusing.


>

-- 
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-05-05 16:23:35]

On 2016/05/04 23:49:46, David Tseng wrote:
> On Wed, May 4, 2016 at 4:30 PM, <mailto:dmazzoni@chromium.org> wrote:
> 
> > Could you add a test to the tree walker test suite?

Done.

> >
> > I think there are cases where we might not want to skip ancestors, like if
> > they're focusable.
> >
> 
> Note that the current cl only skips ancestors if using a jump command. This
> isn't happening in general.
> 
> 
> >
> > What happens with this html?
> >
> > <div tabindex=0 aria-label="Link container">
> > <a href="#">Link 1</a>
> > <a href="#">Link 2</a>
> > </div>
> >
> > We should navigate to the container and then the two links when going
> > forward,
> > and the links and then the container when going backward.
> >
> > Question: would you be able to solve the bug that led you here if you did
> > some
> > sort of sync() (like forward / backward) before any tree walking? I think
> > the
> > key is that the current behavior is incorrect if the current range is not
> > the
> > exact node you'd land on if traversing at that granularity.
> >
> >
> > The bug is that jump navigation is a lie. We don't actually want to be on
> a heading or link, but possibly a descendant of that node. The logic for
> where to go is pretty vague (as it was in Classic).
> 
> But, I think the specific case of jumping should disregard ancestors of the
> original node. You can see the result in Classic when we went with syncing;
> I think it's a lot more messy and confusing.
> 
> 
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-05-05 16:32:47]

lgtm

[David Tseng, 2016-05-05 18:06:27]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-05-05 18:06:27]

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/1948213002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-05-05 18:07:10]

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

[commit-bot: I haz the power, 2016-05-05 19:34:10]

Description was changed from

==========
Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward.
For example, suppose CV focus is on a static texxt, searching backwards for a
heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and
|skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*
In a Google search results page, navigate by headings and links. Ensure
consistency of navigation (we should land on the individual links inside of the
headings).

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward.
For example, suppose CV focus is on a static texxt, searching backwards for a
heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and
|skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*
In a Google search results page, navigate by headings and links. Ensure
consistency of navigation (we should land on the individual links inside of the
headings).

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-05-05 19:34:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-05 19:36:31]

Description was changed from

==========
Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward.
For example, suppose CV focus is on a static texxt, searching backwards for a
heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and
|skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*
In a Google search results page, navigate by headings and links. Ensure
consistency of navigation (we should land on the individual links inside of the
headings).

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add a |skipInitialAncestry| option to tree walking.

This is important when we want to ignore ancestry chain when moving backward.
For example, suppose CV focus is on a static texxt, searching backwards for a
heading, we might end up at an ancestor of the static text. This is undesirable.

Also, made the phases useful by using them for |skipInitialSubtree| and
|skipInitialAncestry|.

TEST=AutomationTreeWalkerTest.*
In a Google search results page, navigate by headings and links. Ensure
consistency of navigation (we should land on the individual links inside of the
headings).

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/391186ab1240b07335ecf82e0504441c5db3c9f7
Cr-Commit-Position: refs/heads/master@{#391870}
==========

[commit-bot: I haz the power, 2016-05-05 19:36:32]

Patchset 3 (id:??) landed as
https://crrev.com/391186ab1240b07335ecf82e0504441c5db3c9f7
Cr-Commit-Position: refs/heads/master@{#391870}