getText(0, -1) fails with empty text

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: capella)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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).
I saw that with Mac a11y too, but I didn't bother to track it down.

Updated

5 years ago
Blocks: 368895
(Assignee)

Comment 2

5 years ago
Created attachment 625249 [details] [diff] [review]
Patch (v1)

This fixs the bug ( (and doesn't seem to break anything else :) )
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #625249 - Flags: feedback?(eitan)
(Assignee)

Comment 3

5 years ago
Created attachment 625250 [details]
Test Case

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

Comment 4

5 years ago
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.
Attachment #625249 - Flags: feedback?(eitan)
(Reporter)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

5 years ago
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?
Attachment #625249 - Flags: review?(surkov.alexander)

Comment 9

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

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

canceling review until Eitan point is addressed
Attachment #625249 - Flags: review?(surkov.alexander)

Comment 10

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

Comment 11

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

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

Comment 13

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

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

Comment 15

5 years ago
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).
Attachment #625249 - Attachment is obsolete: true

Comment 16

5 years ago
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;
}
Attachment #625710 - Flags: review+
(Assignee)

Comment 17

5 years ago
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.
Attachment #625710 - Attachment is obsolete: true
Attachment #625250 - Attachment is obsolete: true

Comment 18

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

Comment 19

5 years ago
FYI Inbound Try push https://tbpl.mozilla.org/?tree=Try&rev=99f631fcb377
(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)
(Assignee)

Comment 21

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=88c9a3d0e187
(Assignee)

Comment 22

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ac38f99528e0
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/2908fe12eb8f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Comment 22 URL is for another bug, use comment 23).
You need to log in before you can comment on or make changes to this bug.