1116862 - Implement testInputSelections for single-line editables

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Capella [:capella]]

As we're creating new tests in support of RTL cleanup in bug 1106800 for <textarea> elements (multi-line input editables), I'd like to implement parallel test sets for <input> elements (single-line input editables).

These will be simpler versions of the <textarea> tests, as they will target and exercise less complex (different) SelectionHandler.js code-paths.

These tests as planned, will:
   a) depend on bug 1096938 comment #5 to enable use of .setSelectionRange()
   b) protect against accidental regressions while implementing / fixing bug 1106800
   c) drive down / tighten up overall code in this area.
   d) get a framework in place for future tests.

Comment 1

[Mark Capella [:capella]]

Attachment: bug1116862_p1of3_testInputSelections.diff

Initial implementation, simple tests / assumption confirmations.

Comment 2

[Mark Capella [:capella]]

Attachment: bug1116862_p2of3_testInputSelections.diff

Expanded testing begins, add drag-and-drop handle tests for LTR and RTL <input> fields.

LTR Drag focusHandle to itself after selectAll(), examine screen positions, etc.
Same for LTR anchorHandle, RTL focus and anchor handles.

TODO:s flushed out for (edge-case?) failures in testLTR_dragAnchorHandleToSelf() and testRTL_dragFocusHandleToSelf().

Looks like I'll need to file followups for further research.

Comment 3

[Mark Capella [:capella]]

Attachment: bug1116862_p3_testInputSelections.diff

Adds tests for select-word (long-tap simulations) for LTR and RTL <input> editables.

Comment 4

[Mark Capella [:capella]]

Can I ping you on these final test reviews? I've got a solution for Bug 1123606 - Fix RTL Logic in SelectionHandler that I'm dogfooding, and I'd like these in place first.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8543326 [details] [diff] [review]
bug1116862_p1of3_testInputSelections.diff

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

Apologies for the slow review. Feel free to land this if there's a green try run.

However, I have some suggestions for improvements that I think we should explore, either here or in follow-up bugs. At the very least, please file follow-up bugs for these :)

::: mobile/android/base/tests/roboextender/testInputSelections.html
@@ +51,5 @@
> + *    ) The UI Selection anchor handle is left of the focus handle.
> + */
> +function testLTR_selectAll() {
> +  // Select entire LTR Input element.
> +  var sh = getSelectionHandler();

This is something in the other tests as well, but we should be using let instead of var. Can you fix that here, and file a follow-up mentor bug to update the other tests?

@@ +63,5 @@
> +  var anchorOffset = selection.anchorOffset;
> +  var focusNode = selection.focusNode;
> +  var focusOffset = selection.focusOffset;
> +
> +  var anchorPt = new Point(sh._cache.anchorPt.x, sh._cache.anchorPt.y);

Why do we need to make a new Point, instead of just using sh._cache.anchorPt here?

@@ +157,5 @@
> +    is(focusNode.nodeName, TEXT_NODE, "RTL Focus node is a TEXT node."),
> +    ok(!document.contains(focusNode), "RTL Focus node is an anonymous TEXT node."),
> +    is(focusNode.parentNode, anchorNode, "RTL Focus node is a child of the Anchor DIV node."),
> +    is(focusOffset, RTL_INPUT_TEXT_VALUE.length,
> +      "RTL Selection ends at Focus node with offset of the RTLInput node length."),

Without comparing exactly line-by-line, it seems like these checks are all the same as what we have above, except with "RTL" instead of "LTR" in the messages, and a different expected value. Could we factor this out into a shared function?

@@ +192,5 @@
> +
> +/* ============================== Utility functions ======================
> + *
> + * Common functions available to all tests.
> + *

I don't love that these are just copy/pasted from other tests. We should see if there's a place to share them. We could just put them in a JS file that gets loaded at the top of this HTML file, the same way we do with EventUtils.js.

One good reason for switching this to use JavascriptTest is that we would be able to just use the JS assert functions built into the framework, rather than implementing our own message-passing dance. Can you file a follow-up to investigate this?

::: mobile/android/base/tests/testInputSelections.java
@@ +9,5 @@
> +
> +import org.json.JSONObject;
> +
> +
> +public class testInputSelections extends UITest {

This class is basically exactly the same as testTextareaSelections. I try to avoid premature abstraction, but maybe it could be a good follow-up to move this logic into a shared parent class.

Comment 6

[Mark Capella [:capella]]

All good points ... I'd love to get back and consolidate some of the common functions.

fwiw ... "let" does not work. I can't explain it, but unless I use "var" the code fails.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8543332 [details] [diff] [review]
bug1116862_p2of3_testInputSelections.diff

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

Same story about a green try run.

::: mobile/android/base/tests/roboextender/testInputSelections.html
@@ +187,5 @@
>  /* =================================================================================
>   *
> + * If we selectAll() in a LTR <input>, then:
> + *    ) drag the focus handle to itself, the selected text, and the
> + *      selection anchor and focus points should all remain the same.

This sentence doesn't make sense to me.

@@ +208,5 @@
> +    JSON.stringify({ handleType : FOCUS,
> +      x : initialSelection.focusPt.x,
> +      y : initialSelection.focusPt.y
> +    })
> +  );

It's cool that we can unit test the JS by sending messages like this :)

I suppose it's fine to directly call observe like this, but you could also call Services.obs.notifyObservers(...) to fire the notification. That would also test that SelectionHandler is actually listening as it should (and that nothing else is interfering).

Also, I realize this is something you've been doing in other tests, sorry for just pointing it out here :(

Comment 8

[Mark Capella [:capella]]

Comment on attachment 8543663 [details] [diff] [review]
bug1116862_p3_testInputSelections.diff

I need to drop the approval request for this patch. This works perfectly locally, but on pushing to try it fails infra-style / hard.

Somethings different in the two environments and I'm trying track it down.

The first two patchs work fine on try.

Comment 9

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #6)

> fwiw ... "let" does not work. I can't explain it, but unless I use "var" the
> code fails.

Any specific error? Does this happen if you only switch out a single "var" for "let"?

I suspect the problem is that you need to declare type="application/javascript;version=1.8" in your script tag.

Comment 10

[Mark Capella [:capella]]

yep, type="application/javascript;version=1.8" did it ... such a stupid thing to get stuck on |:-(

Comment 11

[Mark Capella [:capella]]

awesomeness
https://tbpl.mozilla.org/?tree=Try&rev=6739f05a446d

Comment 12

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/08ad468d5ed9
https://hg.mozilla.org/integration/fx-team/rev/baa6428bb048

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08ad468d5ed9
https://hg.mozilla.org/mozilla-central/rev/baa6428bb048