Last Comment Bug 614585 - mochitest test selection functions for nsIAccessibleText
: mochitest test selection functions for nsIAccessibleText
Status: RESOLVED FIXED
[bk1]
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-24 08:48 PST by Fernando Herrera
Modified: 2012-05-10 18:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch1 (10.44 KB, patch)
2010-11-24 08:50 PST, Fernando Herrera
no flags Details | Diff | Splinter Review
patch2 (11.41 KB, patch)
2010-11-26 05:21 PST, Fernando Herrera
no flags Details | Diff | Splinter Review
Patch (v3) (10.63 KB, patch)
2012-05-06 02:11 PDT, Mark Capella [:capella]
surkov.alexander: review+
hub: feedback+
Details | Diff | Splinter Review
Patch (v4) (9.29 KB, patch)
2012-05-09 00:37 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description Fernando Herrera 2010-11-24 08:48:04 PST

    
Comment 1 Fernando Herrera 2010-11-24 08:50:24 PST
Created attachment 493016 [details] [diff] [review]
patch1

First take into testing selections for TextInterface
Comment 2 alexander :surkov 2010-11-25 09:48:25 PST
Btw, could you move test_text_caret.html into text folder and perhaps it makes sense to move attributes/test_text.html -> text/test_attributes.html since it sounds we're going to keep nsIAccessibleText interface tests in one folder. It shouldn't be done in this bug though.
Comment 3 alexander :surkov 2010-11-25 10:09:52 PST
Comment on attachment 493016 [details] [diff] [review]
patch1

nits and suggestions for the first pass.

