Closed Bug 371680 Opened 15 years ago Closed 14 years ago

Expose nsIAccessibleText::scrollSubstringTo

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(1 file, 5 obsolete files)

 
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.
Keywords: access
Attached patch patch (obsolete) — Splinter Review
(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.
Attachment #256786 - Flags: review?(aaronleventhal)
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.
Attachment #256786 - Flags: review?(aaronleventhal)
(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?
> 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?
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)
Attached patch in progress (obsolete) — Splinter Review
Attachment #256786 - Attachment is obsolete: true
Attached patch patch3 (obsolete) — Splinter Review
Attachment #277091 - Attachment is obsolete: true
Attachment #277213 - Flags: superreview?(roc)
Attachment #277213 - Flags: review?(roc)
Attachment #277213 - Flags: review?(aaronleventhal)
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?
Attachment #277213 - Flags: review?(aaronleventhal) → review-
Attached patch patch4 (obsolete) — Splinter Review
with aaron's recommendations
Attachment #277213 - Attachment is obsolete: true
Attachment #277273 - Flags: superreview?(roc)
Attachment #277273 - Flags: review?(aaronleventhal)
Attachment #277213 - Flags: superreview?(roc)
Attachment #277213 - Flags: review?(roc)
Attachment #277273 - Flags: review?(roc)
(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 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().
Attachment #277273 - Flags: review?(aaronleventhal) → review-
Attached patch patch5 (obsolete) — Splinter Review
Attachment #277273 - Attachment is obsolete: true
Attachment #277534 - Flags: review?(aaronleventhal)
Attachment #277273 - Flags: superreview?(roc)
Attachment #277273 - Flags: review?(roc)
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 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?
Attachment #277534 - Flags: review?(aaronleventhal) → review+
(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.
Attachment #277534 - Flags: superreview?(roc)
Attachment #277534 - Flags: review?(roc)
(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 :).
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.
(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 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.
Attachment #277534 - Flags: superreview?(roc)
Attachment #277534 - Flags: superreview+
Attachment #277534 - Flags: review?(roc)
Attachment #277534 - Flags: review+
Attachment #277534 - Flags: approval1.9?
Do I need additional reviews for this bug before I land it?
Status: NEW → ASSIGNED
Comment on attachment 277534 [details] [diff] [review]
patch5

you can go ahead and land this.
Attachment #277534 - Flags: approval1.9? → approval1.9+
Summary: Expose nsIAccessibleText::scrollToSubstring → Expose nsIAccessibleText::scrollSubstringTo
Attached patch patch6Splinter Review
checked in patch, updated to trunk, fixed roc's recommendations
Attachment #277534 - Attachment is obsolete: true
checked in
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.