Closed Bug 1121586 Opened 5 years ago Closed 5 years ago

[Dialer][Text Selection] Hitting the Delete button will remove the digit from the end of a string of entered digits instead of from where the cursor is currently located.

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S5 (6feb)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: jmitchell, Assigned: salva)

References

()

Details

(Whiteboard: [3.0-Daily-Testing])

Attachments

(4 files)

Description:
 When in the dialer app (dial pad) you can tap in the middle of a string of entered digits to move the cursor to that position. When you hit the delete key the typical expectation will be to delete the digit in the position to the left of the cursor. Instead; the digit at the very end of the string of digits is deleted. This would prevent the user from only changes a single mistyped number in the middle or beginning of the string without having to delete the entire string.
This is also different than other market devices which follows the expected results of deleting the digit to the left of the cursor (Verified on Android ZTE Savvy)

Repro Steps:
1) Update a Flame to 20150114010205
2) Launch Dialer 
3) Type in 6 or 7 digits
4) Tap in the middle of the string of digits to move the cursor to there
5) Hit the delete button

Actual:
The # at the end of the string is deleted

Expected:
The # to the left of the cursor will be deleted

Environmental Variables:
Device: Flame Master
Build ID: 20150114010205
Gaia: e2a0f7c311119d4a8e160bdfb9e28a0e61a180fc
Gecko: 63006936ab99
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 38.0a1 (Master)
Firmware Version: V18d-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Repro frequency: 7/7
See attached: logcat - video clip: http://youtu.be/VOi8ohi2zPM

-------------------------------------------------------------------------------

This also occurs on 2.2 (V18d-1) and (V18d)  Text Selection in not a feature prior to 2.2

Device: Flame 2.2 (KK - Nightly - Full-Flashed)
Build ID: 20150114002502
Gaia: 7c5b27cad370db377b18a742d3f3fdb0070e899f
Gecko: 748b20315f75
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a2 
Firmware Version: V18d-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.2 (KK - Nightly - Full-Flashed)
Build ID: 20150114002502
Gaia: 7c5b27cad370db377b18a742d3f3fdb0070e899f
Gecko: 748b20315f75
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a2
Firmware Version: V18d
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Blocks: 1092427
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
Duplicate of this bug: 1121885
[Blocking Requested - why for this release]: The UI looks confusing now that the Text Selection feature is enabled. A user may think that this new feature is broken.

Carrie, can we do something to help to make things more clear?
blocking-b2g: --- → 2.2?
Flags: needinfo?(cawang)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][COM=Gaia::Dialer]
triage: very confusing to end user.
Carrie, please advise the correct behavior
blocking-b2g: 2.2? → 2.2+
Hi, 

If user locates the cursor in the middle of the numbers, we shall always delete the number from where the cursor is. If the user didn't locate the cursor, we shall delete the numbers from the end of the input digits. Thanks!
Flags: needinfo?(cawang)
Assignee: nobody → salva
Hi Gabriel,

Do you mind to take a look at the patch to check if you feel comfortable with the proposed solution, please? I'm going to write a comment now pointing out all the issues I found while addressing this bug.

Thank you!
Attachment #8552577 - Flags: feedback?(gsvelto)
Hello.

While fixing this bug I found some other problems noteworthy:

 1- Adding digits at the cursor position: in the same way we have a problem deleting characters at the cursor position, we have another problem related with adding more digits. Once the users alter the cursor position, new taps on the keys always add the digits at the end. Although I think this should be a different bug.

 2- Can not paste in the dialer: just that, you can select, copy but not paste.

 3- Can not force cursor to be shown: with the provided patch, even if we fix this bug, each time the delete key is tapped, the cursor dissapear. It's still there, and you can delete the previous character but it's not shown. This could led the user to confussion.

Asking UX for the 3 items above. I would want to confirm both UX and me see some problems here.
Asking Etienne for the caret item. Do you know any way to force the cursor to be shown?
Flags: needinfo?(etienne)
Flags: needinfo?(cawang)
Haven't touch this in a while, redirecting the NI.
Flags: needinfo?(etienne) → needinfo?(drs.bugzilla)
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

I'm taking this since Gabriele is on PTO today.

