(In reply to comment #1) > Created attachment 508216 [details] [diff] [review] > part1v1: fix AppendTextTo fixed crashes bug 612098 and bug 615475.

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

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.

(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.

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 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>

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

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?

(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?

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

(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 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

(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

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.

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., ☆)?

(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.

(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.

(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.

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

(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.

I filed bugs for remaining work: bug 634201 and bug 634202 marking this one as fixed.