452769 - mochitest for nsIAccessibleText

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch1

Marco, could you run line by line carefully and check what's offsets and text are incorrect? This is first part of patches.

Comment 1

[Marco Zehe (:MarcoZ)]

Comment on attachment 336038 [details] [diff] [review]
patch1

>+        getTextAfterOffset(ID, acc, 0, BOUNDARY_WORD_START,
>+                           "hello ", 0, 6);
>+        getTextAfterOffset(ID, acc, 1, BOUNDARY_WORD_START,
>+                           "ello ", 1, 6);

I do have a problem understanding the purpose of these calls. Calling getTextAfterOffset with a BOUNDARY_xxx_START parameter sounds to me like two mutually exclusive parameters. E. G. you cannot expect to call GetTextAfterOffset where the offset is in the middle of a word, and then also expect the BOUNDARY_WORD-START to be honored. The same is the other way around with GetTextBeforeOffset called with a BOUNDARY_xxx_END parameter.

Alexander, do you know where there actually is a distinction between BOUNDARY_WORD_START and BOUNDARY_WORD_END? I looked in the IAccessible2 documentation, and ther they have BOUNDARY_WORD, but that's it. Do you have any source of info on this other than the crap we currently call documentation in our nsIAccessibleText.idl file?

Comment 2

[alexander :surkov (:asurkov)]