>+/**
>+ * Remove all previous selections.

nit: Remove all selections? Just "previous" confuses me here.

>+ *
>+ * @param aElement        [in] Element identifier

nit: Element -> element

>+ */
>+function cleanTextSelections(aID) {

nit: brace on new line please

I'm not sure but perhaps it makes sense to turn it into testing method too so that when you remove all selections then you ensure they were removed actually.

>+/**
>+ * test addSelection.

test -> Test

>+ *
>+ * @param aElement        [in] Element identifier
>+ * @param aTestString     [in] String defining the test

it's not clear how strings can define a test

>+ * @param aStartOffset    [in] Start offset for the new selection
>+ * @param aEndOffset      [in] End offset for the new selection
>+ * @param aSelections     [in] Expected number of selections after addSelection

use lowercase please

>+ * @param aToDoFlag       [in] kTodo or kOk for the final selections count
>+ */
>+function testTextAddSelection(aID, aTestString, aStartOffset, aEndOffset,
>+                              aSelections, aToDoFlag)

I think it makes sense to test not the selection number only but test selection we've got, i.e. getSelection

>+{
>+  var acc = getAccessible(aID, nsIAccessibleText);
>+  var isFunc = (aToDoFlag == kTodo) ? todo_is : is;
>+
>+  acc.addSelection(aStartOffset, aEndOffset);
>+
>+  isFunc(acc.selectionCount,aSelections, aTestString +

space between arguments

>+         ": error adding selection from offset '" + aStartOffset +

nit: "failed to add"?

>+function testTextRemoveSelection(aID, aTestString, aSelectionIndex,
>+                                 aSelections, aToDoFlag)
>+function testTextSetSelection(aID, aTestString, aSelectionIndex, aStartOffset,
>+                              aEndOffset, aSelections, aToDoFlag)


the same for this?

It makes sense to have tests when you manage the selection by DOM selection. So ideally we should have both tests, perhaps in separate files.

>+  <title>test selection functions</title>

Test text selection functions?

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

new line between these?

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

new line

>+    
>+    function doTest()
>+    {
>+      var testString;
>+      var acc = getAccessible("div", nsIAccessibleText);
>+
>+      cleanTextSelections("div");
>+      testString = "Test adding two equal selections";

perhaps it makes sense to get test string from text accessible since you use it for comments?

>+      testTextAddSelection("div", testString, 0, 3, 1, kOk);
>+      testTextAddSelection("div", testString, 0, 3, 1, kOk);
>+
>+      cleanTextSelections("div");
>+      testString = "Test adding 3 selections (one overlapping) and getting back";
>+      testTextAddSelection("div", testString, 0, 3, 1, kOk);
>+      testTextAddSelection("div", testString, 7, 9, 2, kOk);
>+      testTextAddSelection("div", testString, 3, 4, 3, kOk);
>+      testTextGetSelection("div", testString, 0, 0, 3, kOk, kOk);
>+      testTextGetSelection("div", testString, 1, 3, 4, kOk, kOk);
>+      testTextGetSelection("div", testString, 2, 7, 9, kOk, kOk);

that would be really helpful if you provide these test blocks by comments what you want to test.

>+      testTextGetSelection("topdiv", testString, 0, 7, 8, kOk, kOk);
>+
>+
>+
>+      SimpleTest.finish();

too many empty lines between these.

>+  <pre id="test">
>+  <div id="div">This is a test with multiple words</div>
>+  <div id="topdiv">This is a div containing more elements
>+    <div id="innerdiv1">First inner div</div>
>+    <div id="innerdiv2">Second inner div</div>
>+</div>

indent a div properly please
Comment 4 Fernando Herrera 2010-11-26 05:20:41 PST
(In reply to comment #3)

> >+function cleanTextSelections(aID) {
> 
> nit: brace on new line please
> 
> I'm not sure but perhaps it makes sense to turn it into testing method too so
> that when you remove all selections then you ensure they were removed actually.

Well, I though about it, but the only thing we could test after actual removing is acc.selectionCount == 0, and if that is not true, we are entering into a infinite loop.
Comment 5 Fernando Herrera 2010-11-26 05:21:23 PST
Created attachment 493374 [details] [diff] [review]
patch2

Updated patch
Comment 6 alexander :surkov 2010-11-26 05:50:28 PST
Fernando, could you please run through comment #3 and comment things unaddressed in your last patch?
Comment 7 alexander :surkov 2011-08-26 03:05:55 PDT
Comment on attachment 493374 [details] [diff] [review]
patch2

canceling review until comment #6 is addressed
Comment 8 Hubert Figuiere [:hub] 2011-11-01 08:05:26 PDT
Hi Fernando, any chance you could have a look at comment #6 ?

Thanks,
Comment 9 Mark Capella [:capella] 2012-05-06 02:11:14 PDT
Created attachment 621393 [details] [diff] [review]
Patch (v3)

Asking Hub for feedback, as he was the last to inquire on this old request ... I'm assuming we still want it completed.

I re-based the original patch, addressed Surkov's comments, fix a couple bugs and cleaned / trimmed some code on my own ... this patch builds locally and tests ok, with a couple TODO's left. (FYI, the first one's a timing issue, the second one I left as-is without further investigation.)

Let me know what you think -- mark
Comment 10 Hubert Figuiere [:hub] 2012-05-06 23:45:28 PDT
Comment on attachment 621393 [details] [diff] [review]
Patch (v3)

I just commented part of the bug kill.... (aka bug triage)

I'd say, get Alex Surkov to review this. We like good test.
Comment 11 Mark Capella [:capella] 2012-05-07 01:31:07 PDT
Comment on attachment 621393 [details] [diff] [review]
Patch (v3)

Trying to keep Surkov busy :)
Comment 12 alexander :surkov 2012-05-07 04:21:39 PDT
Comment on attachment 621393 [details] [diff] [review]
Patch (v3)

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

thank you for finishing this one!

::: accessible/tests/mochitest/text.js
@@ +339,5 @@
>  
> +/**
> + * Remove all selections.
> + *
> + * @param aElement        [in] element identifier

nit: two spaces indentation is enough

@@ +341,5 @@
> + * Remove all selections.
> + *
> + * @param aElement        [in] element identifier
> + */
> +function cleanTextSelections(aElement)

nit: usually we name it as aID (because it can be either id, DOM node or accessible object), here and below

@@ +349,5 @@
> +    acc.removeSelection(0);
> +}
> +
> +/**
> + * Test addSelection.

nit: add 'method' word? here and below

@@ +354,5 @@
> + *
> + * @param aElement        [in] element identifier
> + * @param aStartOffset    [in] start offset for the new selection
> + * @param aEndOffset      [in] end offset for the new selection
> + * @param aSelections     [in] expected number of selections after addSelection

maybe 'aSelectionsCount'?

@@ +368,5 @@
> +  acc.addSelection(aStartOffset, aEndOffset);
> +
> +  isFunc(acc.selectionCount, aSelections, text +
> +         ": failed to add selection from offset '" + aStartOffset +
> +         "' to offset '" + aEndOffset + "': selectionCount after");

I'm not sure why you add ": selectionCount after" string, here and below

@@ +398,5 @@
> +/**
> + * Test selectionCount.
> + *
> + * @param aElement        [in] element identifier
> + * @param aCount          [in] expected count

expected selection count?

@@ +429,5 @@
> +  var startObj = {}, endObj = {};
> +  acc.getSelectionBounds(aSelectionIndex, startObj, endObj);
> +
> +  is(startObj.value, aStartOffset, text +
> +     ": wrong start offset getting selection '" + 

maybe you could reword somehow 'wrong start offset getting selection' (I failed to translate it).

@@ +431,5 @@
> +
> +  is(startObj.value, aStartOffset, text +
> +     ": wrong start offset getting selection '" + 
> +     aSelectionIndex + "'");
> +  is(endObj.value, aEndOffset, text +

please consider to keep 'text +' on the same line with other text (it's small enough)

@@ +448,5 @@
> + *                             setSelectionBounds
> + * @param aToDoFlag       [in] kTodo or kOk for the final selections count
> + */
> +function testTextSetSelection(aElement, aSelectionIndex, aStartOffset,
> +                              aEndOffset, aSelections)

nit: logically it seems more correct to keep this method grouped with addSelection/removeSelection but it's up to you

also I'm curious if making all function to take aStartOffset/aEndOffset before other parameters make the code more readable

::: accessible/tests/mochitest/text/test_selection.html
@@ +21,5 @@
> +    
> +    function doTest()
> +    {
> +      // Test selection count: clean selection / check count.
> +      cleanTextSelections("div0");

you do clean text selection for each case, I can see it's reasonable to do once (to make sure RemoveSelection(0) when no selection doesn't break anything) but does it useful to do for each case?

@@ +39,5 @@
> +      testTextAddSelection("div2", 7, 9, 2);
> +      testTextAddSelection("div2", 3, 4, 3);
> +      testTextGetSelection("div2", 0, 0, 3);
> +      testTextGetSelection("div2", 1, 3, 4);
> +      testTextGetSelection("div2", 2, 7, 9);

a little bit funny to have (0, 3) and (3, 4) selections but it's out implementation

@@ +53,5 @@
> +      // Test changing a selection that adds a new selection.
> +      cleanTextSelections("div4");
> +      testTextAddSelection("div4", 1, 3, 1);
> +      testTextAddSelection("div4", 7, 9, 2);
> +      testTextSetSelection("div4", 0, 4, 6, 3);

I must be missing something but do you have any ideas why we setSelection adds new range?

@@ +62,5 @@
> +      testTextAddSelection("div5", 7, 9, 2);
> +      testTextSetSelection("div5", 1, 0, 1, 3);
> +      testTextGetSelection("div5", 0, 0, 1);
> +      testTextGetSelection("div5", 1, 4, 5);
> +      testTextGetSelection("div5", 2, 7, 9);

for code readability I would suggest to comment each call like
testTextAddSelection("div5", 4, 5, 1); // 'Test| |changing...'
testTextAddSelection("div5", 7, 9, 2); // 'Test| |ch|an|ging...'
testTextSetSelection("div5", 1, 0, 1, 3); //

@@ +98,5 @@
> +
> +<body>
> +  <a target="_blank"
> +     title="test selection functions"
> +     href="https://bugzilla.mozilla.org/show_bug.cgi?id=614585">Mozilla Bug 614585</a>

you're allowed to skip bug number since it doesn't point to real bug (broken behavior)
Comment 13 alexander :surkov 2012-05-07 04:23:06 PDT
(In reply to Mark Capella [:capella] from comment #11)
> Trying to keep Surkov busy :)

you do and thank you for doing this :)
Comment 14 Mark Capella [:capella] 2012-05-08 10:59:10 PDT
FYI re: I'm not sure why you add ": selectionCount after" string, here and below

Additional information is added by the method if it fails, ie: "...selectionCount after - got 3, expected 4"
Comment 15 Mark Capella [:capella] 2012-05-09 00:37:11 PDT
Created attachment 622309 [details] [diff] [review]
Patch (v4)

Well, this took a little digging, but I finally resolved all TODO's, and all tests work and display as designed. The key to the answer is commented in the code, I'm sure you'll see it.

I'm not sure why it behaves in this fashion, only that now I understand it.
Comment 16 Marco Zehe (:MarcoZ) 2012-05-09 01:59:09 PDT
Comment on attachment 622309 [details] [diff] [review]
Patch (v4)

>+      // NOTE: If inner elements are represented as embbeded chars

Typo, "embedded".
Comment 17 alexander :surkov 2012-05-10 00:14:03 PDT
Comment on attachment 622309 [details] [diff] [review]
Patch (v4)

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

::: accessible/tests/mochitest/text/test_selection.html
@@ +43,5 @@
> +      // Test selection re-ordering: adding two selections.
> +      // NOTE: removeSelections aSelectionIndex is from start of document.
> +      testTextAddSelection("div3", 0, 3, 1); // |Tes|t adding 2...
> +      testTextAddSelection("div3", 7, 9, 2); // |Tes|t ad|di|ng 2...
> +      testTextRemoveSelection("div3", 4, 1); // Test ad|di|ng 2...

how does it happen that it doesn't fail since there's no selection at 4 index?

@@ +48,5 @@
> +
> +      // Test extending existing selection.
> +      // NOTE: setSelectionBounds aSelectionIndex is from start of document.
> +      testTextAddSelection("div4", 4, 5, 1); // Test| |extending...
> +      testTextSetSelection("div4", 4, 9, 6, 1); // Test| exte|nding...

6 index of selection?

@@ +53,5 @@
> +
> +      // Test moving an existing selection.
> +      // NOTE: setSelectionBounds aSelectionIndex is from start of document.
> +      testTextAddSelection("div5", 1, 3, 1); // T|es|t moving...
> +      testTextSetSelection("div5", 5, 9, 6, 1); // Test |movi|ng...

same
Comment 18 alexander :surkov 2012-05-10 07:14:37 PDT
(per irc chat with Mark) the cause of large selection indexes is because the selections are counting per document which is our bug. I'll file it.
Comment 20 alexander :surkov 2012-05-10 07:29:17 PDT
(In reply to alexander :surkov from comment #18)
> (per irc chat with Mark) the cause of large selection indexes is because the
> selections are counting per document which is our bug. I'll file it.

bug 753771
Comment 21 Joe Drew (not getting mail) 2012-05-10 18:42:04 PDT
https://hg.mozilla.org/mozilla-central/rev/9b326e2ad041

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