Closed Bug 928163 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 27
Tracking Status
firefox27 --- verified

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(2 files, 1 obsolete file)

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/
<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.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch bug928163 Fixes Part 2 (obsolete) — Splinter Review
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.
Attachment #819546 - Flags: review?(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 :)
(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.
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.
(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 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.
Attachment #819546 - Flags: review?(margaret.leibovic) → review-
Ok then, that's how I originally did it, but then "thought I thought better" :-)
Attachment #819546 - Attachment is obsolete: true
Attachment #820377 - Flags: review?(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?
Attachment #820377 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/ced482976ebf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.