883129 - [MMS] Attachment is deleted when selecting a suggested word following it

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect, P1)

Description

[isabelrios]

Unagi device. Today (06/14) build:

Gecko-b300478
Gaia-0c836be

PROCEDURE
Word suggestion is on by default.
1. Open Messaging app and create a new message
2. Attach a file, the write some text, i.e. two, once it appears in the suggestion bar, tap on space key

EXPECTED
The word is correctly added to the MMS with the image attached previously

ACTUAL
When selecting a suggested word (by tapping on it or by tapping on space key) the file attached is removed.

Comment 1

[Fabien Cazenave [:kaze]]

I think the problem only occurs for the first word that is typed right after the image is inserted. If there’s a space after the attachment, everything works fine.

Investigating…

Comment 2

[Rick Waldron [:rwaldron]]

Wouldn't this be a Keyboard autocorrect issue? Messages app doesn't have anything control over spelling/autocorrect suggestions (outside of not supporting it at all)

Comment 3

[Fabien Cazenave [:kaze]]

Indeed. The suspects are `wordBeforeCursor()' and `replaceBeforeCursor' in apps/keyboard/js/imes/latin/latin.js.

Comment 4

[Fabien Cazenave [:kaze]]

David, this is probably a bug for you. Feel free to steal it. :-)

Comment 5

[Fabien Cazenave [:kaze]]

I think I found the culprit: 08a9fc142
https://github.com/mozilla-b2g/gaia/commit/08a9fc142

Comment 6

[Fabien Cazenave [:kaze]]

Attachment: link to pull request — (partial) back-out of bug 878843

