448744 - IAccessibleText::caretOffset should return -1 if the system caret is not currently with in that particular object.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Curran]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008072603 Minefield/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008072603 Minefield/3.1a2pre ID:2008072603

It has been agreed upon that the specification for IAccessibleText state that if the caret is not actually in a particular object, then IAccessibleText::caretOffset return -1.
See http://www.linuxfoundation.org/en/Accessibility/IAccessible2/Minutes/20080729
For when it was talked about.
Currently it seems in Gecko, if you ask for IAccessibleText::caretOffset and the caret is not with in that object, then the offset returned seems to be the length of the text (IAccessibleText::nCharacters).
It is useful for ATs to be able to very quickly work out if the system caret is in a particular object, by returning -1 if its not will provide this.
Specifically this will help improve NVDA's support for rich editable documents such as Thunderbird html message composition.

Note that as Gecko Accessibility hyerarchy provides a caretOffset for parents of the object with the caret, because of embedded object characters etc, this should still stay unchanged. The only place where -1 should be returned is where the system caret is not in this object, or any of its descendants.

Perhaps returning -1 may also be a good idea for other accessibility APIs such as ATK, though I guess this would need to be talked about with ATs who use that support, to see if it would be at all useful.

Reproducible: Always

Steps to Reproduce:
1. reply to an existing message in Thunderbird, making sure you are composing messages in HTML 
2. move the system caret so it is on the first character of the quoted text
3. Using Accessibility probe, look at the object representing the quoted text and notice that the caret offset is 0.
4. move the system caret up out of the quoted text in to the start of the document the "blalblalbla wrote:" bit.
5. with Accessibility probe, take another look at the object representing the quoted text and note what its caret offset is.
Actual Results:  
The caret offset when the system caret is not there is the offset past the last character in the quoted text (e.g. same as IAccessibleText::nCharacters).

Expected Results:  
caret offset should be -1.

Comment 1

[Aaron Leventhal]

-1 offset means end of text usually.

Should it throw an exception instead?

Comment 2

[Michael Curran]

Throwing an exception to the client would be also fine.

In IAccessible2 -1 does mean end of text in situations where the client is providing an offset to the server. I'm not aware of any situations though where the server provides -1 to the client to represent end of text, though I could be mistaken.

An exception or -1 is perfectly fine for me.

The only thing NVDA can't deal with is the hresult being S_FALSE. Or I should say, if it is S_FALSE, the actual offset must be -1 as high level languages such as Python can't get access to the hresult for S_OK and S_FALSE.

Comment 3

[Aaron Leventhal]

Will someone ask what the correct behavior is on the IA2 list?
I agree we never pass back -1 to mean end of string, but it did seem like a collision that might cause confusion for someone somewhere.

Comment 4

[Pete Brunet]

The existing IA2 spec defines -1 as an input parameter for specifying the offset on IAText or IAEditableText methods.  In this particular case we're talking about the output parameter on the call to IAText::caretOffset.

I don't think this will be a point of confusion because someone setting the input parameter would be using the enum value of IA2_TEXT_OFFSET_LENGTH.

What do you think Aaron?

 

Comment 5

[Aaron Leventhal]

As long as it's in the spec for caretOffset I don't mind. Should getting the selection do the same thing?

Comment 6

[Michael Curran]

