1475068 - When navigating next text from text leaf, pivot jumps to first link hypertext sibling

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

Given the following markup:
<p>Hello <a href="#">real</a> world</p>

If the pivot position is on the first text leaf ("Hello "), and we call pivot.moveNextByText with a word boundary, the pivot is moved to the <a> ("cruel") and has offsets of 0-4.

What should really happen is the pivot should move to the <p> with an an offset of 0-5 ("Hello").

This happens because nsAccessiblePivot::SearchForText returns the first hypertext sibling it finds before the parent.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: If accessible's parent has text interface, use it in pivot move by text. r?surkov

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #0)
> Given the following markup:
> <p>Hello <a href="#">real</a> world</p>
> 
> If the pivot position is on the first text leaf ("Hello "), and we call
> pivot.moveNextByText with a word boundary, the pivot is moved to the <a>
> ("cruel") and has offsets of 0-4.

reading as if you meant to say "real" instead "cruel"

> 
> What should really happen is the pivot should move to the <p> with an an
> offset of 0-5 ("Hello").

Struggling to understand logic. What do you mean the position is on the first text leaf "Hello "? Do you mean that aAccessible is the first text leaf "Hello " accessible? How it is different from the position on <p> withing 0-5 "Hello " offsets?

> This happens because nsAccessiblePivot::SearchForText returns the first
> hypertext sibling it finds before the parent.

The method implementation and propose look confusing. Do I understand correctly that it's supposed to find the first/last hypertext within the given anchor children or if no children, then the closest (next/prev) hypertext to the given anchor accessible? 

If so, then it appears the second inner loop doesn't handle the case of adjacent text leafs, i.e. the hierarchy like container(aAccessible) -> text_leaf text_leaf hypertext.

Comment 3

[alexander :surkov (:asurkov)]