Here’s a quick back-out in case we’re in a hurry to “fix” this bug. The culprit seems to be the `mozKeyboard.replaceSurroundingText' implementation (at least for the contentEditable case), but the fallback still works fine.

Comment 7

[David Flanagan [:djf]]

Yuck!

I really hope you won't land the attached patch, however, because it causes a performance regression that is serious enough that we'd probably have to disable autocorrect by default.

Instead, let's figure out what is going on. When the keyboard autocorrect code calls replaceSurroundingText, key events are not sent. Instead there is an input event (or something) I think.

Under what conditions does the messaging app switch from MMS to SMS?  How is the input event triggering that? Can't this be handled in the messaging app editor?

Yuan, I suspect that this is a messaging app bug, but since you implemented replaceSurroundingText I wonder if you have any ideas what might be happening here...

Comment 8

[Fabien Cazenave [:kaze]]

(In reply to David Flanagan [:djf] from comment #7)
> I really hope you won't land the attached patch, however, because it causes
> a performance regression that is serious enough that we'd probably have to
> disable autocorrect by default.

It’s there “just in case”. Obviously, fixing the underlying cause is better. :-)

> Under what conditions does the messaging app switch from MMS to SMS?  How is
> the input event triggering that? Can't this be handled in the messaging app
> editor?

An MMS is just an SMS with an attachment:

 • create a new SMS, select a contact
 • attach an image: the SMS becomes an MMS
 • tap in the “Compose” area, the cursor appears next to the image thumbnail
 • tap two letters and auto-complete: T, W, select “Two”

⇒ “Tw” is replaced by “Two” but the thumbnail is deleted; hence, since there’s no attachment any more, the MMS becomes an SMS.

> Yuan, I suspect that this is a messaging app bug, but since you implemented
> replaceSurroundingText I wonder if you have any ideas what might be
> happening here...

My guess is that the selection in the [contenteditable] node is a bit too wide; the <iframe> that embeds the thumbnail is selected along with the surrounding word, thus deleted when using word suggestion or auto-correction. Maybe the selection logic should make sure we’re staying within a text node?

Comment 9

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Fabien Cazenave [:kaze] from comment #8)
> 
> > Yuan, I suspect that this is a messaging app bug, but since you implemented
> > replaceSurroundingText I wonder if you have any ideas what might be
> > happening here...
> 
> My guess is that the selection in the [contenteditable] node is a bit too
> wide; the <iframe> that embeds the thumbnail is selected along with the
> surrounding word, thus deleted when using word suggestion or
> auto-correction. Maybe the selection logic should make sure we’re staying
> within a text node?

Your guess is right. I'll make a patch to replaceSurroundingText to limit the selection range.

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Limits the selection range of the mozKeyboard.replaceSurroudingText.

When [contenteditable] node is being edited, the patch limits the selection range for mozKeyboard.replaceSurroudingText. It won't replace the non-text node before the selection range.

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

David, I think you are capable of reviewing the patch, as there is no gecko specific code. Please help to review the patch if possible. Thanks:)

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Limits the selection range of the mozKeyboard.replaceSurroudingText

A small fix for the patch.

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Limits the selection range of the mozKeyboard.replaceSurroudingText

Sorry for bothering, but |selection.modify("extend", "backword", "character")| is buggy. Sometimes if there is an image before the cursor, we can't move cursor backward by |selection.modify|.

To make a workaround, we always use |selection.modify("extend", "forward", "character")| to move the cursor.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 763094 [details] [diff] [review]
Limits the selection range of the mozKeyboard.replaceSurroudingText

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

r- because I don't understand the patch well enough to evaluate it, and because I've noticed other related autocorrect problems with MMS messages.

Some general comments on the problem of finding the right cursor position when the contenteditable element has more than one text node child would be helpful.  

And a specific comment explaining this particular patch seems necessary. This patch seems to imply that there is more than one possible start position within an element that corresponds to a given position in the textContent of that element. Is there any way to model the mapping from HTML document to plain text that avoids this ambiguity? Could you modify getContentEditableSelectionStart(), for example, so that it was not necessary to do the forward and backward trick in setSelectionRange()?

Basically, the code doesn't explain the problem well enough for me to evaluate the solution. I don't know how to think this through enough to know if the proposed patch is a fully general solution or if there are other cases of mixed text and images that will cause problems.

I had no idea that the MMS app was using a contenteditable element for MMS messages and was allowing images to be embedded directly within the text. I fear that we're going to have a lot of other problem with that. For example:

1) If I type "t", then attach an image, and then tap after the image and type 'h', the keyboard just sees 'th' (with no image) and suggests the word 'the'.  (If I type space, it correctly inserts and e and does not erase the image, which really surprises me.  Maybe I'm using an out-of-date build that is not using replaceSurroundingText)

2) If I now backspace twice, I delete the 'h' and the image.  For some reason the keyboard goes away, which seems like a bug. But if it didn't go away, then we'd be left with the situation where the keyboard thought I had deleted the 't', but I hadn't. If the user typed an 'h' at this point, they'd expect to see 'the' suggested, but they wouldn't get it because the keyboard would only be aware of the 'h'.

When a content editable element contains non-textual elements that are deletable and serve as word delimiters, perhaps you could convert them to a tab or linefeed so that they behave like word breaks.  (Don't use space because we treat that specially for punctuation.)  Maybe some more exotic Unicode character would be better, but that might require a matching change in keyboard/js/imes/latin/latin.js

::: b2g/chrome/content/forms.js
@@ +766,5 @@
> +    if (start < length) {
> +      // try to skip non-text node
> +      while (getContentEditableSelectionStart(element, sel) <= start) {
> +        sel.modify("move", "forward", "character");
> +      }

It confuses me to have two loops that are so similar here. Could you put the first while loop above in an else clause instead? That is, could you have one loop for the start < length case and one for the start===length case?

Comment 15

[David Flanagan [:djf]]

Comment on attachment 762934 [details]
link to pull request — (partial) back-out of bug 878843

r- on this one, too. Based on my investigation for the comment above, I'm almost positive that even if you backed out the replaceSurroundingText() code you'd still have serious problems.

Type 'r', insert an image, tap again to bring the keyboard back, and then type 'he'.  The keyboard doesn't know about the image, so it thinks you've typed 'rhe'. If you now type space, it auto corrects to 'the'.  Without replaceSurroundingText() it does this by sending 3 backspaces, which delete the e, the h, and the image.

Comment 16

[David Flanagan [:djf]]

Yuan, I suspect you should take this bug from Kaze, right?

Comment 17

[Yuan Xulei [:yxl] (Leave from May 18)]

Take bug from Kaze. 

David, the bug seems more complicated than expected. I agree with you that 

"""
When a content editable element contains non-textual elements that are deletable and serve as word delimiters, perhaps you could convert them to a tab or linefeed so that they behave like word breaks.  (Don't use space because we treat that specially for punctuation.) Maybe some more exotic Unicode character would be better, but that might require a matching change in keyboard/js/imes/latin/latin.js
"""

I prefer converting non-textual elements to exotic characters, in case keyboard would like to differentiate non-textual elements from word delimiters in the future. How about the "substitute character (␚) is a control character that is used in the place of a character that is recognized to be invalid or in error or that cannot be represented on a given device"?  This character is encoded by the number 26 (1A hex). See http://en.wikipedia.org/wiki/Substitute_character.

Comment 18

[Wayne Chang [:wchang]]

:yxl - are you still working on this?

Comment 19

[Yuan Xulei [:yxl] (Leave from May 18)]

Yes, still working on it. It takes me lots of time and the patch will be ready soon.

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Converts non-textual element to placeholder for mozKeyboard

The patch changes the flags of the plaintext encoder:

1. OutputDropInvisibleBreak is removed. If it is set, <br> inside the contentEditable won't be converted to '\n'.

2. OutputNonTextContentAsPlaceholder is added to convert non-textual element to the placeholder character of SUB('\x1A').

Comment 21

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 771904 [details] [diff] [review]
Converts non-textual element to placeholder for mozKeyboard

rs=hsivonen, but in principle, this really should be reviewed by a module owner for the Gaia keyboard.

Comment 22

[Yuan Xulei [:yxl] (Leave from May 18)]

Henri, thanks. cc the gaia keybaord module owner Rudy to let him know the coming change.
We are going to use '\uFFFC'(OBJECT REPLACEMENT CHARACTER) as the placeholder character to represent the non-textual content in contentEditable field.

Comment 24

[Leo]

Hi Yuan,

I applied your patch in gecko but I m still able to reproduce this issue.
Do this issue needs changes in Gaia too.
Please give your comment

Thank you

Comment 25

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi Leo, 

Please also apply the patch of the depending Bug 890580, which is under review.
The two gecko patches will resolve the issue.

After these patches applied, we need some changes in Gaia to resolve a side effect and make word suggestion and auto-correction work properly. I think David would provide the patch for the Gaia changes.

Comment 26

[Leo]

Thanks Yuan,
On applying the gecko changes, I think we need to change 
some code in Gaia also, In Gaia/keyboard/imes/latin, I assume
for suggestion and auto correction.
Can you please check and assign David or other Keyboard Engineer for the issue,
so that the issue get solved soon since this is a major issue.

Thank you.

Comment 27

[Yuan Xulei [:yxl] (Leave from May 18)]

David, '\uFFFC'(OBJECT REPLACEMENT CHARACTER) is used to represent the non-textual content in contentEditable field. Does it require a matching change in keyboard/js/imes/latin/latin.js?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/e9eb622a23be

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d1f19de81311

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/d1f19de81311

Comment 32

[Leo]

Hi,
I can still reproduce this issue even by applying patches of both 883129 and 890580.
Following is the procedure to reproduce
1. New SMS > Attach a image
2. Type Some letter and then select suggestion
   Example - type "Rec" and select "Recent" from suggestion

             OR
1. New SMS >> type letters,say , "Re"
2. Attch a image
3. Again type "ce" and select "Recent" from suggestion.
 
     The image get deleted and MMS is converted to SMS in both above cases


The only way which I cannot reproduce this issue is if 
-- type "Rece"
-- Attach a image
-- does not type anything and select "Recent" from suggestion.
 The image retains this time.

Please check this issue and if you can reproduce this same or it needs some
Gaia modification.

Comment 33

[Yuan Xulei [:yxl] (Leave from May 18)]

Yes, I can still reproduce the bug. We need polish on Bug 890580 and I filed Bug 895239.

Comment 35

[David Flanagan [:djf]]

Yuan,

I've been away on vacation. Please help me understand what is needed here. Do we just need to change Gaia to treat \uFFFC as a word separator character, or is there still other work pending?

Comment 36

[Yuan Xulei [:yxl] (Leave from May 18)]

David, you're right, just change Gaia to treat \uFFFC as a word separator character.

Comment 38

[Jason Smith [:jsmith]]

Yuan - Can you file a followup bug for this?

Comment 39

[Yuan Xulei [:yxl] (Leave from May 18)]

The followup bug 902352 was create to track the required change of gaia.

Comment 40

[buri.blff]

Dears,
I can still produce this pr on build 20130806071254.
Brs,
TIAN Min

Comment 41

[Yuan Xulei [:yxl] (Leave from May 18)]

Please try a newer build. The bug 895239 it depends was fixed on Aug 06 and it is very likely that your build doesn't have that patch.