Closed Bug 1043682 Opened 10 years ago Closed 10 years ago

Caret positioning glitch after deleting a saved draft

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: khuey, Assigned: lchang)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Attached image 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.
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.
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.
Depends on: 904846
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
Attached file 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.
Attachment #8489303 - Flags: review?(schung)
Comment on attachment 8489303 [details] [review]
Pull Request 24043

Redirecting the review to Oleg since Steve is away during this week.
Attachment #8489303 - Flags: review?(schung) → review?(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!
Attachment #8489303 - Flags: review?(azasypkin)
Assignee: nobody → lchang
Status: NEW → ASSIGNED
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.
Attachment #8489303 - Flags: review?(azasypkin)
Comment on attachment 8489303 [details] [review]
Pull Request 24043

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

Thanks for fixing this!
Attachment #8489303 - Flags: review?(azasypkin) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Oleg, can you please ask for a 2.1 approval here? Does it also make sense to request a 2.0 approval as well?
Flags: needinfo?(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
Attachment #8489303 - Flags: approval-gaia-v2.1?
Flags: needinfo?(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 on attachment 8489303 [details] [review]
Pull Request 24043

Not bad enough to uplift at this point.
Attachment #8489303 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1-
Blocks: 959201
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: