Closed
Bug 452769
Opened 16 years ago
Closed 14 years ago
mochitest for nsIAccessibleText
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: fherrera)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 8 obsolete files)
38.88 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
Marco, could you run line by line carefully and check what's offsets and text are incorrect? This is first part of patches.
Attachment #336038 -
Flags: review?(marco.zehe)
Comment 1•16 years ago
|
||
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?
Reporter | ||
Comment 2•16 years ago
|
||
Marco, it goes from ATK (http://library.gnome.org/devel/atk/unstable/AtkText.html)
Reporter | ||
Comment 3•16 years ago
|
||
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.
Reporter | ||
Comment 4•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #336038 -
Flags: review?(marco.zehe)
Comment 6•16 years ago
|
||
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•16 years ago
|
||
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>.
(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•16 years ago
|
||
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.
Reporter | ||
Comment 10•16 years ago
|
||
Attachment #336038 -
Attachment is obsolete: true
Attachment #337055 -
Flags: review?(marco.zehe)
Reporter | ||
Updated•16 years ago
|
Attachment #337055 -
Flags: review?(Evan.Yan)
Reporter | ||
Comment 11•16 years ago
|
||
Marco and Evan, please go through the patch step by step, carefully and very much thank you ;)
Comment 12•16 years ago
|
||
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.
Attachment #337055 -
Flags: review?(marco.zehe) → review+
Reporter | ||
Comment 13•16 years ago
|
||
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•16 years ago
|
||
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
Assignee | ||
Comment 15•14 years ago
|
||
Assignee | ||
Comment 16•14 years ago
|
||
I have updated the tests following the docs (after the last comments) and the results from some gtk widgets
Assignee | ||
Updated•14 years ago
|
Attachment #487919 -
Flags: review?(surkov.alexander)
Reporter | ||
Updated•14 years ago
|
Assignee: surkov.alexander → fherrera
Status: NEW → ASSIGNED
Reporter | ||
Comment 17•14 years ago
|
||
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 <surkov.alexander@gmail.com> (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
Reporter | ||
Comment 18•14 years ago
|
||
Comment on attachment 487919 [details] [diff] [review] patch3 could you upload new patch with nits fixed and rerequest review
Attachment #487919 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 19•14 years ago
|
||
(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.
Reporter | ||
Comment 20•14 years ago
|
||
(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.
Assignee | ||
Comment 21•14 years ago
|
||
Updated patch.
Attachment #487919 -
Attachment is obsolete: true
Attachment #487959 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 22•14 years ago
|
||
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?
Assignee | ||
Comment 23•14 years ago
|
||
Updated patch
Attachment #487959 -
Attachment is obsolete: true
Attachment #487974 -
Flags: review?(surkov.alexander)
Attachment #487959 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 24•14 years ago
|
||
Fernando, could you extend test by extending a test string by offsets that outside the word?
Reporter | ||
Comment 25•14 years ago
|
||
add test for testTextAfterOffset(ID, 8, BOUNDARY_WORD_START) - make sure you test every boundary offset.
Reporter | ||
Comment 26•14 years ago
|
||
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.
Reporter | ||
Comment 27•14 years ago
|
||
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.
Reporter | ||
Comment 28•14 years ago
|
||
testTextAtOffset(ID, 0, BOUNDARY_LINE_END, "", 15, 15, true, true, false); should these return "hello my friend" as well?
Reporter | ||
Comment 29•14 years ago
|
||
(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.
Reporter | ||
Updated•14 years ago
|
Attachment #337055 -
Attachment is obsolete: true
Attachment #337055 -
Flags: review?(evan.yan)
Reporter | ||
Comment 30•14 years ago
|
||
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.
Attachment #487974 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 31•14 years ago
|
||
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.
Attachment #487974 -
Attachment is obsolete: true
Attachment #488456 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 32•14 years ago
|
||
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.
Reporter | ||
Comment 33•14 years ago
|
||
(In reply to comment #32) > but the > suggested one isn't satisfactory. perhaps, that's document issue - I see it can be ported well
Reporter | ||
Comment 34•14 years ago
|
||
(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.
Reporter | ||
Comment 35•14 years ago
|
||
"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
Reporter | ||
Comment 36•14 years ago
|
||
"textarea", false, true, true, false); //////////////////////////////////////////////////////////////////////// // getTextBeforeOffset please add empty line between test end and comment.
Reporter | ||
Comment 37•14 years ago
|
||
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?
Reporter | ||
Comment 38•14 years ago
|
||
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?
Assignee | ||
Comment 39•14 years ago
|
||
Updated addressing last comments
Attachment #488456 -
Attachment is obsolete: true
Attachment #488893 -
Flags: review?(surkov.alexander)
Attachment #488456 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 40•14 years ago
|
||
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
Reporter | ||
Comment 41•14 years ago
|
||
> 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.
Reporter | ||
Comment 42•14 years ago
|
||
Comment on attachment 488893 [details] [diff] [review] patch7 please file new patch with comments addressed
Attachment #488893 -
Flags: review?(surkov.alexander) → review-
Assignee | ||
Comment 43•14 years ago
|
||
(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.
Assignee | ||
Comment 44•14 years ago
|
||
(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?
Reporter | ||
Comment 45•14 years ago
|
||
(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
Assignee | ||
Comment 46•14 years ago
|
||
(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?
Assignee | ||
Comment 47•14 years ago
|
||
Updated patch
Attachment #488893 -
Attachment is obsolete: true
Attachment #489042 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 48•14 years ago
|
||
(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.
Reporter | ||
Comment 49•14 years ago
|
||
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.
Attachment #489042 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 50•14 years ago
|
||
Updated patch. Who should I ask for superreview?
Attachment #489042 -
Attachment is obsolete: true
Reporter | ||
Comment 51•14 years ago
|
||
(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 :)
Reporter | ||
Updated•14 years ago
|
Attachment #490048 -
Flags: review?(bolterbugz)
Comment 52•14 years ago
|
||
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.
Attachment #490048 -
Flags: review?(bolterbugz) → review+
Assignee | ||
Comment 53•14 years ago
|
||
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.
Reporter | ||
Comment 54•14 years ago
|
||
But we're doing correct things, right?
Comment 55•14 years ago
|
||
Wait, which documentation are we using?
Assignee | ||
Comment 56•14 years ago
|
||
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•14 years ago
|
||
If Li is correct (on the GNOME bug) regarding start and end offsets always being 'inside the word' then I think we are correct.
Reporter | ||
Comment 58•14 years ago
|
||
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/0c22e26c4fd5
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•