371680 - Expose nsIAccessibleText::scrollSubstringTo

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Comment 1

[Aaron Leventhal]

Please see nsTextAccessibleWrap::scrollToSubstring()
http://lxr.mozilla.org/seamonkey/source/accessible/src/msaa/nsTextAccessibleWrap.cpp#159

This particular IA2 method may be changed, to add parameters that specify how the scrolling will be done. However we probably won't know until some time in April.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

(In reply to comment #1)
> This particular IA2 method may be changed, to add parameters that specify how
> the scrolling will be done. However we probably won't know until some time in
> April.
> 

Ok, if it will be changed then I fix it.

Comment 3

[Aaron Leventhal]

Actually this code is based on a mistake I made.

nsTextAccessible should not end up inheriting from nsHyperTextAccessible, because it's not a text container. It's a leaf. I believe we did that just because all text and image descendants of a link are marked with STATE_LINKED and possibly STATE_TRAVERSED, so we got that for free.

We will need to file a bug to "nsTextAccessible should not be an nsHyperTextAccessible" and fix that carefully.

In the mean time, you will have to either:
1) Reverse the changes to nsTextAccessibleWrap, and have some duplicated code
2) Find a different commonality between the 2 ScrollToSubstring impl's
3) Go up the ancestor chain until you find the first nsIAccessibleText, and use the scroll to substring on that (nsCaretAccessible does something similar to fire caret move events). You may not save much code with this methed, however. You'd have to recalculate the indices. Too much work.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> 3) Go up the ancestor chain until you find the first nsIAccessibleText, and use
> the scroll to substring on that (nsCaretAccessible does something similar to
> fire caret move events). You may not save much code with this methed, however.
> You'd have to recalculate the indices. Too much work.
> 

I wonder how should I change aStartIndex/aEndIndex to pass them to parent nsIAccesibleText? Also, how to implement scrollToSubstring on hypertext, should it be something similiar you do in patch for bug 366438, i.e. to run through accessible nodes only?

Comment 5

[Aaron Leventhal]

> I wonder how should I change aStartIndex/aEndIndex to pass them to parent
nsIAccesibleText?
Look at how nsIAccessibleHyperlink gets the start index for the current link. It's basically the same logic.

> Also, how to implement scrollToSubstring on hypertext, should
> it be something similiar you do in patch for bug 366438, i.e. to run 
> through accessible nodes only?
Can you explain more?

Comment 6

[alexander :surkov (:asurkov)]

note for me: don't forget to use clickable (linkable) accessbile instead hyper text in nsAccessibilityService::GetAccessible if node has @click attribute (http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsAccessibilityService.cpp#1249)

Comment 7

[alexander :surkov (:asurkov)]

Attachment: in progress

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 9

[Aaron Leventhal]

Comment on attachment 277213 [details] [diff] [review]
patch3

This isn't converting from hypertext offsets to content offsets. Also, the start offset could fall in one node, and the end offset could be in a different node.

+NS_IMETHODIMP
+nsHyperTextAccessible::ScrollSubstringTo(PRInt32 aStartIndex, PRInt32 aEndIndex,
+                                         PRUint32 aScrollType)
+{
+  return nsAccUtils::ScrollSubstringTo(GetFrame(), mDOMNode,
+                                       aStartIndex, aEndIndex, aScrollType);
+}
+

Also, a nit: please rename ScrollSubstringTo -> ScrollToSubstring.

How did you test?

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch4

with aaron's recommendations

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> Also, a nit: please rename ScrollSubstringTo -> ScrollToSubstring.

Not sure it's correct because we scroll substring to some area of window specified by aScrollType. It's matched with IA2.

> How did you test?
> 

by simple js test.

Comment 12

[Aaron Leventhal]

Comment on attachment 277273 [details] [diff] [review]
patch4

Surkov, I believe startFrame or endFrame may not be a text frame, in the  case that the start or end offset land on an embedded object character. RenderedToContentOffset() probably returns a failure in that case.

+  nsresult rv = RenderedToContentOffset(startFrame, renderedStartOffset,
+                                        &startOffset);
+  NS_ENSURE_SUCCESS(rv, rv);
...

n fact instead of |if (startOffset ==0)| you may want to make  that conditional on if the frame is not a text frame. Then in the else you can call the RenderedToContentOffset().

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 14

[Aaron Leventhal]

Comment on attachment 277534 [details] [diff] [review]
patch5

Surkov, why is it sometimes called ScrollToSubstring() and other times called ScrollSubstringTo(). It should always just be ScrollToSubstring()

Comment 15

[Aaron Leventhal]

Comment on attachment 277534 [details] [diff] [review]
patch5

Please don't forget to rename ScrollSubstringTo to ScrollToSubstring. 

The code looks right -- did you look at whether it's working correctly with different kinds of start and end points?

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (From update of attachment 277534 [details] [diff] [review])
> Surkov, why is it sometimes called ScrollToSubstring() and other times called
> ScrollSubstringTo(). It should always just be ScrollToSubstring()
> 

Why (please see comment 11)? It should be always ScrollSubstringTo excepting MSAA one.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> The code looks right -- did you look at whether it's working correctly with
> different kinds of start and end points?
> 

I didn't do complex tests yet. But it scrolls substrings of simple text accessible into view. Just it would be fine to get kind of working code to get some platform to for further testing/fixing :).

Comment 18

[Aaron Leventhal]

Now I see comment 5. I'd rather have a consistent name, because you could also say the window is scrolling to show the substring. But it's okay with me either way, if you don't want to change it.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> Now I see comment 5. I'd rather have a consistent name, because you could also
> say the window is scrolling to show the substring. But it's okay with me either
> way, if you don't want to change it.
> 

Really I just like to be consistent with IA2.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 277534 [details] [diff] [review]
patch5

-  if (aEndFrame) {
+  if (aEndFrame)
     *aEndFrame = nsnull;
-  }
-  if (aBoundsRect) {
+  if (aBoundsRect)
     aBoundsRect->Empty();
-  }

Don't do this. I prefer to have braces around non-control-transfer single statements. I don't mind if you don't, but don't churn the code just for the sake of it.

Comment 21

[alexander :surkov (:asurkov)]

Do I need additional reviews for this bug before I land it?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 277534 [details] [diff] [review]
patch5

you can go ahead and land this.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch6

checked in patch, updated to trunk, fixed roc's recommendations

Comment 24

[alexander :surkov (:asurkov)]

checked in