use cached text of text leaf accessibles

RESOLVED FIXED in mozilla2.0b12

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

unspecified
mozilla2.0b12
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [softblocker] for remaining work)

Attachments

(5 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

8 years ago
Created attachment 508216 [details] [diff] [review]
part1v1: fix AppendTextTo (landed b11)
Attachment #508216 - Flags: review?(bolterbugz)
(Assignee)

Comment 2

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

fixed crashes bug 612098 and bug 615475.
Blocks: 615475, 612098
(Assignee)

Updated

8 years ago
No longer blocks: 626660
Depends on: 626660
(Assignee)

Comment 3

8 years ago
Created attachment 508217 [details] [diff] [review]
part2v1: fix nsAccUtils::TextLength (landed b11)
Attachment #508217 - Flags: review?(bolterbugz)
(Assignee)

Comment 4

8 years ago
Created attachment 508226 [details] [diff] [review]
pat3v1: fix GetText (landed b11)
Attachment #508226 - Flags: review?(bolterbugz)
(Assignee)

Updated

8 years ago
Blocks: 630013
No longer blocks: 625653
blocking2.0: --- → betaN+

Comment 5

8 years ago
Created attachment 508411 [details] [diff] [review]
GetText rewrite WIP

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

Updated

8 years ago
Attachment #508226 - Flags: review?(bolterbugz) → review?(fherrera)
Attachment #508216 - Flags: review?(bolterbugz) → review+
Attachment #508217 - Flags: review?(bolterbugz) → review+

Comment 6

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

Comment 7

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

Comment 8

8 years ago
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>
Attachment #508226 - Flags: review+
(Assignee)

Comment 10

8 years ago
part3: fix getText with David's comment landed on 2.0 (fx4beta11) - http://hg.mozilla.org/mozilla-central/rev/6a2099306114
Whiteboard: [softblocker]
Whiteboard: [softblocker] → [softblocker] for remaining work
(Assignee)

Updated

8 years ago
Depends on: 631160
(Assignee)

Comment 11

8 years ago
Created attachment 509378 [details] [diff] [review]
part4v1: fix GetTextAt/Before/After for boundary char (landed b12)
Attachment #509378 - Flags: review?(fherrera)
(Assignee)

Updated

8 years ago
Attachment #508226 - Flags: review?(fherrera)

Comment 12

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

Comment 13

8 years ago
(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

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

Comment 15

8 years ago
(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

8 years ago
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
Attachment #509378 - Flags: review?(fherrera) → review+
(Assignee)

Comment 17

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

Comment 18

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

Comment 20

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

Comment 21

8 years ago
(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

8 years ago
(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.
Attachment #508216 - Attachment description: part1v1: fix AppendTextTo → part1v1: fix AppendTextTo (landed b11)
Attachment #508217 - Attachment description: part2v1: fix nsAccUtils::TextLength → part2v1: fix nsAccUtils::TextLength (landed b11)
Attachment #508226 - Attachment description: pat3v1: fix GetText → pat3v1: fix GetText (landed b11)
Attachment #509378 - Attachment description: part4v1: fix GetTextAt/Before/After for boundary char → part4v1: fix GetTextAt/Before/After for boundary char (landed b12)
Should further work continue on this bug or can we open a new bug for it?
(Assignee)

Comment 24

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

Comment 25

8 years ago
I filed bugs for remaining work: bug 634201 and bug 634202

marking this one as fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.