bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 26, Firefox OS v1.1hd

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zhangdapeng, Assigned: yxl)

Tracking

unspecified
1.1 QE5
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox24 wontfix, firefox25 wontfix, firefox26 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(7 attachments, 6 obsolete attachments)

32.08 KB, image/png
Details
2.48 MB, application/octet-stream
Details
131.02 KB, application/octet-stream
Details
46 bytes, text/x-github-pull-request
Details
692 bytes, patch
janjongboom
: review+
Details | Diff | Splinter Review
110 bytes, text/html
yxl
: review+
Details
6.13 KB, patch
yxl
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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, 



Expected results:

Automaticly add a letter
(Reporter)

Comment 1

5 years ago
(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
Blocks: 899451

Comment 2

5 years ago
leo+ requested per bug 899451.
blocking-b2g: --- → leo?
Can we get more attention on this and find out if it's consistently reproducible?
Keywords: qawanted

Updated

5 years ago
QA Contact: dkumar

Comment 4

5 years ago
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)
Flags: needinfo?(zhang.dapeng)
Keywords: qawanted
QA Contact: dkumar
(Reporter)

Comment 5

5 years ago
Created attachment 789476 [details]
1308074839输入字符异常.zip

it‘s the log correspond to this problem
Flags: needinfo?(zhang.dapeng)
Alive - is this something you could take a look at (log in comment 5)?  It looks like this might be a POVB issue.
Flags: needinfo?(alive)
Tim, if Alive isn't able to help out here can you find someone who can look at this log and confirm POVB?
Flags: needinfo?(timdream)
transfer to rudy :)
Flags: needinfo?(alive)
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.
Flags: needinfo?(timdream)
Please provide an STR that can be followed to reproduce this issue.  Since 2 people have tried and cannot reproduce this, leo-.
blocking-b2g: leo? → -
Flags: needinfo?(zhang.dapeng)
(Reporter)

Comment 11

5 years ago
(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)
Flags: needinfo?(zhang.dapeng)
(Reporter)

Comment 12

