630001 - use cached text of text leaf accessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: part1v1: fix AppendTextTo (landed b11)

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Created attachment 508216 [details] [diff] [review]
> part1v1: fix AppendTextTo

fixed crashes bug 612098 and bug 615475.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: part2v1: fix nsAccUtils::TextLength (landed b11)

Comment 4

[alexander :surkov (:asurkov)]

Attachment: pat3v1: fix GetText (landed b11)

Comment 5

[Fernando Herrera]

Attachment: GetText rewrite WIP

Notice that this WIP patch removes lot of functions I planned to rewrite too. Just ignore those removals.

Comment 6

[Fernando Herrera]

Regarding part3 patch:

So I like the idea of having a cached text on the a11y side updated when the content is modified.

Also having a generic GetText function that other GetText related functions (At/Before/After) call is a good idea IMHO and helps making things easier to follow and maintain. The performance penalty of this approach (calling several times GetText() until we find the desired boundaries) is minimized is we are caching the text in our side.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> Also having a generic GetText function that other GetText related functions
> (At/Before/After) call is a good idea IMHO and helps making things easier to
> follow and maintain. The performance penalty of this approach (calling several
> times GetText() until we find the desired boundaries) is minimized is we are
> caching the text in our side.

GetText might be not very suitable for other getText(At...) functions since they may be more performant if they don't use it. I thought to have special functions (well with some dupe code but pretty trivial like in GetText) for each boundary. That allows us to be fast as possible, at least we can avoid excess search of child index by offset (well it's fast but not o(1)). In other words once we get a child index then we run backward/forward through children until we reach needed boundaries. That's the way I would go.

Comment 8

[alexander :surkov (:asurkov)]

part1: fix AppendTextTo - http://hg.mozilla.org/mozilla-central/rev/0a5bf58306bc
part2: fix nsAccUtils::TextLength - http://hg.mozilla.org/mozilla-central/rev/ba5140cd34c8

landed on 2.0 beta 11

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 508226 [details] [diff] [review]
pat3v1: fix GetText (landed b11)

r=me a nice improvement. (Worried about bitrotting fer's work but we shouldn't
wait).

>+nsHyperTextAccessible::GetText(PRInt32 aStartOffset, PRInt32 aEndOffset,
>+                               nsAString &aText)
> {

>-  return GetPosAndText(aStartOffset, aEndOffset, &aText) ? NS_OK : NS_ERROR_FAILURE;

Wow GetPosAndText is messy.

>+		test_richtext.html \
>+++ b/accessible/tests/mochitest/text/test_richtext.html

I'm uncomfortable with 'richtext' in the name here because there is such a strong association with RTF (Rich Text Format). How about: test_complextext?
(Whatever is chosen find and replace for the richtext ID as well).
>+      var IDs = [ "richtext" ];
>+  <div id="richtext">hello <a>friend</a> see <img></div>

>+    function doTest()
>+    {
>+      // ! - embedded object char
>+      // @ - imaginary object char, space is used

I had to go read bug 397485 to understand this :)

Do we have text coverage for bullets? Like <div>hello <ol><li>foo</li></ol> there</div>

Comment 10

[alexander :surkov (:asurkov)]

part3: fix getText with David's comment landed on 2.0 (fx4beta11) - http://hg.mozilla.org/mozilla-central/rev/6a2099306114

Comment 11

[alexander :surkov (:asurkov)]

Attachment: part4v1: fix GetTextAt/Before/After for boundary char (landed b12)

Comment 12

[Fernando Herrera]

Comment on attachment 509378 [details] [diff] [review]
part4v1: fix GetTextAt/Before/After for boundary char (landed b12)

For GetText{At,After,Before}Offset functions with BOUNDARY_CHAR we are returning NS_OK even if GetCharAt fatils (invalid offset). However for GetCharacterAtOffset we may return NS_ERROR_INVALID_ARG if GetChatAt fails. Is there any reason for this?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> Comment on attachment 509378 [details] [diff] [review]
> part4v1: fix GetTextAt/Before/After for boundary char
> 
> For GetText{At,After,Before}Offset functions with BOUNDARY_CHAR we are
> returning NS_OK even if GetCharAt fatils (invalid offset). However for
> GetCharacterAtOffset we may return NS_ERROR_INVALID_ARG if GetChatAt fails. Is
> there any reason for this?

Well you're bug about ATK doesn't want failures makes be doubt. We don't fail always on wrong offsets now, after the patch we fail never. It's good for XPCOM and for ATK I guess but that's wrong for IA2. We need to clean up this mess, but I afraid after fx4. So, it wouldn't be easy to preserve current behavior. What option would you choose?

Comment 14

[Fernando Herrera]

With this patch we are failing on getCharacterAtOffset if GetCharAt fails:

553 nsAutoString character;
554	  if (GetCharAt(aOffset, eGetAt, character)) {
555	    *aCharacter = character.First();
556	    return NS_OK;
557	  }	
558
559	  return NS_ERROR_INVALID_ARG;

doing this in the mochitest:
var textacc = getAccessible("div", [nsIAccessibleText]);
textacc.getCharacterAtOffset(800);

results in:

failed | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAccessibleText.getCharacterAtOffset]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://mochitests/content/a11y/accessible/text/test_singleline.html :: doTest :: line 532" data: no] at :0

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> With this patch we are failing on getCharacterAtOffset if GetCharAt fails:

It failed previously in this case, all I did is I've changed generic failure to invalid index.

Comment 16

[Fernando Herrera]

Comment on attachment 509378 [details] [diff] [review]
part4v1: fix GetTextAt/Before/After for boundary char (landed b12)

Do you think we should unify these behaviours?

regardless of this open question, the patch looks ok. r=me

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> Created attachment 509378 [details] [diff] [review]
> part4v1: fix GetTextAt/Before/After for boundary char

landed on 2.0 (fx4beta12) - http://hg.mozilla.org/mozilla-central/rev/e9fe7402dc53

Comment 18

[alexander :surkov (:asurkov)]

Masayuki, could you please advise a best way how to find word start and word end in the string before and after the given position? I'm looking at nsILineBreaker but it sounds its Next/Prev methods aren't exactly we need.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

They define "word" is one or more consecutive characters which isn't breakable. So, I think they're not useful for you.

There is nsIWordBreaker but I don't know the behavior of it.
http://mxr.mozilla.org/mozilla-central/source/intl/lwbrk/public/nsIWordBreaker.h

I guess if you want to get real word, you need to write such method. But I'm not sure how to write it. I think there are some issues:

1. There are many white spaces, we need to check them all.
2. Some languages are not using white spaces for word separator, e.g., Japanese, Chinese and Thai.
3. How about for parentheses, punctuations and symbols (e.g., ☆)?

Comment 20

[Fernando Herrera]

(In reply to comment #19)

> There is nsIWordBreaker but I don't know the behavior of it.
> http://mxr.mozilla.org/mozilla-central/source/intl/lwbrk/public/nsIWordBreaker.h
> 
> I guess if you want to get real word, you need to write such method. But I'm
> not sure how to write it. I think there are some issues:

The ideal situation would be to use code from the different platform backends to do this. At least the pango one provides this information:

http://git.gnome.org/browse/pango/tree/pango/pango-break.h#n53

We should take a look at the Carbon and Windows ones to check if we can easily get this information from them.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (In reply to comment #19)
> 
> > There is nsIWordBreaker but I don't know the behavior of it.
> > http://mxr.mozilla.org/mozilla-central/source/intl/lwbrk/public/nsIWordBreaker.h
> > 
> > I guess if you want to get real word, you need to write such method. But I'm
> > not sure how to write it. I think there are some issues:
> 
> The ideal situation would be to use code from the different platform backends
> to do this.

I don't agree it's ideal since we should do what Gecko does, i.e. should use the same word term definition, if user moves by words (ctrl+arrow) then a11y should report consistent results.

Comment 22

[Fernando Herrera]

(In reply to comment #21) 
> I don't agree it's ideal since we should do what Gecko does, i.e. should use
> the same word term definition, if user moves by words (ctrl+arrow) then a11y
> should report consistent results.

Good point. Then we should try to share the word breaking code (not only definitions) with caret navigation (the Frame Peek stuff) and any other part of ff doing word breaking.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Should further work continue on this bug or can we open a new bug for it?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> Should further work continue on this bug or can we open a new bug for it?

we can both I think. Perhaps it's preferable to open new bug if we're not in time to fix it in fx4 timeframe.

Comment 25

[alexander :surkov (:asurkov)]

I filed bugs for remaining work: bug 634201 and bug 634202

marking this one as fixed.