Last Comment Bug 749810 - getText(0, -1) fails with empty text
: getText(0, -1) fails with empty text
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: texta11y
  Show dependency treegraph
 
Reported: 2012-04-27 15:16 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-24 09:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (1.04 KB, patch)
2012-05-18 13:56 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Test Case (1.47 KB, text/html)
2012-05-18 13:58 PDT, Mark Capella [:capella]
no flags Details
Patch (v2) (1.22 KB, patch)
2012-05-21 11:49 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v3) (3.37 KB, patch)
2012-05-22 15:14 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-04-27 15:16:59 PDT
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] 2012-04-27 15:31:33 PDT
I saw that with Mac a11y too, but I didn't bother to track it down.
Comment 2 Mark Capella [:capella] 2012-05-18 13:56:55 PDT
Created attachment 625249 [details] [diff] [review]
Patch (v1)

This fixs the bug ( (and doesn't seem to break anything else :) )
Comment 3 Mark Capella [:capella] 2012-05-18 13:58:57 PDT
Created attachment 625250 [details]
Test Case

Test I used ... could be added or worked into an existing test
Comment 4 Eitan Isaacson [:eeejay] 2012-05-18 15:27:29 PDT
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] 2012-05-18 15:28:20 PDT
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] 2012-05-18 16:29:38 PDT
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] 2012-05-18 16:49:35 PDT
-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] 2012-05-19 08:37:28 PDT
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 2012-05-21 02:27:35 PDT
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 2012-05-21 02:34:42 PDT
(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] 2012-05-21 08:04:33 PDT
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 2012-05-21 08:22:23 PDT
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] 2012-05-21 08:37:17 PDT
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 2012-05-21 08:50:42 PDT
(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] 2012-05-21 11:49:10 PDT
Created attachment 625710 [details] [diff] [review]
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 2012-05-21 22:32:01 PDT
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] 2012-05-22 15:14:42 PDT
Created attachment 626205 [details] [diff] [review]
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 2012-05-23 04:46:06 PDT
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] 2012-05-23 12:01:59 PDT
FYI Inbound Try push https://tbpl.mozilla.org/?tree=Try&rev=99f631fcb377
Comment 20 Hubert Figuiere [:hub] 2012-05-23 15:10:40 PDT
(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] 2012-05-23 15:12:36 PDT
https://tbpl.mozilla.org/?tree=Try&rev=88c9a3d0e187
Comment 22 Mark Capella [:capella] 2012-05-23 21:17:26 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ac38f99528e0
Comment 23 Ed Morley [:emorley] 2012-05-24 09:21:56 PDT
https://hg.mozilla.org/mozilla-central/rev/2908fe12eb8f
Comment 24 Ed Morley [:emorley] 2012-05-24 09:22:40 PDT
(Comment 22 URL is for another bug, use comment 23).

Note You need to log in before you can comment on or make changes to this bug.