Pivot text traversal should traverse into the subtree

RESOLVED FIXED in mozilla27

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: maxli, Assigned: maxli)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Followup to bug 886076.

This is needed, for example, to traverse into links.
(Assignee)

Comment 1

4 years ago
Created attachment 783069 [details] [diff] [review]
Patch
Attachment #783069 - Flags: review?(surkov.alexander)

Comment 2

4 years ago
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?
(Assignee)

Comment 3

4 years ago
Created attachment 784479 [details] [diff] [review]
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).
Attachment #783069 - Attachment is obsolete: true
Attachment #783069 - Flags: review?(surkov.alexander)
Attachment #784479 - Flags: review?(surkov.alexander)
(Assignee)

Comment 4

4 years ago
Comment on attachment 784479 [details] [diff] [review]
Patch v2

Changing reviewer per irc discussion.
Attachment #784479 - Flags: review?(surkov.alexander) → review?(trev.saunders)
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
Attachment #784479 - Flags: review?(trev.saunders)
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 794126 [details] [diff] [review]
Patch v3
Attachment #784479 - Attachment is obsolete: true
Attachment #794126 - Flags: review?(trev.saunders)
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
Attachment #794126 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb60799d72fe
https://hg.mozilla.org/mozilla-central/rev/bb60799d72fe
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.