5 years ago
Created attachment 791894 [details]
screenshots.7z
(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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [zffos1.1][input method] Spanish input method error → [zffos1.1][input method] word suggestion may output one extra character in SMS message composing
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.
Component: Gaia::Keyboard → General
Flags: needinfo?(xyuan)
Summary: [zffos1.1][input method] word suggestion may output one extra character in SMS message composing → [zffos1.1][input method] replaceSurroundingText() may output the wrong result when it mutates a contenteditable element
It is a bug of mozKeyboard.replaceSurroudingText with contentEditable. The API replaces wrong word if there is a new line character before the word.
Assignee: nobody → xyuan
Flags: needinfo?(xyuan)
Jan, please feel free to take the bug if you have time.
Status: NEW → ASSIGNED
Assignee: xyuan → janjongboom
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...
Created attachment 793518 [details] [diff] [review]
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...
Attachment #793518 - Flags: review?(xyuan)
+ some test cases, so we can also reproduce it.
Verified problem on v1-train. It's not possible to write multiline SMS messages if autocorrect is enabled.
blocking-b2g: - → leo?
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');
Attachment #793518 - Flags: review?(xyuan)
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

5 years ago
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.
blocking-b2g: leo? → leo+
(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.
Created attachment 793918 [details] [diff] [review]
Workaround for B2G forms.js v2
Attachment #793518 - Attachment is obsolete: true
Attachment #793918 - Flags: review?(xyuan)
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.
Attachment #793918 - Flags: review?(xyuan) → review+
Comment on attachment 793520 [details]
Tests

Rudy, please help to review these tests.:)
Attachment #793520 - Flags: review?(xyuan) → review?(rlu)
(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...
Created attachment 793946 [details] [diff] [review]
Workaround for B2G forms.js v3

Alright, my diff generation script was buggy. This will do.
Attachment #793918 - Attachment is obsolete: true
Attachment #793946 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0a8fb7ea378b

Should this have a test?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Yes, but the tests live in Gaia. (see other attachment)
Great, thanks!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/54d3febd5d49
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox24: --- → wontfix
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → 1.1 QE5

Comment 37

5 years ago
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

5 years ago
Do you want me to open a new bug or can this one be reopened?
Comment on attachment 793520 [details]
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.
Attachment #793520 - Flags: review?(rlu)
Reproduced on english locale as well with
T R Y SPACE ENTER J A SPACE
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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...
Assignee: janjongboom → nobody
Flags: needinfo?(daniel)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 910687
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
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.
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?
Created attachment 797596 [details]
partial gaia-based workaround
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.
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)
status-b2g18: fixed → affected
status-b2g-v1.1hd: fixed → affected
Flags: needinfo?(xyuan)
Flags: needinfo?(timdream)
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.
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.
Taking this myself to try the gaia workaround I proposed above.
Assignee: nobody → dflanagan
Flags: needinfo?(xyuan)
Flags: needinfo?(timdream)
Created attachment 797728 [details]
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.
Attachment #797596 - Attachment is obsolete: true
Attachment #797728 - Flags: review?(xyuan)
Attachment #797728 - Flags: review?(janjongboom)
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.
cc Henry as he knows much more about the plaintext serializer.
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...
Attachment #797728 - Flags: review?(xyuan) → review+
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;-)
(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>.
(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.
(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
(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.
(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.
(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
Assignee: dflanagan → xyuan
Alright, let's land it for 1.1 and come up with a real fix in 1.2
Created attachment 798715 [details] [diff] [review]
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.
Attachment #798715 - Flags: review?(hsivonen)
Attachment #798715 - Flags: review?(dflanagan)
Flags: needinfo?(daniel)
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!
Attachment #798715 - Flags: review?(dflanagan) → review+
We backporting this to v1.1 then?
Yes, we already had leo+ set.

Comment 68

5 years ago
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!
Flags: needinfo?(xyuan)
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.
Attachment #798715 - Flags: review?(hsivonen) → feedback?(hsivonen)
Created attachment 799980 [details] [diff] [review]
part2 for B2G forms.js

Add commit message.
Attachment #798715 - Attachment is obsolete: true
Attachment #798715 - Flags: feedback?(hsivonen)
Attachment #799980 - Flags: review+
Attachment #799980 - Flags: feedback?(hsivonen)
Flags: needinfo?(xyuan)
Please checkin the second patch of comment 70.

@Ivan, I think it will land by Friday.
Keywords: checkin-needed
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.
Attachment #799980 - Flags: feedback?(hsivonen) → feedback+
(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.
(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.
Keywords: checkin-needed
Created attachment 800021 [details] [diff] [review]
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.
Attachment #799980 - Attachment is obsolete: true
Attachment #800021 - Flags: review?(hsivonen)
Lol, I also want a bear.
Comment on attachment 800021 [details] [diff] [review]
part2 for B2G forms.js - v2

r+ if you rename OutputNoRemoveLineEndingSpaces to OutputDontRemoveLineEndingSpaces. (For consistency with OutputDontRewriteEncodingDeclaration.)
Attachment #800021 - Flags: review?(hsivonen) → review+
Created attachment 800132 [details] [diff] [review]
part2 for B2G forms.js - v3

Rename OutputNoRemoveLineEndingSpaces to OutputDontRemoveLineEndingSpaces.
Attachment #800021 - Attachment is obsolete: true
Attachment #800132 - Flags: review+
Henri, thank you and I owe you a bear(not a beer) :-D.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c2e5fd62134e
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2b7ee3f00bdc

As mentioned elsewhere, please direct any further follow-up work to new bugs.
status-b2g18: affected → fixed
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.
(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.
Attachment #797728 - Flags: review?(janjongboom)
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.