1207086 - Cursor shows up in wrong position when user taps newline button.

Status: RESOLVED WONTFIX

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

Description

[Autumn.Qiu(Leave from Mozilla)]

Attachment: logcat_mess.txt

[1.Description]:
[Aries KK_v2.5][Flame KK v2.2&2.5]Create a MMS with several letters and a picture, tap the end of existing letters, then tap newline button, another line does not appear and the cursor moves up in wrong position, which moving at the beginning of the existing line or locating below the picture.

Found at: 15:29
See attachment:logcat_mess.txt & Aries_KK v2.5_mess.3gp.

[2.Testing Steps]: 
1.Launch Messages.
2.Tap "+" icon.
3.Type several letters.
4. Add a picture from Gallery or Camera.
5.Tap the end of those letters.
6.Tap newline button and observe.

[3.Expected Result]: 
In step 6, another line should appear above the picture and cursor shows up in the begining of the new line.

[4.Actual Result]: 
In step 6, another line does not appear and the cursor shows up in wrong position, at the beginning of the existing line or locating below the picture.

[5.Reproduction build]: 
Device: Arise KK 2.5[Affected]
Build ID               20150921174442
Gaia Revision          29991414eb94b6baa1ec2e63fdb4f6dfae05fb01
Gaia Date              2015-09-21 09:27:10
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/197af2fb7e29ff8e4b3b6ced723b6172e954e17d
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150921.170449
Firmware Date          Mon Sep 21 17:04:57 UTC 2015
Bootloader             s1

Device: Flame KK 2.5[Affected]
Build ID               20150921073455
Gaia Revision          2d370fa35c1a0ee2a637e3772c0843586a5f96c9
Gaia Date              2015-09-21 02:41:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/039a8490891595736b16a3ccb17f025f4dcf13eb
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150921.112037
Firmware Date          Mon Sep 21 11:20:52 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame 2.2[Affected]
Build ID               20150921032501
Gaia Revision          95950c9d48ad2fc2da7686c2b133b750a99cd3da
Gaia Date              2015-09-18 09:55:13
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9644c82a5b88
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150921.070345
Firmware Date          Mon Sep 21 07:03:54 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0


[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Autumn.Qiu(Leave from Mozilla)]

Attachment: Aries_KK v2.5_mess.3gp

Comment 2

[Norry.L.F(Leave from Mozilla)]

Hi Gerry,

Could you help to dispatch this bug? thanks.

Comment 3

[Gerry Chang [:gchang]]

Hi Fred,
Can you help to dispatch this?

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

It looks like the SMS message body does something extra to handle the attachment and the text.

Comment 5

[Julien Wajsberg [:julienw]]

I can reproduce in Firefox, I think we can come up with a simpler testcase for the contenteditable developers.

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: testcase

Actually I can't reproduce fully the first behavior but I still see one weird thing happening.

Here is a testcase that you can try in Firefox. The green thing is an iframe.

Here is the STR for this testcase.
1. Open the attachment.
2. Put the cursor (with the mouse or keyboard) just after "some text"
3. Press "Enter".
=> Notice the cursor is just before the iframe. But we expect to see a new line.
4. Enter some text.
=> The new line is actually added now, and we see the text in this new line.

Of course I understand what the issue is here: because the "iframe" is in "display: block" it's on its own line. When we press "enter" we insert a "br", but the "br" has no effect.

Some fixes could be done on the gaia side: if we make sure we always have a "br" before the iframe, it should behave correctly. We'll also need to make sure we do not convert this "br" to a new line when we get the value back...

But first I'd like to NI the contenteditable wizards to see if they think the gecko behavior is correct. What do you think, Ehsan, Masayuki ?

Comment 7

[(no longer active)]

It's clear that this is a Gecko bug.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Chrome inserts <div> element and <br> element at pressing Enter and inserting text at the start of the <div>.

Edge behaves similar, but not append <br> and wraps the "some text" with another <div>.

There would be a web standard defining this behavior, though...

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I guess that we should insert additional <br> element if following content is block or something?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: If "Some text" is in <p>, Enter key press creates new <p> element. That might hide this bug in Gaia side.

Comment 11

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> I guess that we should insert additional <br> element if following content
> is block or something?

Yeah that would be the right fix.  (Note how pressing Enter twice gives you what you should have gotten by pressing Enter once.)

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Treat iframe as a block element. (v1)

We had already inserted two <br> elements if the content after the caret
is a block element. However, <iframe> does not treated as a block
element in nsHTMLEditor::NodeIsBlockStatic(). This patch make it so.

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8672284 [details] [diff] [review]
Treat iframe as a block element. (v1)

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

Hi Ehsan,

I understand you are not reviewing patch this moment, but I cannot find other editor peer in this list [1]. Would you help review this change?

[1] https://wiki.mozilla.org/Modules/Core

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8672284 [details] [diff] [review]
Treat iframe as a block element. (v1)

I don't think that this is right approach since iframe's default style is replaced inline. The problem here is, we don't check the actual style of the following element.

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Determine whether a node is block element from its style context. (v2)

We had already inserted two <br> elements if the content after the caret
is a block element. However, we do not really look into the content's
style context to determine whether it's really a block element or not.

This patch added the code to look up the element's style context. If
there's none, it will fall back to look up the tag as before.

Comment 16

[(no longer active)]

Please don't change the behavior of NodeIsBlockStatic().  That's used all over the place in the editor code.  If you need to look at the elements' styles, please do so only for this specific case.

Comment 17

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I agree changing NodeIsBlockStatic is risky. However, in this specific case, nsHTMLEditor::IsVisBreak() will be used in [1] to determine whether to insert extra <br>. It uses utilities like GetPriorHTMLNode() which can accepts aNoBlockCrossing=true to decide not to cross a block. The result node will be wrong if the prior node is a <span style="display: block"> or <div style="display: inline">.

I haven't come up a good solution yet.

[1] https://dxr.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709/editor/libeditor/nsHTMLEditRules.cpp#7564

Comment 18

[(no longer active)]

Can we just do this in IsVisBreak()?

Comment 19

[Isabel Rios[:isabel_rios]]

Hi, this issue is still seen in master.
Nom to 2.6. Hopefully seeing your comments there is a chance to get it fixed there in the future.

Comment 20

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 21

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(