902847 - [zffos1.1][input method] replaceSurroundingText() may output the wrong result when it mutates a contenteditable element

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[zhangdapeng]

Attachment: 13080748522013-08-06-08-38-23.png

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; TCO_20130808162156)

Steps to reproduce:

in Spanish , enter sms application, edit the message content, enter the word, press the Enter key and then press the spacebar to enter some letters, then select the first letter of the word， 



Expected results:

Automaticly add a letter

Comment 1

[zhangdapeng]

(In reply to zhangdapeng from comment #0)
> Created attachment 787401 [details]
13080748522013-08-06-08-38-23.png

User
> Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET
> CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729;
> TCO_20130808162156)

Steps to reproduce:

in Spanish , enter sms
> application, edit the message content, enter the word, press the Enter key
> and then press the spacebar to enter some letters, then select the first
> letter of the word， 



results:

Automaticly add a letter，it is wrong

Comment 2

[Scoobidiver (away)]

leo+ requested per bug 899451.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get more attention on this and find out if it's consistently reproducible?

Comment 4

[dkumar]

I was not able to reproduce this issue on Leo Moz and Com Ril,also tried to flash the device on 08/08 Moz and Com Ril was unable to reproduce this issue.

Environmental Variables
Build ID: 20130812041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/36bbc5943448
Gaia: c60e3507d9df160e006d2e25967d26df2dc543cb
Platform Version: 18.1

Is it possible to get more information as to how to reproduce this issue?(Maybe a video)

Comment 5

[zhangdapeng]

Attachment: 1308074839输入字符异常.zip

it‘s the log correspond to this problem

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Alive - is this something you could take a look at (log in comment 5)?  It looks like this might be a POVB issue.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Tim, if Alive isn't able to help out here can you find someone who can look at this log and confirm POVB?

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

transfer to rudy :)

Comment 9

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I cannot reproduce this issue with the latest revision of Gaia v1-train,
0f1f1ab0ab31a1df8a780baa048b5e7b2854205d

Can someone on this bug provide repro steps in more detail?
Thank you.

Comment 10

[Andrew Overholt [:overholt]]

Please provide an STR that can be followed to reproduce this issue.  Since 2 people have tried and cannot reproduce this, leo-.

Comment 11

[zhangdapeng]