Marco, it goes from ATK (http://library.gnome.org/devel/atk/unstable/AtkText.html)

Comment 3

[alexander :surkov (:asurkov)]

atk_text_get_text_after_offset

If the boundary_type is ATK_TEXT_BOUNDARY_WORD_START the returned string is from the word start after the offset to the next word start.

The returned string will contain the word after the offset if the offset is inside a word or if the offset is not inside a word.

If the boundary_type is ATK_TEXT_BOUNDARY_WORD_END the returned string is from the word end at or after the offset to the next work end.

The returned string will contain the word after the offset if the offset is inside a word and will contain the word after the word after the offset if the offset is not inside a word.

Comment 4

[alexander :surkov (:asurkov)]

atk_text_get_text_before_offset()

If the boundary_type is ATK_TEXT_BOUNDARY_WORD_START the returned string is from the word start before the word start before the offset to the word start before the offset.

The returned string will contain the word before the offset if the offset is inside a word and will contain the word before the word before the offset if the offset is not inside a word.

If the boundary_type is ATK_TEXT_BOUNDARY_WORD_END the returned string is from the word end before the word end at or before the offset to the word end at or before the offset.

The returned string will contain the word before the offset if the offset is inside a word or if the offset is not inside a word.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 336038 [details] [diff] [review]
patch1

First, please extend the test string to 3 words. Then:

>+        // getTextAfterOffset
>+        getTextAfterOffset(ID, acc, 0, BOUNDARY_CHAR,
>+                           "h", 0, 1);
>+        getTextAfterOffset(ID, acc, 11, BOUNDARY_CHAR,
>+                           "d", 11, 12);
>+
>+        getTextAfterOffset(ID, acc, 0, BOUNDARY_WORD_START,
>+                           "hello ", 0, 6);

This must be "friend" with its according offsets. The word *after* the offset. Since offset 0 points to the "h" in "hello", we must return the next word.

>+        getTextAfterOffset(ID, acc, 1, BOUNDARY_WORD_START,
>+                           "ello ", 1, 6);

Same, must also be "friend" with that word's offset.

>+        getTextAfterOffset(ID, acc, 5, BOUNDARY_WORD_START,
>+                           " ", 5, 6);

Must be the third word (word after the word after the offset).

>+        getTextAfterOffset(ID, acc, 6, BOUNDARY_WORD_START,
>+                           "friend", 6, 12);

Must be the third word and its boundaries.

>+        getTextAfterOffset(ID, acc, 11, BOUNDARY_WORD_START,
>+                           "d", 11, 12);

Same here.

>+        getTextAfterOffset(ID, acc, 12, BOUNDARY_WORD_START,
>+                           "", 0, 0); // ??

Correct. Must be empty, because after the third word, there is no fourth word.

>+
>+        getTextAfterOffset(ID, acc, 0, BOUNDARY_WORD_END,
>+                           "hello", 0, 5);

Must be " friend" if I read "from ehe end of the word after the offset to the end of the word after the word after the offset" correctly.

>+        getTextAfterOffset(ID, acc, 1, BOUNDARY_WORD_END,
>+                           "ello", 1, 5);

Same here.

>+        getTextAfterOffset(ID, acc, 5, BOUNDARY_WORD_END,
>+                           " friend", 5, 12);

Must be from the end of "friend" to the end of the third word.

>+        getTextAfterOffset(ID, acc, 6, BOUNDARY_WORD_END,
>+                           "friend", 6, 12);

Must be from the end of "friend" to the end of the next word.

>+        getTextAfterOffset(ID, acc, 11, BOUNDARY_WORD_END,
>+                           "d", 11, 12);

Same here.

>+        getTextAfterOffset(ID, acc, 12, BOUNDARY_WORD_END,
>+                           "", 0, 0); // ??

Correct (see above).

>+
>+        getTextAfterOffset(ID, acc, 0, BOUNDARY_LINE_START,
>+                           "hello friend", 0, 12);
>+        getTextAfterOffset(ID, acc, 1, BOUNDARY_LINE_START,
>+                           "ello friend", 1, 12);
>+        getTextAfterOffset(ID, acc, 11, BOUNDARY_LINE_START,
>+                           "d", 11, 12);
>+        //getTextAfterOffset(ID, acc, 12, BOUNDARY_LINE_START,
>+        //                   "", 0, 0); // fails
>+
>+        getTextAfterOffset(ID, acc, 0, BOUNDARY_LINE_END,
>+                           "hello friend", 0, 12);
>+        getTextAfterOffset(ID, acc, 1, BOUNDARY_LINE_END,
>+                           "ello friend", 1, 12);
>+        getTextAfterOffset(ID, acc, 11, BOUNDARY_LINE_END,
>+                           "", 0, 0);
>+        getTextAfterOffset(ID, acc, 12, BOUNDARY_LINE_END,
>+                           "", 0, 0); // fails

These all can only work on multiline text. They should all return stuff from the "second" line if "hello friend" is on the first line (the line start after the offset).

Cancelling review request. I think we should fix these in this patch and then have a correct Mochitest according to the above and the docs you quoted.

Comment 6

[Marco Zehe (:MarcoZ)]

A request from the ATK/AT-SPI experts: Could you go through the test and my comments on them to see if I interpreted the documentation about ATKText::GetTextAfterOffset with all the boundary types etc. correctly? It seems we've messed up prety badly in this regard and want to fix that. :-)

Comment 7

[Aaron Leventhal]

here is some additional HTML that we need to make sure line breaks continue to work correctly for:
http://www.mozilla.org/access/qa/jstest-start.html
They are all cases that I found to be fragile -- including things like <pre> and <br>.

Comment 8

[Evan Yan]

(In reply to comment #5)
> >+        getTextAfterOffset(ID, acc, 5, BOUNDARY_WORD_START,
> >+                           " ", 5, 6);
> 
> Must be the third word (word after the word after the offset).
> 
I think this should be "friend".

> >+        getTextAfterOffset(ID, acc, 12, BOUNDARY_WORD_START,
> >+                           "", 0, 0); // ??
> 
> Correct. Must be empty, because after the third word, there is no fourth word.
> 
I'm not sure what we will get from atk_text_get_text_after_offset() in this case. It's not specified in the doc, maybe implementation dependent.

> >+        getTextAfterOffset(ID, acc, 5, BOUNDARY_WORD_END,
> >+                           " friend", 5, 12);
> 
> Must be from the end of "friend" to the end of the third word.
> 
I think it should be " friend", because for BOUNDARY_WORD_END, the returned string is from the word end _at_ or after the offset to the next work end.

> >+        getTextAfterOffset(ID, acc, 12, BOUNDARY_WORD_END,
> >+                           "", 0, 0); // ??
> 
> Correct (see above).
> 
Same as above.

I agree with the others.

Another thing is, does IAccessible2 support all the same boundary types?

Comment 9

[Marco Zehe (:MarcoZ)]

IAccessible2 has a much simpler interface. It has boundary types for characters, words, lines, but no distinction between a word_start or word_end boundary.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 11

[alexander :surkov (:asurkov)]

Marco and Evan, please go through the patch step by step, carefully and very much thank you ;)

Comment 12

[Marco Zehe (:MarcoZ)]

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

Yes, this looks correct. 
Once this is in, we should test for multiline text in a textarea, too, to see if we get textual results for line start/line end and TextBeforeOffset and TextAfterOffset. Also, we should test what text we get back for when the caret is at the boundary between the two lines (magic value -2 for getTextAtOffset). But each of these two points can be done in a separate bug that enhance this text file.

Comment 13

[alexander :surkov (:asurkov)]

I don't want to rape my reviewers actually. So I will continue put not huge mochitests and when I finish then will start to fix them.

We need to test magic values, but that's a bit different I think. Once we get correct result for possible offsets then we can start to deal with caret problems if any.

thank you for review!

Comment 14

[Evan Yan]

When I went through the testcase, I'm totally confused by the API specifications of atk_text_get_text_after/at/before_offset(). I think there are some inconsistencies in the specification.

I filed a bug in gnome community.
http://bugzilla.gnome.org/show_bug.cgi?id=551489

Comment 15

[Fernando Herrera]

Attachment: patch3

Comment 16

[Fernando Herrera]

I have updated the tests following the docs (after the last comments) and the results from some gtk widgets

Comment 17

[alexander :surkov (:asurkov)]

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

style nits (no logic review), perhaps that's funny I'm asking to fix stuffs I introduced a while ago, but


>  		nsIAccessible_selects.js \
>+		nsIAccessibleText.js \

text.js

>+const BOUNDARY_WORD_START = nsIAccessibleText.BOUNDARY_WORD_START;
>+const BOUNDARY_WORD_END = nsIAccessibleText.BOUNDARY_WORD_END;
>+const BOUNDARY_LINE_START = nsIAccessibleText.BOUNDARY_LINE_START;
>+const BOUNDARY_LINE_END = nsIAccessibleText.BOUNDARY_LINE_END;
>+const BOUNDARY_ATTRIBUTE_RANGE = nsIAccessibleText.BOUNDARY_ATTRIBUTE_RANGE;
>+
>+function getText(aID, aAcc, aStartOffset, aEndOffset, aText)

testText?

pass aID, don't pass aAcc

use getAccessible(aID) to get an accessible

>+{
>+  try {
>+    is(aAcc.getText(aStartOffset, aEndOffset), aText,
>+       "getText: wrong text with start offset '" + aStartOffset +
>+       "' and end offset '" + aEndOffset + " for ID '" + aID + "'");

perhaps with -> between?

don't use aID in message, use prettyName(aID)

>+
>+function getTextAtOffset(aID, aAcc, aOffset, aBoundaryType,

testTextAtOffset and below

>+////////////////////////////////////////////////////////////////////////////////
>+// Private
>+
>+function getTextByOffset(aID, aAcc, aOffset, aBoundaryType,

perhaps testTextHelper ? I'm not sure by is suitable here

>+                         aText, aStartOffset, aEndOffset,
>+                         aToDoFlag1, aToDoFlag2, aToDoFlag3, aExceptionFlag,
>+                         aOffsetType)
>+{
>+  try {
>+    var getTextFunc = null;
>+    switch (aOffsetType) {
>+      case "at":
>+        getTextFunc = aAcc.getTextAtOffset;
>+        break;
>+      case "after":
>+        getTextFunc = aAcc.getTextAfterOffset;
>+        break;
>+      case "before":
>+        getTextFunc = aAcc.getTextBeforeOffset;
>+        break;
>+    }

aOffsetType might be replaced on aAcc.method

>+    startMsg += "(" + getBoundary(aBoundaryType) + "): ";

we could save boundary conversion to string as separate function if you like but boundaryToString() is more readable I think.

>diff -r 10439d6e1db2 accessible/tests/mochitest/text/Makefile.in
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/accessible/tests/mochitest/text/Makefile.in	Wed Nov 03 16:59:58 2010 +0100

>+# Contributor(s):
>+#   Alexander Surkov <[e-mail]> (original author)

not me, you, it's not my file

>+_TEST_FILES = \
>+		test_nsIAccessibleText_getText_1.html \

test_input.html ?

>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=452769
>+-->

remove it, we don't use it anymore

>+<head>
>+  <title>nsIAccessibleText test for html:input</title>

please more specific title, nsIAccessibleText interface is wider than we test here

>+  <script type="application/javascript" src="../common.js"></script>
>+  <script type="application/javascript" src="../nsIAccessibleText.js"></script>

src on new line, we've used to do this

>+  <script type="application/javascript">
>+    
>+    function doTest()
>+    {
>+      var ID = "input";
>+      var acc = getAccessible(ID, nsIAccessibleText);
>+
>+      if (acc) {

remove this if (acc), it must be accessible

>+
>+    SimpleTest.waitForExplicitFinish();
>+    addLoadEvent(doTest);

addA11yLoadEvent or

Comment 18

[alexander :surkov (:asurkov)]

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

could you upload new patch with nits fixed and rerequest review

Comment 19

[Fernando Herrera]

(In reply to comment #17)
> >+{
> >+  try {
> >+    is(aAcc.getText(aStartOffset, aEndOffset), aText,
> >+       "getText: wrong text with start offset '" + aStartOffset +
> >+       "' and end offset '" + aEndOffset + " for ID '" + aID + "'");
> 
> perhaps with -> between?

where?

> 
> >+                         aText, aStartOffset, aEndOffset,
> >+                         aToDoFlag1, aToDoFlag2, aToDoFlag3, aExceptionFlag,
> >+                         aOffsetType)
> >+{
> >+  try {
> >+    var getTextFunc = null;
> >+    switch (aOffsetType) {
> >+      case "at":
> >+        getTextFunc = aAcc.getTextAtOffset;
> >+        break;
> >+      case "after":
> >+        getTextFunc = aAcc.getTextAfterOffset;
> >+        break;
> >+      case "before":
> >+        getTextFunc = aAcc.getTextBeforeOffset;
> >+        break;
> >+    }
> 
> aOffsetType might be replaced on aAcc.method

what do you mean?



> >+_TEST_FILES = \
> >+		test_nsIAccessibleText_getText_1.html \
> 
> test_input.html ?

Humm, not sure, we may want to repeat the tests over different kind of text elements.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)

> > >+       "getText: wrong text with start offset '" + aStartOffset +
> > >+       "' and end offset '" + aEndOffset + " for ID '" + aID + "'");
> > 
> > perhaps with -> between?
> 
> where?

wrong text with start offset and end offset -> wrong text between start and end offsets
 
> what do you mean?

getTextAfterOffset()
{
  getTextHelper(args, args, aAcc.getTextAfterOffset);
}

> > test_input.html ?
> 
> Humm, not sure, we may want to repeat the tests over different kind of text
> elements.

sure but it makes sense to keep them in separate files, I'm not fan of 10k size files, especially for tests.

Comment 21

[Fernando Herrera]

Attachment: patch4

Updated patch.

Comment 22

[alexander :surkov (:asurkov)]

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


>+function testText(aID, aStartOffset, aEndOffset, aText)
>+{
>+  aAcc = getAccessible(aID);

var acc = getAccessible(aID);

>+  try {
>+    is(aAcc.getText(aStartOffset, aEndOffset), aText,
>+       "getText: wrong text between start and end offsets '" + aStartOffset +
>+       "', '" + aEndOffset + " for ID '" + prettyName(aID) + "'");

" for ID" -> "for ", prettyName(aID) is not ID actually

>+function testTextHelper(aID, aOffset, aBoundaryType,
>+                        aText, aStartOffset, aEndOffset,
>+                        aToDoFlag1, aToDoFlag2, aToDoFlag3, aExceptionFlag,
>+                        getTextFunc, startMsg)

use 'a' prefix for arguments

getTextFunc -> aTextFunc?

startMsg -> aTextFuncName?

>diff -r 10439d6e1db2 accessible/tests/mochitest/text/test_input.html
>+    function doTest()
>+    {
>+      var ID = "input";
>+      var acc = getAccessible(ID, nsIAccessibleText);

why do you need acc for?

Comment 23

[Fernando Herrera]

Attachment: patch5

Updated patch

Comment 24

[alexander :surkov (:asurkov)]

Fernando, could you extend test by extending a test string by offsets that outside the word?

Comment 25

[alexander :surkov (:asurkov)]

add test for testTextAfterOffset(ID, 8, BOUNDARY_WORD_START) - make sure you test every boundary offset.

Comment 26

[alexander :surkov (:asurkov)]

as I said in letter we should have tests for editable area, rich text. Plus we need tests for multiline textarea. You may deal with it in followups.

Comment 27

[alexander :surkov (:asurkov)]

testTextBeforeOffset(ID, 5, BOUNDARY_WORD_END,
                     "hello", 0, 5, true, false, true);

should return empty string because there's no word before the offset, since 5 is end offset of 'hello' word.

testTextBeforeOffset(ID, 6, BOUNDARY_WORD_END,
                     "hello", 0, 5, true, true, true);

this makes me worry since word start of 'hello' word we treat as word end of not existing word before 'hello' word. But if it's ok then we should return "hello " because 5 ' ' is the word end of 'hello' word and we should return the text to the word end before offset.

the same is applicable for other testTextBeforeOffset/BOUNDARY_WORD_END tests.

Comment 28

[alexander :surkov (:asurkov)]

testTextAtOffset(ID, 0, BOUNDARY_LINE_END,
                 "", 15, 15, true, true, false);

should these return "hello my friend" as well?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Fernando, could you extend test by extending a test string by offsets that
> outside the word?

as separate test, may be followup.

Comment 30

[alexander :surkov (:asurkov)]

Comment on attachment 487974 [details] [diff] [review]
patch5

canceling review until comments are addressed, please feel rerequest the review if nothing to change in the patch.

Comment 31

[Fernando Herrera]

Attachment: patch6

I have updated the patch. Now it is testing the same line over an input text, a normal div, an editable div and a textarea. Notice that instead of iterating over an array with all the elements it execs the test explicitly for every element. I'm doing it because sometimes textarea fails for something that an input doesn't.

I will start writing tests for mutiline and extra whitespaces in separate bugs.

Comment 32

[alexander :surkov (:asurkov)]

  for (var i=5; i<arguments.length; i=i+5)

that's unfriendly, unclear and makes methods work with predefined number of elements, it may be hard to port it to other tests. Btw, use spaces between operands and operators.

perhaps repeating the method for each element is better we can do, though much nosier, if you can think of better approach that method copy/paste, but the suggested one isn't satisfactory.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)

> but the
> suggested one isn't satisfactory.

perhaps, that's document issue - I see it can be ported well

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> (In reply to comment #32)
> 
> > but the
> > suggested one isn't satisfactory.
> 
> perhaps, that's document issue - I see it can be ported well

Ok, I like it, sorry for the noise, get tired today. Please add comments before each method, one of them should describe arguments magic you do - what attributes are expected and what are they for.

Comment 35

[alexander :surkov (:asurkov)]

"input", true, true, false, false,
"input", undefined, undefined, undefined, true,

you might want to use these without fourth argument, for example if all (one) of them are undefined then exception is assumed

Comment 36

[alexander :surkov (:asurkov)]

                          "textarea", false, true, true, false);
////////////////////////////////////////////////////////////////////////
// getTextBeforeOffset

please add empty line between test end and comment.

Comment 37

[alexander :surkov (:asurkov)]

      testTextAfterOffset(1, BOUNDARY_WORD_START, "my  ", 6, 9,
                          "input", true, true, true, false,

it looks like you have two spaces that makes you do todo_is

btw, true always confuses me because true makes me think everything is alright. Isn't it better to use constants like kTodo and kOk?

Comment 38

[alexander :surkov (:asurkov)]

I might miss something but why textAfterOffset and textBeforeOffset with boundary word end are so different? textBeforeOffset allows " my " but textAfterOffset allows "my ". Word end boundary means from the word end to the word end, here 5 and 8 are word ends

// __h__e__l__l__o__ __m__y__ __f__r__i__e__n__d__
//  0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15

so testTextBeforeOffset(10, BOUNDARY_WORD_END, " my ", 5, 9) should return (5, 8) and " my" text, right?

Comment 39

[Fernando Herrera]

Attachment: patch7

Updated addressing last comments

Comment 40

[alexander :surkov (:asurkov)]

Comment on attachment 488893 [details] [diff] [review]
patch7


>+
>+const kTodo = true;
>+const kOk = false;

>+/**
>+ * Test getTextAtOffset function over different elements
>+ *
>+ * @param aOffset         [in] the offset to get the text at
>+ * @param aBoundaryType   [in] Boundary type for text to be retrieved
>+ * @param aText           [in] expected return text for getTextAtOffset
>+ * @param aStartOffset    [in] expected return start offset for getTextAtOffset
>+ * @param aEndOffset      [in] expected return end offset for getTextAtOffset
>+ * @param ...             [in] list of tuples made of:
>+ *                              elementid, test status for text, test status for start, text status for end

element identifier, perhaps because you may pass here DOM node or accessible or ID attribute value. Btw, it makes sense to use prettyName() in testTextHelper

"test status" doesn't sound very clear, would be nice to refer to kTodo and kOk constants defined above.

>+ *          
>+ */
>+function testTextAtOffset(aOffset, aBoundaryType, aText,
>+                          aStartOffset, aEndOffset)
>+{
>+  for (var i=5; i<arguments.length; i=i+4)

use spaces between operand and operators like var i = 5

>+  {
>+    var ID = arguments[i];
>+    var acc = getAccessible(ID, nsIAccessibleText);
>+    var toDoFlag1 = arguments[i+1];
>+    var toDoFlag2 = arguments[i+2];
>+    var toDoFlag3 = arguments[i+3];

the same

>+
>+function testTextHelper(aID, aOffset, aBoundaryType,
>+                        aText, aStartOffset, aEndOffset,
>+                        aToDoFlag1, aToDoFlag2, aToDoFlag3,
>+                        aTextFunc, aTextFuncName)
>+{
>+  var exceptionFlag = aToDoFlag1 == undefined ||
>+                      aToDoFlag2 == undefined ||
>+                      aToDoFlag3 == undefined;

>+  } catch (e) {
>+    var okFunc = exceptionFlag ? todo : ok;

I wonder whether ok is expected at all, i.e. when do we expect this will fail?

>+      testTextAtOffset(14, BOUNDARY_LINE_START, "hello my friend", 0, 15,
>+                       "input", kOk, kOk, kOk,
>+                       "div", kOk, kOk, kOk,
>+                       "editable", kOk, kOk, kOk,
>+			       "textarea", kTodo, kOk, kTodo);
>+	      testTextAtOffset(15, BOUNDARY_LINE_START, "hello my friend", 0, 15,
>+			       "input", kOk, kOk, kOk,
>+			       "div", kOk, kOk, kOk,
>+			       "editable", kOk, kOk, kOk,
>+			       "textarea", kTodo, kOk, kTodo);
>+

use correct indentation

Comment 41

[alexander :surkov (:asurkov)]

> testTextAfterOffset(5, BOUNDARY_WORD_END, " friend", 8, 15,

should return " my" because 5 is end offset of "hello" word, text_after_offset returns the text from the word end _at_ of after offset (or contains the word after offset).

the same for testTextAfterOffset(8, BOUNDARY_WORD_END, "", 8, 15,

> testTextAtOffset(5, BOUNDARY_WORD_END, " my", 5, 8,

should return "hello" because "The returned string will contain the word at the offset if the offset is inside a word"

the same for testTextAtOffset(8, BOUNDARY_WORD_END, " friend", 8, 15,

please add test for testTextAtOffset(9, BOUNDARY_WORD_END, " friend", 8, 15,

make sure you have tests for 0, 5, 6, 8, 9, 14 and 15 offsets for every test group (boundary offsets) and one tests for offsets in the middle of each word.

I see through the patch wrong indentations here and there, please fix them.

Comment 42

[alexander :surkov (:asurkov)]

Comment on attachment 488893 [details] [diff] [review]
patch7

please file new patch with comments addressed

Comment 43

[Fernando Herrera]

(In reply to comment #40)

  
> 
> >+  } catch (e) {
> >+    var okFunc = exceptionFlag ? todo : ok;
> 
> I wonder whether ok is expected at all, i.e. when do we expect this will fail?

When an unexpected exception happens.

Comment 44

[Fernando Herrera]

(In reply to comment #41)

> make sure you have tests for 0, 5, 6, 8, 9, 14 and 15 offsets for every test
> group (boundary offsets) and one tests for offsets in the middle of each word.

Also for CHAR, LINE_START and LINE_END?

Comment 45

[alexander :surkov (:asurkov)]

(In reply to comment #43)

> > I wonder whether ok is expected at all, i.e. when do we expect this will fail?
> 
> When an unexpected exception happens.

right, but the question is any exception is expected? Are these methods going to fail ever or do we tests when they should expectedly fail?

(In reply to comment #44)
> (In reply to comment #41)
> 
> > make sure you have tests for 0, 5, 6, 8, 9, 14 and 15 offsets for every test
> > group (boundary offsets) and one tests for offsets in the middle of each word.
> 
> Also for CHAR, LINE_START and LINE_END?

nope, skip'em

Comment 46

[Fernando Herrera]

(In reply to comment #45)
> (In reply to comment #43)
> 
> > > I wonder whether ok is expected at all, i.e. when do we expect this will fail?
> > 
> > When an unexpected exception happens.
> 
> right, but the question is any exception is expected? Are these methods going
> to fail ever or do we tests when they should expectedly fail?

Not at least with the atk implementation, but other implementations could raise exceptions in the case of undefined behaviours, right?

Comment 47

[Fernando Herrera]

Attachment: patch8

Updated patch

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #46)

> > right, but the question is any exception is expected? Are these methods going
> > to fail ever or do we tests when they should expectedly fail?
> 
> Not at least with the atk implementation, but other implementations could raise
> exceptions in the case of undefined behaviours, right?

We don't deal with atk implementation here, we deal with gecko crossplatform api. If crossplatform API is going to throw an exception and we want to test this then it's ok. I just wanted you to confirm.

Comment 49

[alexander :surkov (:asurkov)]

Comment on attachment 489042 [details] [diff] [review]
patch8


>+
>+const kTodo = true;
>+const kOk = false;

for constants I prefer numbers, otherwise it makes me wonder why we need new constants for booleans.

>+  for (var i = 0; i < aIDs.length; i++)
>+  {

nit: put brace on the same line with for statement:
for (var i = 0; i < aIDs.length; i++) {

>+ * @param ...             [in] list of tuples made of:
>+ *                              - element identifier
>+ *                              - kTodo or kOk for text result
>+ *                              - kTodo or kOk for start offset result
>+ *                              , kTodo or kOk for end offset result

nit: , -> -, or just remove these '-' at all, make two spaces indent

text result and other xxx results don't sound clear, perhaps returned text and  etc?





>+    var acc = getAccessible(ID, nsIAccessibleText);
>+    var toDoFlag1 = arguments[i+1];

empty space between operand and operators please: i + 1

>+    var toDoFlag2 = arguments[i+2];
>+    var toDoFlag3 = arguments[i+3];
>+
>+    testTextHelper(ID, aOffset, aBoundaryType,
>+                 aText, aStartOffset, aEndOffset,
>+                 toDoFlag1, toDoFlag2, toDoFlag3,
>+                 acc.getTextAtOffset, "getTextAtOffset ");

right indentation please

>+  }
>+}
>+
>+/**
>+ * Test getTextAfterOffset function over different elements
>+ *
>+ * @param aOffset         [in] the offset to get the text after
>+ * @param aBoundaryType   [in] Boundary type for text to be retrieved
>+ * @param aText           [in] expected return text for getTextAfterOffset
>+ * @param aStartOffset    [in] expected return start offset for getTextAfterOffset
>+ * @param aEndOffset      [in] expected return end offset for getTextAfterOffset
>+ * @param ...             [in] list of tuples made of:
>+ *                              elementid, test status for text, test status for start, text status for end

fix this comment corresponding to above one.


>+    var isFunc1 = aToDoFlag1 ? todo_is : is;
>+    var isFunc2 = aToDoFlag2 ? todo_is : is;
>+    var isFunc3 = aToDoFlag3 ? todo_is : is;

if you would use numbering constants then you should fix these.

>+      testTextAtOffset(15, BOUNDARY_WORD_END, "", 15, 15,
>+                        "input", kOk, kTodo, kTodo,
>+                        "div", kOk, kTodo, kTodo,
>+                        "editable", kOk, kTodo, kTodo,
>+                        "textarea", kTodo, kOk, kTodo);

testTextAtOffset(15, BOUNDARY_WORD_END, "", 15, 15, should return " friend" because 15 is end offset of "friend" word.

>+    }
>+
>+
>+    SimpleTest.waitForExplicitFinish();

excess empty line

r=me with comments fixed, please get one more careful review, we should make sure I didn't miss anything.

Comment 50

[Fernando Herrera]

Attachment: patch9

Updated patch.
Who should I ask for superreview?

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #50)
> Created attachment 490048 [details] [diff] [review]
> patch9
> 
> Updated patch.
> Who should I ask for superreview?

sticky speaking you don't need sr, but you could get one more review. That could be David if he promise to be careful or somebody else who can do the same :)

Comment 52

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 490048 [details] [diff] [review]
patch9

r=me. I cross referenced this with the (confusing) AtkText documentation and it looks right to me. Thanks!

Someone needs to add examples to the AtkText documentation.

Comment 53

[Fernando Herrera]

I have checked our tests against gtk+ implementation in gail, and the only diffs are:

getTextAfterOffset(5, TEXT_BOUNDARY_WORD_END)
gail = " friend",8,15
us = " my", 5, 8

getTextAfterOffset(8, TEXT_BOUNDARY_WORD_END)
gail = "",15,15
us = " friend", 8, 15

getTextBeforeOffset(5, TEXT_BOUNDARY_WORD_END)
gail = "hello",0,5
us = "", 0, 0

getTextBeforeOffset(8, TEXT_BOUNDARY_WORD_END)
gail = " my",5,8
us = "hello ", 0, 6

getTextBeforeOffset(15, TEXT_BOUNDARY_WORD_END)
gail = " friend",8,15
us = " my", 5, 8

getTextAtOffset(5, TEXT_BOUNDARY_WORD_END)
gail = " my",5,8
us = "hello", 0, 5

getTextAtOffset(8, TEXT_BOUNDARY_WORD_END)
gail = " friend",8,15
us = " my", 5, 8

getTextAtOffset(15, TEXT_BOUNDARY_WORD_END)
gail = "",15,15
us = " friend", 8, 15

I'll double check those with joanie.

Comment 54

[alexander :surkov (:asurkov)]

But we're doing correct things, right?

Comment 55

[David Bolter [:davidb] (NeedInfo me for attention)]

Wait, which documentation are we using?

Comment 56

[Fernando Herrera]

We are using:

http://library.gnome.org/devel/atk/unstable/AtkText.html

plus the doc patch for atk_text_get_text_before_offset at:

https://bugzilla.gnome.org/show_bug.cgi?id=551489

I'll contact Li (gail maintainer) to agree in the correct results and fix either the tests or gail.

Comment 57

[David Bolter [:davidb] (NeedInfo me for attention)]

If Li is correct (on the GNOME bug) regarding start and end offsets always being 'inside the word' then I think we are correct.

Comment 58

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/0c22e26c4fd5