749810 - getText(0, -1) fails with empty text

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

On an accessible with zero-length text, doing:
textIface.getText(0, nsIAccessibleText.TEXT_OFFSET_END_OF_TEXT);
raises NS_ERROR_ILLEGAL_VALUE.

This defeats the purpose of TEXT_OFFSET_END_OF_TEXT, because it requires us to retrieve the characterCount beforehand, and doesn't let us blindly retrieve the text (or empty string in this case).

Comment 1

[Hubert Figuiere [:hub]]

I saw that with Mac a11y too, but I didn't bother to track it down.

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

This fixs the bug ( (and doesn't seem to break anything else :) )

Comment 3

[Mark Capella [:capella]]

Attachment: Test Case

Test I used ... could be added or worked into an existing test

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 625249 [details] [diff] [review]
Patch (v1)

Review of attachment 625249 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for giving this a go! I would definitely add these cases into our test suite in this patch as well.

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +465,1 @@
>  

There seems to be more than one reason why GetChildIndexAtOffset would return -1. Should all of them return a status of NS_OK? I'm not sure, It is hard to tell. You really just want to return NS_OK when it is truely an empty string, and the end offset is -1. If the end offset is 20, it should still raise an error.

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 625250 [details]
Test Case

>        is(divAcc2.getText(0, -1), "", "getText() END_OF_TEXT with null string");
>        is(divAcc2.getText(0, 0), "", "getText() Len==0 with null string");
>        is(divAcc2.getText(0, 1), "", "getText() Len==1 with null string");
>        is(divAcc2.getText(0, 2), "", "getText() Len==2 with null string");

I would think the last two tests should raise an error.

Comment 6

[Mark Capella [:capella]]

I'm not sure I agree ... if you're asking for text that doesn't exist for whatever reason and even at positions that don't exist, you get back "no text" gracefully. ie: text from a null string at position foo to foo+bar is null plain and simple.

If you want to be strict and return errors, then you wind up with the code as we began, and getText from start to finish where there is no "start" or "finish" errors out (vs. returning gracefully... ?)

Re: "Should all of them return a status of NS_OK? I'm not sure" ... I lean towards saying "yes".

Well, I don't have a dog in this fight, just trying to help patch a bug.

Comment 7

[Eitan Isaacson [:eeejay]]

-1 as an end boundary has a special meaning. Just like any other subscriptable object, I would expect an error if you request something beyond the bounds.

(In reply to Mark Capella [:capella] from comment #6)
> Well, I don't have a dog in this fight, just trying to help patch a bug.

Me neither :) just put it up for review and do whatever the reviewer wants.

Comment 8

[Mark Capella [:capella]]

Comment on attachment 625249 [details] [diff] [review]
Patch (v1)

Ok then, since this fixes the immediate bug request without over-conceptualizing the whole thing ... we could move this and follow-up if any future issues present themselves ...

Alex?

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 625249 [details] [diff] [review]
Patch (v1)

Review of attachment 625249 [details] [diff] [review]:
-----------------------------------------------------------------

canceling review until Eitan point is addressed

Comment 10

[alexander :surkov (:asurkov)]

(I missed your discussion before I cancelled review).

I think IA2 doesn't agree with you to get back the text gracefully. So you might argue it's ok for XPCOM but then you should have special processing on IA2 side. Actually I wouldn't change the logic in this bug, otherwise I think you get out of sync with this (end offset check) and other methods.

So you could fix this bug by checking offsets and children count instead.

Comment 11

[Mark Capella [:capella]]

Wow, not being familiar with "IA2", this has me confused. You mean to say that in the IA2 world, (with my change) asking for nullfoo.gettext(0, 20) and receiving back null string and zero-return-code obscurs information that is provided (currently?) by nullfoo.getttext(0, 20) that returns null/non-zero-return-code?

Comment 12

[alexander :surkov (:asurkov)]

IA2 says:

[in] 	startOffset 	Index of the first character to include in the returned string. The valid range is 0..length.
[in] 	endOffset 	Index of the last character to exclude in the returned string. The valid range is 0..length. 

Return values:
    	S_OK 	
    	E_INVALIDARG 	if bad [in] passed 

http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_accessible_text.html#84497731096e4563e28a191a46086725

Comment 13

[Mark Capella [:capella]]

That's for the IA2 reference link!

So it looks like gettext on a null string per IA2 spec is always considered "bad [in] passed", and therefore the code as-exists is correct ... ?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #13)
> That's for the IA2 reference link!
> 
> So it looks like gettext on a null string per IA2 spec is always considered
> "bad [in] passed", and therefore the code as-exists is correct ... ?

empty text range is (0, 0) and it seems we can treat 0 as valid offset.

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v2)

This is a more-targeted solution that (I believe) satisfies the bug request, and IA2 specs also (from comment 5 the last two tests do indeed fail now).

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 625710 [details] [diff] [review]
Patch (v2)

Review of attachment 625710 [details] [diff] [review]:
-----------------------------------------------------------------

would you mind to add mochitest please?

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +468,2 @@
>  
>    PRInt32 endOffset = ConvertMagicOffset(aEndOffset);

I think I would do something like:
PRInt32 startOffset = ConvertMagicOffset()
PRint32 endOffset = ConvertMagicOffset();

PRint32 startChildIdx = GetChildIndexAtOffset(startOffset);
if (startChildIdx == -1) {
  // 0 offsets are considered valid for empty text.
  return (startOffset == 0 && endOffset == 0) ? NS_OK : NE_ERROR_INVALID_ARG;
}

Comment 17

[Mark Capella [:capella]]

Attachment: Patch (v3)

Yah, that code's a little "tighter" :)

FYI, I added the tests to an existing set, rather than add a whole new file.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 626205 [details] [diff] [review]
Patch (v3)

Review of attachment 626205 [details] [diff] [review]:
-----------------------------------------------------------------

looks ok

::: accessible/tests/mochitest/text/test_hypertext.html
@@ +24,5 @@
>        //////////////////////////////////////////////////////////////////////////
> +      // null getText
> +      //////////////////////////////////////////////////////////////////////////
> +
> +      var nullAcc = getAccessible("nulltext", [nsIAccessibleText]);

maybe emptyTextAcc?

@@ +25,5 @@
> +      // null getText
> +      //////////////////////////////////////////////////////////////////////////
> +
> +      var nullAcc = getAccessible("nulltext", [nsIAccessibleText]);
> +      if (nullAcc) {

you don't really need this check but I don't mind if you like it

Comment 19

[Mark Capella [:capella]]

FYI Inbound Try push https://tbpl.mozilla.org/?tree=Try&rev=99f631fcb377

Comment 20

[Hubert Figuiere [:hub]]

(In reply to Mark Capella [:capella] from comment #19)
> FYI Inbound Try push https://tbpl.mozilla.org/?tree=Try&rev=99f631fcb377

You should try running the test when pushing to try so we can see if anything is broken.

(see the TryChooser for the syntax)

Comment 21

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=88c9a3d0e187

Comment 22

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ac38f99528e0

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2908fe12eb8f

Comment 24

[Ed Morley [:emorley]]

(Comment 22 URL is for another bug, use comment 23).