928163 - Allow or disallow proper text selection caret display based on HTML <input> type

Status: VERIFIED FIXED

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

Description

[Mark Capella [:capella]]

Of the 23 possible html <input> possible input types
http://www.w3schools.com/tags/tag_input.asp

We are currently not displaying a text select caret for type=number
And incorrectly displaying (due to enhanced Java input methods) carets where not required, for types=date, datetime-local, month, time and week.

navigate to test page: http://jsfiddle.net/vCUdF/9/

Comment 1

[Mark Capella [:capella]]

<input> field type=number is being excluded from having a TextSelection caret here:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.h?mark=1089-1090#1084

implemented in bug 835055, and referencing bug 635240 as the solution, and just passed review+  :-)
So this may be repaired in the next few days.

Comment 2

[Mark Capella [:capella]]

Attachment: bug928163 Fixes Part 2

part 2 of the original comment - see attached screenshot
   re: |And incorrectly displaying (due to enhanced Java input methods) carets where not required|

This fixes the problem, where user is navigating into <input> fields that we support with native android inputHelpers.

Basically, the issue is that after the inputHelper has changed the elements value, we don't adjust the caret position to the new cursor location.

Comment 3

[Mark Capella [:capella]]

Attachment: Screenshot part 2 problem

Comment 4

[:Margaret Leibovic]

I wonder if now would be a good time to try to write some tests for text selection... it could be interesting to include a testcase page like this, try changing the focus to different elements, then checking to see if the caret is shown or not. I haven't thought about this in too much detail, but maybe we do something similar to what wesj did for testPromptGridInput:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testPromptGridInput.java.in
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/robocop_prompt_gridinput.html

That is, we can load a chome-privledged test page in a normal robocop Java test, but then have some JS in that page change the focus around and check to see whether or not the handle is shown. Although maybe that part would be better done on the Java side? So maybe we could just load a regular test page and do tapping/checking for the handle all in a regular robocop test.

Anyway, just wanted to put some of these ideas out there :)

Comment 5

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #2)
> Created attachment 819546 [details] [diff] [review]
> bug928163 Fixes Part 2
> 
> 
> part 2 of the original comment - see attached screenshot
>    re: |And incorrectly displaying (due to enhanced Java input methods)
> carets where not required|
> 
> This fixes the problem, where user is navigating into <input> fields that we
> support with native android inputHelpers.
> 
> Basically, the issue is that after the inputHelper has changed the elements
> value, we don't adjust the caret position to the new cursor location.

I'm trying to get a sense of what we really want in all these different situations, and I feel like we shouldn't be showing the caret at all for inputs that have native picker widgets. Do you agree with that?

If so, maybe the real thing we need to do here is update IsExperimentalMobileType. I'm cc'ing Mounir in case he has any advice to give here.

Comment 6

[Mark Capella [:capella]]

bug 928163 does that for part one iirc from reading it this weekend

for part two I wasn't sure diasbling was correct for helper widget not available fall what do we do, so I settled for just always positioning correctly.

Comment 7

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #6)
> bug 928163 does that for part one iirc from reading it this weekend
> 
> for part two I wasn't sure diasbling was correct for helper widget not
> available fall what do we do, so I settled for just always positioning
> correctly.

I think disabling it would be better, since the user can't actually manually edit the entry anyway, since the keyboard doesn't come up in those cases.

Comment 8

[:Margaret Leibovic]

Comment on attachment 819546 [details] [diff] [review]
bug928163 Fixes Part 2

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

I'm going to give this an r-, since I think we should go with the approach of disabling the caret for these cases.

Comment 9

[Mark Capella [:capella]]

Attachment: bug928163 Fixes Part 2 (v2)

Ok then, that's how I originally did it, but then "thought I thought better" :-)

Comment 10

[:Margaret Leibovic]

Comment on attachment 820377 [details] [diff] [review]
bug928163 Fixes Part 2 (v2)

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

This looks good to me, although I was confused by the name of this helper method, so I think we should rename it.

::: mobile/android/chrome/content/browser.js
@@ +4312,2 @@
>              if (!element.disabled &&
> +                !InputWidgetHelper.isValidInput(element) &&

I want us to rename this isValidInput method, because it's confusing that we want to attach a caret if we *don't* have a "valid input". Maybe something like hasInputWidget?

Comment 11

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=d8110b1365f1

Comment 12

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/ced482976ebf

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ced482976ebf