ni? just in case

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> (In reply to Eitan Isaacson [:eeejay] from comment #0)
> > Given the following markup:
> > <p>Hello <a href="#">real</a> world</p>
> > 
> > If the pivot position is on the first text leaf ("Hello "), and we call
> > pivot.moveNextByText with a word boundary, the pivot is moved to the <a>
> > ("cruel") and has offsets of 0-4.
> 
> reading as if you meant to say "real" instead "cruel"

Oops, right.

> 
> > 
> > What should really happen is the pivot should move to the <p> with an an
> > offset of 0-5 ("Hello").
> 
> Struggling to understand logic. What do you mean the position is on the
> first text leaf "Hello "? Do you mean that aAccessible is the first text
> leaf "Hello " accessible? How it is different from the position on <p>
> withing 0-5 "Hello " offsets?
> 

right, the initial pivot position is on paragraphAccessible.firstChild. Not sure I understand the question.

Here is a walkthrough a hypothetical test. Given this markup:

<p id="p1">Here is a simple paragraph</p>
<p id="p2">Hello <a href="#" id="link">real</a> world</p>

This chunk would pass right now in central, and it would be *right*:

gQueue.push(new setVCPosInvoker(docAcc, null, null,
            getAccessible(doc.getElementById("p1")).firstChild));
gQueue.push(new setVCTextInvoker(docAcc, "moveNextByText", WORD_BOUNDARY, [0, 4], // == "Here"
            getAccessible(doc.getElementById("p1"), nsIAccessibleText)));


This chunk would pass right now in central, but it is *wrong*:

gQueue.push(new setVCPosInvoker(docAcc, null, null,
            getAccessible(doc.getElementById("p2")).firstChild));
gQueue.push(new setVCTextInvoker(docAcc, "moveNextByText", WORD_BOUNDARY, [0, 4],  // == "real"
            getAccessible(doc.getElementById("link"), nsIAccessibleText)));

If the initial position is on a text leaf, the start offset of the text traversal should be in it's offset in it's parent. In these cases, it should be in the <p> with an offset of 0, because it is the first child. Does that make sense?


> > This happens because nsAccessiblePivot::SearchForText returns the first
> > hypertext sibling it finds before the parent.
> 
> The method implementation and propose look confusing. Do I understand
> correctly that it's supposed to find the first/last hypertext within the
> given anchor children or if no children, then the closest (next/prev)
> hypertext to the given anchor accessible? 
> 
> If so, then it appears the second inner loop doesn't handle the case of
> adjacent text leafs, i.e. the hierarchy like container(aAccessible) ->
> text_leaf text_leaf hypertext.

So <p>Hello <strong>real</strong><a href="#"> world</p> ?

It's worth testing for that. I'm not sure I understand why it wouldn't work.

I think it's there for hypertext nodes who's parent is not a hypertext node, but it's sibling is (eg. table cells)

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> (In reply to alexander :surkov from comment #2)
> > (In reply to Eitan Isaacson [:eeejay] from comment #0)
> > > Given the following markup:
> > > <p>Hello <a href="#">real</a> world</p>
> > > 
> > > If the pivot position is on the first text leaf ("Hello "), and we call
> > > pivot.moveNextByText with a word boundary, the pivot is moved to the <a>
> > > ("cruel") and has offsets of 0-4.
> > 
> > reading as if you meant to say "real" instead "cruel"
> 
> Oops, right.
> 
> > 
> > > 
> > > What should really happen is the pivot should move to the <p> with an an
> > > offset of 0-5 ("Hello").
> > 
> > Struggling to understand logic. What do you mean the position is on the
> > first text leaf "Hello "? Do you mean that aAccessible is the first text
> > leaf "Hello " accessible? How it is different from the position on <p>
> > withing 0-5 "Hello " offsets?
> > 
> 
> right, the initial pivot position is on paragraphAccessible.firstChild. Not
> sure I understand the question.
> 
> Here is a walkthrough a hypothetical test. Given this markup:
> 
> <p id="p1">Here is a simple paragraph</p>
> <p id="p2">Hello <a href="#" id="link">real</a> world</p>
> 
> This chunk would pass right now in central, and it would be *right*:
> 
> gQueue.push(new setVCPosInvoker(docAcc, null, null,
>             getAccessible(doc.getElementById("p1")).firstChild));
> gQueue.push(new setVCTextInvoker(docAcc, "moveNextByText", WORD_BOUNDARY,
> [0, 4], // == "Here"
>             getAccessible(doc.getElementById("p1"), nsIAccessibleText)));
> 
> 
> This chunk would pass right now in central, but it is *wrong*:
> 
> gQueue.push(new setVCPosInvoker(docAcc, null, null,
>             getAccessible(doc.getElementById("p2")).firstChild));
> gQueue.push(new setVCTextInvoker(docAcc, "moveNextByText", WORD_BOUNDARY,
> [0, 4],  // == "real"
>             getAccessible(doc.getElementById("link"), nsIAccessibleText)));
> 
> If the initial position is on a text leaf, the start offset of the text
> traversal should be in it's offset in it's parent. In these cases, it should
> be in the <p> with an offset of 0, because it is the first child. Does that
> make sense?

Yeah, I can see the use case. Can you describe please what SearchForText method is supposed to return exactly?

> > If so, then it appears the second inner loop doesn't handle the case of
> > adjacent text leafs, i.e. the hierarchy like container(aAccessible) ->
> > text_leaf text_leaf hypertext.
> 
> So <p>Hello <strong>real</strong><a href="#"> world</p> ?

if strong element is not accessible, then yes. If it is, then <span> would work I think.

> It's worth testing for that. I'm not sure I understand why it wouldn't work.

SearchForText would return null for such case if I read the code right. It might be ok, and MoveNextByText will handle that, I'm just not sure I understand what SearchForText is supposed to return.

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #5)
> > > If so, then it appears the second inner loop doesn't handle the case of
> > > adjacent text leafs, i.e. the hierarchy like container(aAccessible) ->
> > > text_leaf text_leaf hypertext.
> > 
> > So <p>Hello <strong>real</strong><a href="#"> world</p> ?
> 
> if strong element is not accessible, then yes. If it is, then <span> would
> work I think.

I tested this, and with my patch it works fine.

> 
> > It's worth testing for that. I'm not sure I understand why it wouldn't work.
> 
> SearchForText would return null for such case if I read the code right. It
> might be ok, and MoveNextByText will handle that, I'm just not sure I
> understand what SearchForText is supposed to return.

Max Li wrote this when he implemented this text stuff. It does a pre-order tree traversal. So it will find the first (or "closest") descendant in a subtree with a text interface.

It took me a while to figure this function out because it isn't classic pre-order traversal - it "revisits" and checks parents for text interfaces as well when it navigates to the next sibling/aunt[1]. Only now, it visits siblings first which is not what we want.

My change fixes that, and it should not affect the classical subtree traversal because the parents are visited initially on the way to the children.

1. https://searchfox.org/mozilla-central/rev/448f792f9652d29daebdad21bf50b03405e40a45/accessible/base/nsAccessiblePivot.cpp#828

Comment 7

[Eitan Isaacson [:eeejay]]

It was introduced in bug 899333.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #6)

> > > So <p>Hello <strong>real</strong><a href="#"> world</p> ?
> > 
> > if strong element is not accessible, then yes. If it is, then <span> would
> > work I think.
> 
> I tested this, and with my patch it works fine.

could you add a test please while you are here?

> > > It's worth testing for that. I'm not sure I understand why it wouldn't work.
> > 
> > SearchForText would return null for such case if I read the code right. It
> > might be ok, and MoveNextByText will handle that, I'm just not sure I
> > understand what SearchForText is supposed to return.
> 
> Max Li wrote this when he implemented this text stuff. It does a pre-order
> tree traversal. So it will find the first (or "closest") descendant in a
> subtree with a text interface.
> 
> It took me a while to figure this function out because it isn't classic
> pre-order traversal - it "revisits" and checks parents for text interfaces
> as well when it navigates to the next sibling/aunt[1]. Only now, it visits
> siblings first which is not what we want.

aha, it looks indeed confusing. I think I'm good with the change, but it makes the things even more confusing imho :) Could you please add a good comment why you break there