(In reply to comment #5)
> As long as it's in the spec for caretOffset I don't mind. Should getting the
> selection do the same thing?
As long as nSelections returns 0 if there is no selection, then that is good enough in regards to selection.

Comment 7

[Aaron Leventhal]

If the caret is in a descendant we should still put the offset on the embedded obj char for that, I believe.

This is blocking NVDA support for rich text editing!

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 350318 [details] [diff] [review]
patch

>+              is(test.caretOffsetFromEvent, test.caretOffset,
>+                 "Wrong caret offset in test " + gTestIdx);

This should just be "idx", right? otherwise you'd always put in the same number, not the loop counter.
And one question for clarity: Where does "test." come from? How does this shortcut to the gTestsArray[idx] happen here?

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 350318 [details] [diff] [review]
patch

right, Marco, it's juggling with facts

Comment 11

[alexander :surkov (:asurkov)]

Actually there is a question. When I select text in textbox then there is no caret but there is focus (similar behaviour has safari but ie). Should we report caret offset = -1 in this case?

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 13

[Marco Zehe (:MarcoZ)]

Comment on attachment 350425 [details] [diff] [review]
patch2

This test looks correct. Thanks!

Comment 14

[Aaron Leventhal]

(In reply to comment #11)
> Actually there is a question. When I select text in textbox then there is no
> caret but there is focus (similar behaviour has safari but ie). Should we
> report caret offset = -1 in this case?

No I don't think so. The caret is there, just not visible. Otherwise how will AT know which end of the selection is the side the user is actively controlling?

Comment 15

[Aaron Leventhal]

Comment on attachment 350425 [details] [diff] [review]
patch2

This patch would break rich text editing case, I believe. No ancestor of the caret should return -1. You need to be able to follow the descendants down to the one where the caret is not on an embedded object character.

if (mDOMNode != gLastFocusedNode &&
    !nsCoreUtils::IsAncestorOf(mDOMNode, gLastFocusedNode))

I believe one more condition is needed to check for the ancestor relation in the opposite order:

+ && !nsCoreUtils::IsAncestorOf(mDOMNode, gLastFocusedNode))

In the contenteditable case, gLastFocusedNode will be on the container with contenteditable="true".

<div contenteditable="true">  // Hypertext, text="\xfffc"  (embedded obj char)
   <p>                        // Hypertext, text="abcdef"
     abc|def                  // | = caret
     
The div should have caretOffset of 0
The p should have caret offset of 3

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 17

[Marco Zehe (:MarcoZ)]

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

>+  // Turn the focus node and offset of the selection into caret hypretext
>+  // offset.

Nit: "Hypertext", the r and e are swapped in this comment.

Comment 18

[Aaron Leventhal]

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

r+ if Marcoz tests and says it's ok

Comment 19

[Marco Zehe (:MarcoZ)]

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

When running the tests, I get these 4 error messages:
100 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_events_caretmove.html | test 10 failed.
101 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_events_caretmove.html | test 11 failed.
102 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_events_caretmove.html | test 12 failed.
103 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_events_caretmove.html | test 13 failed.

Tests 0 through 9 are a pass.

Comment 20

[alexander :surkov (:asurkov)]

Marco, what is your platform? My tests were successful on windows xp.

Comment 21

[Marco Zehe (:MarcoZ)]

I ran the tests on Windows XP also, I was running the whole test suite, not the individual file.

Comment 22

[Aaron Leventhal]

We also may want to return S_FALSE for IA2 methods where the offset is -1. Mick, Pete does that make sense to you?

Comment 23

[Michael Curran]

(In reply to comment #22)
> We also may want to return S_FALSE for IA2 methods where the offset is -1.
> Mick, Pete does that make sense to you?
That sounds fine to me.

Comment 24

[Aaron Leventhal]

Cool. I also just got verification in the spec, from http://accessibility.freestandards.org/a11yspecs/ia2/api/AccessibleText.idl
> @retval S_FALSE if the caret is not currently active on this object, i.e. the
>    caret is located on some other object.  The returned offset value will be -1.

Alex, did we miss that?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Cool. I also just got verification in the spec, from
> http://accessibility.freestandards.org/a11yspecs/ia2/api/AccessibleText.idl
> > @retval S_FALSE if the caret is not currently active on this object, i.e. the
> >    caret is located on some other object.  The returned offset value will be -1.
> 
> Alex, did we miss that?

Yes. I'll fix this with failed mochitests.

Comment 26

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 27

[Marco Zehe (:MarcoZ)]

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

This patch also contains fixes from bug 441974, correct? I see changes in the Comboboxes file which are not related to this bug.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> (From update of attachment 352685 [details] [diff] [review])
> This patch also contains fixes from bug 441974, correct? I see changes in the
> Comboboxes file which are not related to this bug.

Roughly speaking yes. But these changes must be here because I change eventQueue object to avoid my local failures of caret move tests.

Comment 29

[Pete Brunet]

Aaron, Please provide some text that I can use in the IA2 spec for IAText::caretOffset at http://accessibility.freestandards.org/a11yspecs/ia2/docs/html/interface_i_accessible_text.html#b02c838409659f949a3ca02620ffd77a

Comment 30

[Marco Zehe (:MarcoZ)]

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

r=me. Thanks!

Comment 31

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/c6b0a91832ad