Closed Bug 610568 Opened 9 years ago Closed 9 years ago

mochitest with extra whitespaces for nsIAccessibleText

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: fherrera, Assigned: fherrera)

References

Details

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.28 Safari/534.10
Build Identifier: 

Add tests with extra whitespaces and punctuation signs for nsIAccessibleText getText functions.

Reproducible: Always
Attached patch patch1 (obsolete) — Splinter Review
Initial bunch of tests. Still need to add tests for divs where the string is collapsed
Attachment #489075 - Flags: review?(surkov.alexander)
Assignee: nobody → fherrera
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to comment #1)
> Initial bunch of tests. Still need to add tests for divs where the string is
> collapsed

use pre tag
perhaps you will hate me but can we get smaller test string?

      // __B__r__a__v__e__ __ __S__i__r__ __ __ __R__o__b__i__n__ __ __ __ __ __ __r__a__n__ __ __a__w__a__y__.__.__.__ __b__r__a__v__e__l__y__ 

24 	

      //  0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44

or could you describe what kind of special test it allows us?
Well, I wanted to test different amounts of whitespaces (2,3,5,2,1) and also the usage of "..." extra characters.
Attached patch patch2 (obsolete) — Splinter Review
Updated patch extending test to div and editable elements inside a <pre>
Attachment #489075 - Attachment is obsolete: true
Attachment #489075 - Flags: review?(surkov.alexander)
(In reply to comment #4)
> Well, I wanted to test different amounts of whitespaces (2,3,5,2,1) and also
> the usage of "..." extra characters.

Ok I agree it might make sense to test 1, 2, 3. But what's about 5, why it is special?
Btw, does ATK spec have definition of 'word' term? I wonder about 'away...' where's the word here.
and btw why do you need to test '...' boundaries here? In general I would like to see small test string as possible.
ok, so let keep this one just for single vs. multiple whitespaces and I'll add another one testing words.

So, what about using this string:
__B__r__a__v__e__ __S__i__r__ __ __R__o__b__i__n__ __ __ __r__a__n
  0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21
Should we also skip line boundaries testing here or is there any reason to keep them?
(In reply to comment #9)
> ok, so let keep this one just for single vs. multiple whitespaces and I'll add
> another one testing words.
> 
> So, what about using this string:
> __B__r__a__v__e__ __S__i__r__ __ __R__o__b__i__n__ __ __ __r__a__n
>   0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21

sounds good till it fits to 80 lines restriction ;)

(In reply to comment #10)
> Should we also skip line boundaries testing here or is there any reason to keep
> them?

I think we could skip'em. I don't see any peculiarities they could bring.
Attached patch patch3 (obsolete) — Splinter Review
Updated patch with new test string
Attachment #490062 - Attachment is obsolete: true
Attachment #491599 - Flags: review?(surkov.alexander)
Comment on attachment 491599 [details] [diff] [review]
patch3


>+  <title>nsIAccessibleText getText related function tests with whitespaces for html:input,html:div and html:textarea</title>

perhaps something like that?
getText... methods tests on string with whitespaces for plain text containers

>+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />

put @href on new line please

>+  <script type="application/javascript"
>+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript"
>+          src="../common.js"></script>

add new line between standard js and a11y js.

>+    function doTest()
>+    {
>+      // Tests for elements rendering the original sring
>+      var IDs = ["input", "div", "editable", "textarea"];
>+  
>+
>+      // __B__r__a__v__e__ __S__i__r__ __ __R__o__b__i__n__ __ __ __r__a__n

remove one empty line

>+      //   0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21
>+      ////////////////////////////////////////////////////////////////////////

but please add new one between these

please put char index before char like we do in test_single.html so that index points to the char visually
;
>+      testTextAfterOffset(9, BOUNDARY_WORD_START, "Sir   ", 7, 13,
>+      testTextAfterOffset(10, BOUNDARY_WORD_START, "Sir   ", 7, 13,

should be Robin

>+      testTextAfterOffset(21, BOUNDARY_WORD_END, "", 22, 22,

please add test for 22 as well

>+      testTextBeforeOffset(21, BOUNDARY_WORD_START, "Robin   ", 11, 19,

please add test for 22 as well

>+      testTextBeforeOffset(22, BOUNDARY_WORD_END, "   ran", 16, 22,

should be "  Robin" because "... to the word end before the offset", word end before the 22 offset is "Robin" since 22 is end offset of "run".

>+      testTextAtOffset(21, BOUNDARY_WORD_START, "ran", 19, 22,

please add test for 22 as well

>+      testTextAtOffset(21, BOUNDARY_WORD_END, "   ran", 16, 22,

please add test for 22 as well
Attachment #491599 - Flags: review?(surkov.alexander)
Attached patch patch4 (obsolete) — Splinter Review
Updated patch
Attachment #491599 - Attachment is obsolete: true
Attachment #491831 - Flags: review?(surkov.alexander)
Comment on attachment 491831 [details] [diff] [review]
patch4

r=me, thank you
Attachment #491831 - Flags: review?(surkov.alexander) → review+
Attached patch patch5Splinter Review
Just forgot to update kOk/kTodo status for new tests.
Attachment #491831 - Attachment is obsolete: true
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/b89d1824a762
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.