899333 - Pivot text traversal should traverse into the subtree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Max Li [:maxli]]

Followup to bug 886076.

This is needed, for example, to traverse into links.

Comment 1

[Max Li [:maxli]]

Attachment: Patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 783069 [details] [diff] [review]
Patch

Review of attachment 783069 [details] [diff] [review]:
-----------------------------------------------------------------

how does it work when you have
<p>half<a>link hello</a></p>

since you are restricted by mPosition and mEnd/StartOffset

but in general code seems complicated, why don't you get a word/character after offset and if text contains embedded character then get an accessible at embed char offset and then go into it etc?

Comment 3

[Max Li [:maxli]]

Attachment: Patch v2

(In reply to alexander :surkov from comment #2)
> how does it work when you have
> <p>half<a>link hello</a></p>

Right, it didn't. I've fixed that now.

> 
> but in general code seems complicated, why don't you get a word/character
> after offset and if text contains embedded character then get an accessible
> at embed char offset and then go into it etc?

That's more or less what the patch does. A fair amount of the code is due to handling cases where you're not on a text node (my initial attempts which didn't handle that were much smaller).

Comment 4

[Max Li [:maxli]]

Comment on attachment 784479 [details] [diff] [review]
Patch v2

Changing reviewer per irc discussion.

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 784479 [details] [diff] [review]
Patch v2

the algorithm used here really needs to be explained in comments

> nsAccessiblePivot::MoveNextByText(TextBoundaryType aBoundary, bool* aResult)
>+  while (true) {
>+    HyperTextAccessible* text = mPosition->AsHyperText();
>+    Accessible* childPosition = mPosition;
>+    while (!text && (mPosition = mPosition->FirstChild()))

use a local var instead of changing mPosition all over, it will be faster and probably cleaner.

>+      text = mPosition->AsHyperText();
>+    if (!text)

nit, blank line before if

>+      mPosition = childPosition;
>+
>+    while (!text) {
>+      childPosition = mPosition;
>+      mPosition = mPosition->Parent();
>+      text = mPosition->AsHyperText();
>+    }

what are you trying to do here and why, on its face looking down the set of first children and then the parents isn't particularly sensible, shouldn't you be doing a preorder traversal of the tree with root root and starting at mPosition?  Which I suspect is something this code does in other places, templating that and only having it written in one place might be nice.

>+      Accessible* sibling = mPosition->NextSibling();
>+      if (mPosition->IsLink()) {
>+        if (sibling && sibling->IsLink() &&
>+            mPosition->EndOffset() == sibling->StartOffset()) {

how can this last part not be a constant?

> nsAccessiblePivot::MovePreviousByText(TextBoundaryType aBoundary, bool* aResult)

same as previous function more or less

Comment 6

[Max Li [:maxli]]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> what are you trying to do here and why, on its face looking down the set of
> first children and then the parents isn't particularly sensible, shouldn't
> you be doing a preorder traversal of the tree with root root and starting at
> mPosition?  Which I suspect is something this code does in other places,
> templating that and only having it written in one place might be nice.

Sure, done.


> how can this last part not be a constant?

Oops, removed it.

Comment 7

[Max Li [:maxli]]

Attachment: Patch v3

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 794126 [details] [diff] [review]
Patch v3

sorry about the lag here
>+    Accessible* childAtOffset = nullptr;
>+    for (int32_t i = tempStart; i < tempEnd; i++) {
>+      childAtOffset = text->GetChildAtOffset(i);
>+      if (childAtOffset && childAtOffset->AsHyperText()) {

test with nsAccUtils::IsEmbeddedObject() not IsHyperText()

>+    // If there's an embedded character at the very start of the range, we
>+    // instead want to traverse into it. So restart the movement with
>+    // the child as the starting point.
>+    if (childAtOffset && childAtOffset->AsHyperText() &&
>+        tempStart == childAtOffset->StartOffset()) {

same

>+Accessible*
>+nsAccessiblePivot::SearchForText(Accessible* aAccessible, bool aBackward)

This always returns a HyperTextAccessible* right? so make that its return type not Accessible*, and when you do that make sure you simplify the callers accordingly

nit, aForward and reversing the logic might make for easier reading but whatever

>+{
>+  Accessible* root = GetActiveRoot();
>+  Accessible* accessible = aAccessible;
>+  while (true) {
>+    Accessible* child = nullptr;

this is only used in the loop right? so you could do

while (Accessible* child = ...) {
  ...
}

>+    Accessible* sibling = nullptr;
>+    Accessible* temp = accessible;
>+    do {
>+      if (temp == root)
>+        break;
>+
>+      if (temp != aAccessible && temp->AsHyperText())
>+        return temp;

if (temp != aAccessible && temp->IsHyperText())
  return temp->AsHyperText();

>+    if (accessible->AsHyperText())
>+      return accessible;

same

Comment 9

[Max Li [:maxli]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb60799d72fe

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bb60799d72fe