(In reply to Salvador de la Puente González [:salva] from comment #6)
>  3- Can not force cursor to be shown: with the provided patch, even if we
> fix this bug, each time the delete key is tapped, the cursor dissapear. It's
> still there, and you can delete the previous character but it's not shown.
> This could led the user to confussion.

> Asking Etienne for the caret item. Do you know any way to force the cursor
> to be shown?

This is happening because the phone number field is losing focus. It can be fixed by setting a 'focus' event listener on the delete button, and then returning focus to the phone number field.

This otherwise seems reasonable to me.
Flags: needinfo?(drs.bugzilla)
Attachment #8552577 - Flags: feedback?(gsvelto) → feedback+
Hi Dough, what do you prefer for item 1:

>  1- Adding digits at the cursor position: in the same way we have a problem
> deleting characters at the cursor position, we have another problem related
> with adding more digits. Once the users alter the cursor position, new taps
> on the keys always add the digits at the end. Although I think this should
> be a different bug.

Should we file a different bug or reword this one to include this case?
Flags: needinfo?(drs.bugzilla)
s/Dough/Doug/
(In reply to Salvador de la Puente González [:salva] from comment #9)
> Should we file a different bug or reword this one to include this case?

Please file a new bug for that and set "See Also: bug 11121586" and "blocking-b2g: 2.2?".
Flags: needinfo?(drs.bugzilla)
See Also: → 1126538
I may need some input from keyboard UX here.
Flags: needinfo?(cawang) → needinfo?(ofeng)
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

Hi Doug, do you mind reviewing the patch? Thank you.
Attachment #8552577 - Flags: review?(drs.bugzilla)
By the way, related with the UX, I discovered another not covered case. Now, if we keep pressed the delete key, we clear the whole number. What if the cursor is set in the middle of the number? In case of a long press, what should we do? I was thinking about clear from the beginning to the cursor's position bu I think we need UX input here.
For comment 14.
Flags: needinfo?(cawang)
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

Redirecting to Gabriele. At the very least, this needs:
1. Tests.
2. The same changes to be copied over to the Emergency Call app.
Attachment #8552577 - Flags: review?(drs.bugzilla) → review?(gsvelto)
I'm going to prepare the tests, Gabriel. I don't know what is your policy here but it seems to me this needs integration tests more than unit ones. Even so, I can provide unit tests for the conversions between caret position and insert position if you want.

Regarding integration tests, do you know if there is possible to say for an input: "click before the nth character"?
Flags: needinfo?(gsvelto)
(In reply to Salvador de la Puente González [:salva] from comment #17)
> I'm going to prepare the tests, Gabriel. I don't know what is your policy
> here but it seems to me this needs integration tests more than unit ones.

I agree that integration tests would be better for this scenario.

> Regarding integration tests, do you know if there is possible to say for an
> input: "click before the nth character"?

I'm not sure about that, Johan might know more so I'm needinfo'ing him. I've come across at least one test that is explicitly setting `selectionStart' and `selectionEnd' so that might work too:

https://github.com/mozilla-b2g/gaia/blob/6318d7923760d071bd1faa5258e5244777b5d33e/tests/python/gaia-ui-tests/gaiatest/tests/functional/keyboard/test_keyboard_bug_1073870.py#L32
Flags: needinfo?(gsvelto) → needinfo?(jlorenzo)
I don't think there is a way to make a selection thanks to Marionette. Do you agree, Malini?

If that's the case, I would use the same piece of JS than Gabriele pointed out. One note though compared to it, this piece of JS seems generic enough to be placed at a higher level than the test case itself.
Flags: needinfo?(jlorenzo) → needinfo?(mdas)
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

This is looking good to me save for a few small changes I've described in the PR. I'm r- it mostly because it doesn't have tests yet (I'd f+ it but :drs did it first ;-)). With my comments addressed and the appropriate tests I'd say this should be good to land.
Attachment #8552577 - Flags: review?(gsvelto) → review-
After discussion with Carrie, we think the UX should be:
1. When tapping on the digit area, there should be a cursor, and user can move it to adjust the insertion/deletion point.
2. Long press on the digit area (when the digit area is empty) or tap the touch caret, there should be a utility bubble with Paste button ONLY.
3. User CANNOT select text.

However, after discussion with George, there seems to be some limitation for now. George, could you explain it?
Flags: needinfo?(ofeng)
Flags: needinfo?(gduan)
Flags: needinfo?(cawang)
Regarding the original items with need for UX input in comment 6:

>  1- Adding digits at the cursor position: in the same way we have a problem
> deleting characters at the cursor position, we have another problem related
> with adding more digits. Once the users alter the cursor position, new taps
> on the keys always add the digits at the end. Although I think this should
> be a different bug.

This is being addressed in bug 1126538.

>  2- Can not paste in the dialer: just that, you can select, copy but not
> paste.

With the UX clarification, now it turns out to not allow to copy but allow to paste. I usually use the dialer to take a quick numerical note. It's a pity to loose copy functionallity. Moreover when there are more problems implementing paste than implementing copy.

>  3- Can not force cursor to be shown: with the provided patch, even if we
> fix this bug, each time the delete key is tapped, the cursor dissapear. It's
> still there, and you can delete the previous character but it's not shown.
> This could led the user to confussion.

This is already solved.

We still need input about what should happen when long pressing the delete button if the cursor is not at the end of the input. See comment 14.
Flags: needinfo?(ofeng)
Hi Salvador, 

For comment 14, if users set the cursor in the middle of the digits and keep pressing the delete button, it will clear from the digit which is on the left side of the cursor to the beginning.
For example, 

1234|555
user long press the delete button
it will delete from 4 to 1

Thanks!
According to comment 22, we do think that "Copy" is quite convenient in some use cases. Our expectation is that if user long taps on the numbers and the numbers will be selected with a copy option, then user can copy the numbers to do some further edit in other APPS. 

However, we don't have API to do only "Copy" for now, if we want to provide the function here in Dialer, the option "copy" will show up with two handles and the utility bubble (copy/ paste/ cut/ select all) as in general edit field. This will make the behavior in Dialer look weird. It's like "edit" is the highest priority in Dialer and makes inputting digits become complicated. Hence, we'd suggest remove it for now to keep things simple until the API is ready. Thanks!
Flags: needinfo?(ofeng)
(In reply to Omega Feng [:Omega] [:馮於懋] (please ni?) from comment #21)
> After discussion with Carrie, we think the UX should be:
> 1. When tapping on the digit area, there should be a cursor, and user can
> move it to adjust the insertion/deletion point.
> 2. Long press on the digit area (when the digit area is empty) or tap the
> touch caret, there should be a utility bubble with Paste button ONLY.
The input field is set 'readonly', we have to remove this attribute to enable paste function and perhaps create some special attribute for system's input manager to decide whether to show the keyboard or not.
> 3. User CANNOT select text.
AFAK, there's no api or attribute to only enable cursor but disable selection (long-press) function.
> 
> However, after discussion with George, there seems to be some limitation for
> now. George, could you explain it?
Flags: needinfo?(gduan)
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

Gabriele, I'm having problems with integration tests. I don't know why, but the test "Deleting a couplle of digits..." [1] is failing and I don't understand where the problem is. Can you help me, please?

It seems the restore caret position function is not being called as the second del tap removes the last digit but it is called indeed after the first del tap. The problem is, by the moment the second tap is performed, selectionStart and selectionEnd are not properly set.

[1] https://github.com/mozilla-b2g/gaia/pull/27574/files#diff-f0cc16144fe4eb21f930543ed9412d7aR141
Attachment #8552577 - Flags: review- → review?(gsvelto)
Mmmm, a doubleTap instead of two consecutive taps fix the problem. Let me know if you feel ok with this solution. if you have a clue, let me know, please.
Duplicate of this bug: 1127659
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

This is looking good except for a test where you've added an explicit timeout. I've left a comment on the PR.
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

I finished to squish all the changes the PR is ready.
Status: NEW → ASSIGNED
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

Excellent, thanks. Remember to add r=gsvelto to the commit message before landing.
Attachment #8552577 - Flags: review?(gsvelto) → review+
master: 1b73c61286410034804146b947f4a6ef2ff6d880

Notice as, per comment 25, we cannot implement the behaviour recommended by UX in comment 21.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S5 (6feb)
Keywords: verifyme
Comment on attachment 8552577 [details] [review]
Tracking the caret position to allow deleting at the current character

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1092427
[User impact] if declined: misleading and annoying behaviour in the dialer input flow for deleting digits.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): moderate
[String changes made]: none
Attachment #8552577 - Flags: approval-gaia-v2.2?
Attachment #8552577 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
This bug verified successfully on flame 2.2
Build ID               20150208002500
Gaia Revision          e827781324cbde91d2434b388f5dead3303a85ee
Gaia Date              2015-02-06 20:54:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0552759956d3
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Refer to video VIDEO0280.mp4
Attached video VIDEO0280.mp4
This bug has been successfully verified on latest Flame v3.0.
See attachment: verified_v3.0.mp4
Reproduce rate: 0/5

Flame 3.0 build:
Build ID               20150208010208
Gaia Revision          994896b89ba10cad39a87f90eeaba9ae5e17c3a6
Gaia Date              2015-02-07 19:22:02
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/be65d1fde126
Gecko Version          38.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150208.050436
Firmware Date          Sun Feb  8 05:04:47 EST 2015
Bootloader             L1TC000118D0
Status: RESOLVED → VERIFIED
Keywords: verifyme
removing mdas' n-i
Flags: needinfo?(mdas)
See Also: → 1158727
Depends on: 1158727
Duplicate of this bug: 1159590
Duplicate of this bug: 1206513
You need to log in before you can comment on or make changes to this bug.