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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Michael Curran, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

23.08 KB, patch
MarcoZ
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Keywords: access

Comment 1

9 years ago
-1 offset means end of text usually.

Should it throw an exception instead?
(Reporter)

Comment 2

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
As long as it's in the spec for caretOffset I don't mind. Should getting the selection do the same thing?
(Reporter)

Comment 6

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

9 years ago
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!
Blocks: 368895
(Assignee)

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 8

9 years ago
Created attachment 350318 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #350318 - Flags: superreview?(roc)
Attachment #350318 - Flags: review?(aaronleventhal)
(Assignee)

Updated

9 years ago
Attachment #350318 - Flags: review?(marco.zehe)

Updated

9 years ago
Attachment #350318 - Flags: review?(marco.zehe) → review+

Comment 9

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

Comment 10

9 years ago
Comment on attachment 350318 [details] [diff] [review]
patch

right, Marco, it's juggling with facts
Attachment #350318 - Attachment is obsolete: true
Attachment #350318 - Flags: superreview?(roc)
Attachment #350318 - Flags: review?(aaronleventhal)
(Assignee)

Comment 11

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

Comment 12

9 years ago
Created attachment 350425 [details] [diff] [review]
patch2
Attachment #350425 - Flags: superreview?(roc)
Attachment #350425 - Flags: review?(aaronleventhal)
(Assignee)

Updated

9 years ago
Attachment #350425 - Flags: review?(marco.zehe)
Comment on attachment 350425 [details] [diff] [review]
patch2

This test looks correct. Thanks!
Attachment #350425 - Flags: review?(marco.zehe) → review+
Attachment #350425 - Flags: superreview?(roc) → superreview+

Comment 14

9 years ago
(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?

Updated

9 years ago
Attachment #350425 - Flags: review?(aaronleventhal) → review-

Comment 15

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

Comment 16

9 years ago
Created attachment 351508 [details] [diff] [review]
patch3
Attachment #350425 - Attachment is obsolete: true
Attachment #351508 - Flags: review?(aaronleventhal)
(Assignee)

Updated

9 years ago
Attachment #351508 - Flags: review?(marco.zehe)
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.

Updated

9 years ago
Attachment #351508 - Flags: review?(aaronleventhal) → review+

Comment 18

9 years ago
Comment on attachment 351508 [details] [diff] [review]
patch3

r+ if Marcoz tests and says it's ok
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.
Attachment #351508 - Flags: review?(marco.zehe) → review-
(Assignee)

Comment 20

9 years ago
Marco, what is your platform? My tests were successful on windows xp.
I ran the tests on Windows XP also, I was running the whole test suite, not the individual file.

Comment 22

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

Comment 23

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

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

Comment 25

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

Comment 26

9 years ago
Created attachment 352685 [details] [diff] [review]
patch4
Attachment #351508 - Attachment is obsolete: true
Attachment #352685 - Flags: review?(marco.zehe)
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.
(Assignee)

Comment 28

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

9 years ago
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 on attachment 352685 [details] [diff] [review]
patch4

r=me. Thanks!
Attachment #352685 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 31

9 years ago
http://hg.mozilla.org/mozilla-central/rev/c6b0a91832ad
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.