(In reply to Andrew Overholt [:overholt] from comment #10)
> Please provide an STR that can be followed to reproduce this issue.  Since 2
> people have tried and cannot reproduce this, leo-.

see the attachment screenshots.7z
it has 5 photos
prerequisite： switch language to Spanish，enable auto correction and word suggestion(see 1.png ,2.png)
step 1: in sms content text area,enter some character,generate a word,then click the enter key to the next line(see 3.png)
step 2: enter some character(such as hi)(see 4.png),then click the space key,you will see a additional character(see 5.png)

Comment 12

[zhangdapeng]

Attachment: screenshots.7z

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to zhangdapeng from comment #11)
> (In reply to Andrew Overholt [:overholt] from comment #10)
> > Please provide an STR that can be followed to reproduce this issue.  Since 2
> > people have tried and cannot reproduce this, leo-.
> 
> see the attachment screenshots.7z
> it has 5 photos
> prerequisite： switch language to Spanish，enable auto correction and word
^^^^^^^^
> suggestion(see 1.png ,2.png)
> step 1: in sms content text area,enter some character,generate a word,then
> click the enter key to the next line(see 3.png)
> step 2: enter some character(such as hi)(see 4.png),then click the space
> key,you will see a additional character(see 5.png)

It's just word suggestion(auto correction) corrects your word when you type space key.

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

zhangdapeng, thanks for the tip to reproduce this issue.

Here is the exact sequence that I used to reproduce this issue.
1. In SMS message input field, input "fue".
   1.a then press [space] key, this is the key step, without this, this issue would not occur.
   1.b Then press [enter].

2. Input "hi" and then [space] key.
=> Can be reproduced.

It seems that this is not a Spanish-specific issue, you can reproduce this same issue if your try the same steps (1.a is important) with English.

Comment 15

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

After further checking, this issue could be reproduced with contenteditable but not with a textarea element.

The logic in Gaia keyboard latin IME is the same, but the result is different, so I suspect there is something wrong in Gecko mozKeyboard or forms.js?

Yuan,

Could you please help investigate this issue from Gecko side?
Thank you.

Comment 16

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

It is a bug of mozKeyboard.replaceSurroudingText with contentEditable. The API replaces wrong word if there is a new line character before the word.

Comment 17

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

Jan, please feel free to take the bug if you have time.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

This is a problem with the way we pass info into `replaceSurroundingText`. The new text is `suggestion\r` (inserted from latin.js, the enter key is bound to CR), now in forms.js we pass the new string into `editor.insertText`. All this works nice for textarea's etc. (gets converted into proper newline), but for contenteditable the state will be completely off. I have no clue where to look regarding contenteditable behavior however...

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Workaround for B2G forms.js

Here's a quick workaround. It's not pretty but it gets around the problem... A problem that now shows up is that after you let a word be autocorrected by pressing ENTER in in contenteditable that the backspace doesn't delete the newline but that's intended because it should undo the autocorrect and not remove the space/anything, but it looks crazy...

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Tests

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

+ some test cases, so we can also reproduce it.

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Verified problem on v1-train. It's not possible to write multiline SMS messages if autocorrect is enabled.

Comment 23

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

Comment on attachment 793518 [details] [diff] [review]
Workaround for B2G forms.js

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

Hi Jan, 

We always use '\n'(new line) as line break when retrieving text from input field for the Keyboard API. To be consistent, when sending text to input field, we should use '\n' as well. So I suggest to replace all '\r'(carriage return) characters by '\n' regardless of the input field type.

::: b2g/chrome/content/forms.js
@@ +1004,5 @@
>  
> +  // Passing CRs in contenteditable bugs,
> +  // see https://bugzilla.mozilla.org/show_bug.cgi?id=902847
> +  if (isContentEditable(element)) {
> +    text = text.replace(/

You can use this instead:
  text = text.replace('\r', '\n');

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Do you mean in Gecko or in Gaia layer? Could easily fix it there but we need to make sure that 3rd parties will do this as well...

Comment 25

[Ivan Tsay (:ITsay)]

Partner think this is a blocker because it could affect the any application that allow inputting the message such as sms. Need to fix it.

Comment 26

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Yuan Xulei [:yxl] from comment #23)
> You can use this instead:
>   text = text.replace('\r', '\n');
No, that does not do a global replace.

Comment 27

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Workaround for B2G forms.js v2

Comment 28

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

Comment on attachment 793918 [details] [diff] [review]
Workaround for B2G forms.js v2

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

r=me if the comment is addressed.

::: b2g/chrome/content/forms.js
@@ +1004,5 @@
>  
>    if (text) {
> +    // We don't use CR but LF
> +    // see https://bugzilla.mozilla.org/show_bug.cgi?id=902847
> +    text = text.replace(/

Please use \n and \r to represent new line and carriage return characters respectively. It isn't straightforward here and may be error prone as some windows text editors will convert \n to \r\n when we edit the source code file.

Comment 29

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

Comment on attachment 793520 [details] [review]
Tests

Rudy, please help to review these tests.:)

Comment 30

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Yuan Xulei [:yxl] from comment #28)
> Please use \n and \r to represent new line and carriage return characters
> respectively. It isn't straightforward here and may be error prone as some
> windows text editors will convert \n to \r\n when we edit the source code
> file.

Oh wow, wtf is this. In my editor it looks all nice, but the diff makes a complete mess out of it...

Comment 31

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Workaround for B2G forms.js v3

Alright, my diff generation script was buggy. This will do.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0a8fb7ea378b

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a8fb7ea378b

Should this have a test?

Comment 34

[Jan Jongboom [:janjongboom] (inactive)]

Yes, but the tests live in Gaia. (see other attachment)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Great, thanks!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/54d3febd5d49

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/54d3febd5d49

Comment 37

[Jeni]

Issue still repros on Leo Moz Ril

Build ID: 20130828041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/c3b4683d1360
Gaia: 518178ca234c98eb1ca0c0997b9d59faea640a85
Platform Version: 18.1
RIL Version: 01.01.00.019.204 

and on Com Ril

Build ID: 20130828041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/c3b4683d1360
Gaia: 518178ca234c98eb1ca0c0997b9d59faea640a85
Platform Version: 18.1
RIL Version: 01.01.00.019.205 

Leo device adds a letter incorrectly when following steps in comment 14

Comment 38

[Jeni]

Do you want me to open a new bug or can this one be reopened?

Comment 39

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 793520 [details] [review]
Tests

Sorry that I don't understand why there will be an extra '\n' at the end of the result.

Can you help me figure it out or we need to ping someone familiar with that part of code?

Thank you.

Comment 40

[Jan Jongboom [:janjongboom] (inactive)]

Reproduced on english locale as well with
T R Y SPACE ENTER J A SPACE

Comment 41

[Jan Jongboom [:janjongboom] (inactive)]

So the problem still persists when not going the replaceSurroundingText route, but sending CR's (charcode 13) via sendKeys to a contenteditable. F.e.:

sendKeys: T R Y SPACE CR J A

Now set selection range from 4 to 6. The A is not selected.

I've looked around in the nsEditor.cpp code but we'll need someone who actually knows this stuff...

Comment 43

[David Flanagan [:djf]]

If I add debugging comments to the keyboard tests in the uitest app, I notice that for content editable elements, if I log elt.textContent on every input event, the textcontent never includes newlines.  If i look at the innerHTML of the element, I see <br> elements for newlines.  But if I look at just textContent, the newlines just are not there.

I'm not sure that actually has anything to do with this bug, since I don't know anything about nsIPlaintextEditor

Comment 44

[David Flanagan [:djf]]

I've tried working around this in sendKey() at keyboard.js:1622 by changing the way we handle the enter key.

Instead of sending \r, I've tried sending \n and \r\n, but neither of those work.
I've also tried changing sendKey to use replaceSurroundingText() to handle the enter key, so that it gets the \r to \n patch that is in forms.js, but that doesn't help either.

Comment 45

[David Flanagan [:djf]]

Now I'm wondering if this has anything to do with the css white-space property.  The default seems to be to collapse sequences of whitespace, which might explain why space enter is being treated as a single character instead of two characters.

I've tried setting white-space: pre-wrap on a content editable element. That doesn't fix the bug.

But I notice that we can produce a similar bug without using the enter key at all.

Type 'foo space space backspace (to undo the auto-punctuation) space space ba space'

That should give foo plus four spaces plus bar. But when the auto-correct happens, the four spaces are condensed into one.  At this point the latin im and the underlying element are out of sync.  If you type seven backspaces, you'll just see an F on the screen, but the latin im thinks that the element contains "Foo" so it will offer suggestions like "food".

So my working hypothesis is now that content editable elements automatically do the right thing with whitespace but that the nsIPlaintextEditor we're using in forms.js is collapsing whitespace.

I wonder if there is an option to change that?

Comment 46

[David Flanagan [:djf]]

Attachment: partial gaia-based workaround

Comment 47

[David Flanagan [:djf]]

I've tried playing around with editor options and doing things like removing the deleteSelection() call since it happens automatically when text is inserted.  I've tried deleting one character at a time.  None of that makes a difference.  So I agree with Jan: there is a bug in the plaintext editor and we need someone who knows it deeply to help us here.

Meanwhile, let's think about workarounds.

If there is a bug when we have whitespace before newlines we could modify the latin IM to delete trailing whitespace before sending a newline. Arguably, that would be a feature... we wouldn't waste text message characters on unnecessary spaces.

If we can do that, it might be good enough for a temporary workaround for 1.1. I don't have a lot of time, so I'd prefer someone else to do that, and ask for my review.  But if needed, you can assign this bug to me for that workaround.

I've just attached a partial workaround. It changes the call to replaceSurroundingText to make it more efficient. It doesn't solve the problem, but minimizes it so that the bug only occurs when the autocorrection changes the first letter of the word on the new line.  So autocorrecting 'ba' to 'bar' would work fine, but changing 'yo' to 'to' would still cause the bug.

Comment 48

[David Flanagan [:djf]]

I just noticed that this bug had been re-opened but the tracking flags said "fixed".

Setting needinfo for tim and yuan to get this bug on their radar.  Yuan, if you think you can fix this in forms.js, that would be best. But if not, the workaround I describe above to strip trailing whitespace might work.

Tim: would you make sure this bug gets assigned to someone?  (To me, if necessary for the workaround)

Comment 49

[David Flanagan [:djf]]

I can't stop working on this bug...

I tried adding this code to the editor observer: 

    // If there is no selection and the cursor is not in a text node
    // it probably means that the user just started a new line.  When
    // this happens, we re-activate the keyboard because
    // contenteditable elements handle spaces before newlines weirdly
    // and the input method can't stay in sync with us.
    // See bug 902847.
    var sel = this.focusedElement.ownerDocument.defaultView.getSelection();
    if (sel.isCollapsed && sel.anchorNode.nodeType !== 3) {
      this.sendKeyboardState(this.focusedElement);
      return;
    }

That had the desired effect of keeping the internal IM state in sync with the content of the text editor (that is, the one trailing space was removed) but it didn't fix the bug with the newline going away.

Comment 50

[David Flanagan [:djf]]

The content editable element is doing something sort of magical with trailing spaces.  If I type a word, then a space, then a newline, the space disappears. But if I backspace that removes the newline and re-inserts the space.  I think maybe the problem is that the nsIplaintextEditor doesn't know about this magical behavior.  I wonder if the problem is that getSelectionRange() for content editable elements returns the wrong cursor position when replaceSurroundingText() is called because the editor object is out of sync with what the element is actually displaying.

Comment 51

[David Flanagan [:djf]]

Taking this myself to try the gaia workaround I proposed above.

Comment 52

[David Flanagan [:djf]]

Attachment: link to patch on github

Here's my Gaia based patch.  It includes the optimization for calls to replaceSurroundingText described earlier and also removes trailing whitespace before the cursor before sending the RETURN key.

This seems to resolve the bug reported here.  It makes me nervous, though, and I'd like help ensuring that it is tested thoroughly before we land it.

Jan, Yuan: does the code look right to you? Can you think of any ways that removing trailing whitespace would break apps?

I'd prefer a patch that solved the more fundamental issue of the mismatch between the IM, the nsIEditor object, and the content of the contenteditable element, but that seems like a really hard problem. So if this seems like a reasonable workaround, it may be the best we can do for now.

Comment 53

[Jan Jongboom [:janjongboom] (inactive)]

Couple of things:

1. With the new inputmethod API we should not keep state in the keyboard itself, but rely on the events coming from inputmethod (which always give us correct state).
2. The problem with fixing it in Gaia is that this is a problem that third party keyboards will also face, and they should include the same type of workarounds which doesn't seem viable.
3. Code from line 351 feels flaky. If I want to type something with spaces at the end, this patch will prevent me from submitting that data in a web page.

Comment 54

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

cc Henry as he knows much more about the plaintext serializer.

Comment 55

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

Comment on attachment 797728 [details]
link to patch on github

It looks good to me as a workaround. I'm trying to resolve it from gecko side, but it needs some time...

Comment 56

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

Thanks for digging into this.
(In reply to David Flanagan [:djf] from comment #50)
> The content editable element is doing something sort of magical with
> trailing spaces.  If I type a word, then a space, then a newline, the space
> disappears. But if I backspace that removes the newline and re-inserts the
> space.  I think maybe the problem is that the nsIplaintextEditor doesn't
> know about this magical behavior.  I wonder if the problem is that
> getSelectionRange() for content editable elements returns the wrong cursor
> position when replaceSurroundingText() is called because the editor object
> is out of sync with what the element is actually displaying.

It is quite likely the root cause. Let me look into it;-)

Comment 57

[Henri Sivonen (:hsivonen)]

(In reply to David Flanagan [:djf] from comment #43)
> if I log elt.textContent on every
> input event, the textcontent never includes newlines.

This is expected. textContent  returns a concatenation of the descendent text nodes. Unlike IE's innerText, it ignores <br>.

Comment 58

[David Flanagan [:djf]]

(In reply to Jan Jongboom [:janjongboom] from comment #53)
> Couple of things:
> 
> 1. With the new inputmethod API we should not keep state in the keyboard
> itself, but rely on the events coming from inputmethod (which always give us
> correct state).

I haven't even looked at that API yet. I agree, however. But that doesn't help us for 1.1.  Since this is marked as a 1.1 blocker, I'd like to try to fix it for that release.


> 2. The problem with fixing it in Gaia is that this is a problem that third
> party keyboards will also face, and they should include the same type of
> workarounds which doesn't seem viable.

Again, this doesn't apply to 1.1

> 3. Code from line 351 feels flaky. If I want to type something with spaces
> at the end, this patch will prevent me from submitting that data in a web
> page.

Can you think of a concrete example where trailing whitespace is significant and you actually do want to submit it?  Before being exposed to the "no trailing whitespace" rigor of gjslint, I would have worried about this, too, but I've gotten used to it and can see that there is a good argument for discarding it always.

Note that even without my patch, Gecko is already turning "space newline" into "newline", so one trailing whitespace is automatically being removed, and none of us knew it (because we never care about trailing whitespace).  Admittedly, Gecko only removes one trailing whitespace and leaves the others, so this workaround is more aggressive than Gecko.

Comment 59

[David Flanagan [:djf]]

(In reply to Yuan Xulei [:yxl] from comment #56)
> Thanks for digging into this.
> (In reply to David Flanagan [:djf] from comment #50)
> > The content editable element is doing something sort of magical with
> > trailing spaces.  If I type a word, then a space, then a newline, the space
> > disappears. But if I backspace that removes the newline and re-inserts the
> > space.  I think maybe the problem is that the nsIplaintextEditor doesn't
> > know about this magical behavior.  I wonder if the problem is that
> > getSelectionRange() for content editable elements returns the wrong cursor
> > position when replaceSurroundingText() is called because the editor object
> > is out of sync with what the element is actually displaying.
> 
> It is quite likely the root cause. Let me look into it;-)

Thanks, Yuan. Do you want to take the bug and then decide whether to land the workaround or not?

    David

Comment 60

[David Flanagan [:djf]]

(In reply to Jan Jongboom [:janjongboom] from comment #53)
> Couple of things:
> 
> 1. With the new inputmethod API we should not keep state in the keyboard
> itself, but rely on the events coming from inputmethod (which always give us
> correct state).

Having the correct state will be really good. But I don't think it will fix this bug. One of the things I tried was re-activiting the keyboard (i.e. sending fresh state) when a newline was added or deleted.  But even when doing that, I still saw the incorrect deletion of the newline after an auto-correct.  So even when forms.js and gaia agree on the state of the input, we can't correctly do an auto-correct right after space newline. I suspect this is because forms.js itself does not know exactly what the state of the contenteditable element is when there is trailing whitespace.

Comment 61

[David Flanagan [:djf]]

(In reply to Henri Sivonen (:hsivonen) from comment #57)
> (In reply to David Flanagan [:djf] from comment #43)
> > if I log elt.textContent on every
> > input event, the textcontent never includes newlines.
> 
> This is expected. textContent  returns a concatenation of the descendent
> text nodes. Unlike IE's innerText, it ignores <br>.

Thanks for looking at this, Henri.  I was surprised when I added comment 43, but then realized that the messaging app must be reading the user's input with innerHTML and converting <br> to newline.

More generally, if you know anything about how trailing whitespace works in contenteditable elements, that would be a big help.

Note that there is also a bug where the cursor sometimes disappears when it is at the end of the input. (Yuan do we have a bug # or STR for that?) I'm guessing that that bug might be related to this one, so if you know anything about cursor display in content-editable elements, that might help us here, too.

Comment 62

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

(In reply to David Flanagan [:djf] from comment #61)
> (In reply to Henri Sivonen (:hsivonen) from comment #57)
(In reply to David Flanagan [:djf] from comment #59)
> Thanks, Yuan. Do you want to take the bug and then decide whether to land
> the workaround or not?
> 
>     David

I tend to land this workaround first, as I'm not sure how much time I will spend and it may takes a week or even more.

> > (In reply to David Flanagan [:djf] from comment #43)
> Note that there is also a bug where the cursor sometimes disappears when it
> is at the end of the input. (Yuan do we have a bug # or STR for that?) I'm
> guessing that that bug might be related to this one, so if you know anything
> about cursor display in content-editable elements, that might help us here,
> too.
Yes, see bug 890207

Comment 63

[Jan Jongboom [:janjongboom] (inactive)]

Alright, let's land it for 1.1 and come up with a real fix in 1.2

Comment 64

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

Attachment: Don't trim spaces before newline for contentEditable.

@Henry, could you help to check if the patch uses correct nsIDocumentEncoder flags without introducing side effects?

@David, please help to verify if the patch resolves the issue of the keyboard auto-correction.

The bug is caused by mistakenly trimming the spaces before new line. The keyboard API - replaceSurroundingText relies on nsIDocumentEncoder to get the raw text content of a content editable elment. By digging into nsPlainTextSerializer.cpp(https://hg.mozilla.org/mozilla-central/file/d54e0cce6c17/content/base/src/nsPlainTextSerializer.cpp#l1369) I found if nsIDocumentEncoder.OutputFormatFlowed isn't set, the spaces from the end of the line will be removed. So the patch addes the nsIDocumentEncoder.OutputFormatFlowed flag to let replaceSurroundingText get correct text content.

Comment 65

[David Flanagan [:djf]]

Comment on attachment 798715 [details] [diff] [review]
Don't trim spaces before newline for contentEditable.

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

Yuan,

I've applied this patch to a b2g18 build (by editing omni.ja and pushing it back to the phone) and it fixes the bug for me.

I had tried altering flags for the editor object, but didn't think to try the encoder object. This is a nice, simple solution, and it makes perfect sense that this fixes the bug.  

Thanks for fixing it!

Comment 66

[Jan Jongboom [:janjongboom] (inactive)]

We backporting this to v1.1 then?

Comment 67

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

Yes, we already had leo+ set.

Comment 68

[Ivan Tsay (:ITsay)]

Hi Xulei,

May I know when this fix will be back port and landed to v1-train? Is it possible to be done this Friday? Our partner is targeting on 9/9 to integrate our latest gaia for the TA build. Thank you!

Comment 69

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

Comment on attachment 798715 [details] [diff] [review]
Don't trim spaces before newline for contentEditable.

Hi Henri, if you have time, please give us some feedback about the patch.

Comment 70

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

Attachment: part2 for B2G forms.js

Add commit message.

Comment 71

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

Please checkin the second patch of comment 70.

@Ivan, I think it will land by Friday.

Comment 72

[Henri Sivonen (:hsivonen)]

Comment on attachment 799980 [details] [diff] [review]
part2 for B2G forms.js

> @Henry, could you help to check if the patch uses correct nsIDocumentEncoder flags without introducing side effects?

As far as I can tell,  the main side effect that could be introduced is a  mismatch between display as HTML and serialization as plaintext if this code is used in the context where the HTML comes from an external source, since HTML coming from an external source  could have multiple spaces in the DOM where only a single space should appear in rendering. If this code only operates on HTML that was generated by Gaia itself, the use of  OutputPreformatted should be okay.

Comment 73

[Henri Sivonen (:hsivonen)]

(In reply to David Flanagan [:djf] from comment #61)
> More generally, if you know anything about how trailing whitespace works in
> contenteditable elements, that would be a big help.

I don't know about the specifics. I just know on the general level that during the Netscape days, hack was introduced to generate <br>  here and there to force conceptually empty blocks to actually be non-empty so that they'd render as one-line-high instead of letting them collapse to zero-height  so that there was somewhere to place the I-beam.

Accomplishing this by introducing cruft in the DOM (as opposed to making empty blocks show up as minimum 1em tall in layout when editing)  has  been a source of trouble ever since. Aryeh Probably knows more about the current details.

Comment 74

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

(In reply to Henri Sivonen (:hsivonen) from comment #72)
> As far as I can tell,  the main side effect that could be introduced is a 
> mismatch between display as HTML and serialization as plaintext if this code
> is used in the context where the HTML comes from an external source, since
> HTML coming from an external source  could have multiple spaces in the DOM
> where only a single space should appear in rendering. If this code only
> operates on HTML that was generated by Gaia itself, the use of 
> OutputPreformatted should be okay.

If so, I'd like create another patch to avoid such cases.

Comment 75

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

Attachment: part2 for B2G forms.js - v2

Henri, please help with this urgent bug. Thank you in advance and I'll buy you a bear :-)

The patch adds the flag `OutputNoRemoveLineEndingSpaces` to plaintext encoder to stop removing endings spaces of a line.

Comment 76

[Jan Jongboom [:janjongboom] (inactive)]

Lol, I also want a bear.

Comment 77

[Henri Sivonen (:hsivonen)]

Comment on attachment 800021 [details] [diff] [review]
part2 for B2G forms.js - v2

r+ if you rename OutputNoRemoveLineEndingSpaces to OutputDontRemoveLineEndingSpaces. (For consistency with OutputDontRewriteEncodingDeclaration.)

Comment 78

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

Attachment: part2 for B2G forms.js - v3

Rename OutputNoRemoveLineEndingSpaces to OutputDontRemoveLineEndingSpaces.

Comment 79

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

Henri, thank you and I owe you a bear(not a beer) :-D.

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c2e5fd62134e

Comment 81

[Ed Morley [:emorley]]

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

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2b7ee3f00bdc

As mentioned elsewhere, please direct any further follow-up work to new bugs.

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2b7ee3f00bdc

Comment 84

[Jan Jongboom [:janjongboom] (inactive)]

I landed the tests and the updated forms.js (so this is also fixed in FF nightly) in https://github.com/mozilla-b2g/gaia/commit/f482645c9d0e8ca3c4cb600f046f7e469ae41c00. They use the XPIDL syntax so we need to update when bug 906096 lands but soit.

Comment 85

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to David Flanagan [:djf] from comment #61)
> More generally, if you know anything about how trailing whitespace works in
> contenteditable elements, that would be a big help.

I know something about the subject, although very possibly not what you need to know.  Do you have any specific questions?  AFAIK, Gecko automatically converts some spaces into nbsp's in contenteditable so that hitting space twice actually works.  All browsers do something similar.  If you want some brief essays on the topic that aren't Gecko-specific, you can refer to

https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#canonical-space-sequences

and

https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-inserttext-command (click the first "View comments", right-floated next to "Action:", for the relevant bit)

See also discussion here:

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-June/032187.html

The upshot is that there's no good answer except using white-space: pre-wrap on everything contenteditable-related, which solves all problems in one fell swoop as long as the content isn't subsequently reposted somewhere that's not pre-wrapped.

This may or may not be relevant to what you want to know.