1043682 - Caret positioning glitch after deleting a saved draft

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 2014-07-24-16-09-39.png

STR

1. Type an SMS to someone.
2. Go back to the main SMS screen and save the draft message.
3. Click on the contact again.
4. Delete the entire draft message by typing backspace.

Results:
The caret is now positioned weirdly (substantially above the text "Message")

Expected result:
The caret is positioned normally.

Comment 1

[Julien Wajsberg [:julienw]]

Thanks for filing !

I've tried to debug this in the past and then gave up. I also couldn't reproduce in a simpler testcase and so haven't filed a separate bug.

Comment 2

[Oleg Zasypkin [:azasypkin]]

Looks like current behavior is a Core bug 904846.

As workaround we probably can use autogenerated placeholder "Message" with element other than contenteditable message input.

Comment 4

[Luke Chang [:lchang] (inactive)]

After investigating, I found another root cause besides bug 904846. That is, Gecko always adds a "<br>" at the end of a content-editable element. When we try to focus the compose field with moving the cursor to the end [1], the cursor will be located behind the "<br>" abnormally. Hence, the "<br>" will be deleted by backspace key when we attempt to clear the draft message.

Actually, this root cause will also introduce another symptom. The STR is as follows:

1. launch messages app and create a new message.
2. input some characters in the message input field.
3. click "back" button on the upper-left corner and choose "Save as Draft".
4. click the draft message that we just added to reenter editing mode.
   (by default, the input field is focused and keyboard is displayed.)
5. click any key to input a character in the input field.
6. then, you'll see that the new character will create a new line.

In the step 6, the new character is supposed to follow the last character of the message in the compose field and seems not to be fixed by bug 904846. So, I'm working on a patch for this and will attach it to here later.


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/compose.js#L501

Comment 5

[Luke Chang [:lchang] (inactive)]

WIP patch: https://github.com/luke-chang/gaia/commit/98359373c2287a0cc19f276bf6f6790ab3c15015

Comment 6

[Luke Chang [:lchang] (inactive)]

Attachment: Pull Request 24043

Hi Steve,

This patch can resolve the issue I mentioned in comment 4 as well. Could you please help reviewing it? Thanks.

In this patch, however, I have no idea how to write a unit test to verify the cursor position.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8489303 [details] [review]
Pull Request 24043

Redirecting the review to Oleg since Steve is away during this week.

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8489303 [details] [review]
Pull Request 24043

Hey Luke,

I've left two suggestions on github, one that can help to eliminate two unnecessary DOM operations and one about unit-tests.

Thanks!

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8489303 [details] [review]
Pull Request 24043

Hi Oleg,

Thanks for your suggestions. They're much helpful.

I've updated my patch and modified the unit test accordingly. Would you mind reviewing it again? Thanks.

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8489303 [details] [review]
Pull Request 24043

Looks good now, please r=me in the commit message.

Thanks for fixing this!

Comment 11

[Luke Chang [:lchang] (inactive)]

Oleg, Thanks for reviewing.


Gaia-try passed: https://tbpl.mozilla.org/?rev=7ca08a976befffcd66ca36bae640c67195cec85a&tree=Gaia-Try

landed on master: https://github.com/mozilla-b2g/gaia/commit/0a919528165622c3e12947d6363c3ab5b111c37e

Comment 12

[Julien Wajsberg [:julienw]]

Oleg, can you please ask for a 2.1 approval here? Does it also make sense to request a 2.0 approval as well?

Comment 13

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8489303 [details] [review]
Pull Request 24043

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 959201
[User impact] if declined: When user reopens draft there will be 2 issues: 
  1. if user tries to remove entire draft content with backspace, caret will be put into abnormal position;
  2. if user continues to edit message caret and newly entered text will be always moved to the next line.
[Testing completed]: yes, unit test
[Risk to taking this patch] (and alternatives if risky): relatively low
[String changes made]: n/a

Comment 14

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> Oleg, can you please ask for a 2.1 approval here? Does it also make sense to
> request a 2.0 approval as well?

I believe v2.0 doesn't have such problem as bug 959201 is not uplifted to v2.0, so when user reopens draft in v2.0, cursor will be put at the message input beginning

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8489303 [details] [review]
Pull Request 24043

Not bad enough to uplift at this point.