592592 - Non-breaking spaces inserted when multiple spaces are typed, even in pre-wrap-styled text

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jaroslav Benc]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

When user types a space in an element with designMode enabled   is returned when accessing innerHTML. See attached example. 

Reproducible: Always

Steps to Reproduce:
1. create a document with designMode on
2. type a space 
3. see body innerHTML
Actual Results:  
there is a   entity instead of a space character

Expected Results:  
should be a space character as in older versions of FF

This is affecting our code editor which saves   entities instead of spaces now. We could replace   entities by spaces before saving but there could be a case when   is actually part of the code and has to be saved.

Comment 1

[Jaroslav Benc]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

When you type multiple spaces in a row the editor inserts non-breaking spaces in the DOM, because typing two spaces needs to look like two spaces and if it inserted regular spaces it wouldn't: spaces are collapsed when rendering HTML.

Safari and Chrome have the same behavior on the attached testcase, for the same reason.

It looks like Opera also puts a non-breaking space in the DOM (as you can tell if you examine the actual chars in the textnode in the DOM) but then leaves it as a non-breaking space in the innerHTML (Unicode char U+00A0 as opposed to U+0020 or replacement with the entity escape).  I have no idea whether you want to be saving those raw non-breaking space chars in your editor.

So what's the bug, exactly?

Comment 3

[Jaroslav Benc]

Sorry, I didn't noticed an important thing - white-space on body is set to: pre-wrap. This makes FF3 and webkit returning spaces instead of the   See following attached example

Comment 4

[Jaroslav Benc]

Attachment: test case with white-space:pre-wrap;

Comment 5

[Boris Zbarsky [:bzbarsky]]

"pre-wrap" still collapses whitespace, so you still need to use non-breaking spaces or some other kind of uncollapsible space to get the spaces to show up next to each other.

Again, what's the bug?

Comment 6

[Jaroslav Benc]

Looks like "pre-wrap" actually prevents collapsing whitespaces to me, try the second example in FF3 & webkit. See latest example in FF4 with multiple spaces - FF doesn't collapse them as expected.

Comment 7

[Jaroslav Benc]

Attachment: test case with multipe spaces - FF4 doesn't collapse them however returns innerHTML with the  

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, right; I got confused between pre-wrap and pre-line.

So what you want is for the editor to generate different DOMs when pressing space depending on the styles that happen to be applied to the subtree being edited?  Is that really desirable?

Comment 9

[Jaroslav Benc]

Yes, that would be great if it works same as in FF3 - our XML editor uses innerHTML to build the code again from HTML - removing all html staff so it actually returns inner text. 

I have tried element.textContent some time ago using FF2 and FF3 & webkit and it was buggy (I think it didn't return whitespaces correctly) 

When using innerHTML we can't replace   by spaces manually as there might be some   in the XML code which we need to preserve.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Yes, that would be great if it works same as in FF3

Hmm.  Do you want to use nightlies to narrow down when the behavior changed?  That might make it easier to see why it did so.

Resummarizing this bug to make it clear what it's about.

> I have tried element.textContent some time ago using FF2 and FF3 & webkit and
> it was buggy (I think it didn't return whitespaces correctly) 

.textContent just returns whatever is in the DOM.  I have no idea what your problems with that were.

Comment 11

[Jaroslav Benc]

I've tried few nightly builds and found it's even in the 3.6.9pre 

This changeset seems to cause the problem: 
http://forums.mozillazine.org/viewtopic.php?f=23&t=1957265

This bug might cause it: 
https://bugzilla.mozilla.org/show_bug.cgi?id=581499

Last working build: 
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.9pre) Gecko/20100727 Namoroka/3.6.9pre

Build where this problem occured first:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.9pre) Gecko/20100728 Namoroka/3.6.9pre

Summary of the bug: 
U+00A0(NBSP) shouldn't be generated instead of U+0020 when pressing space when white-space is set or inherited to pre-wrap.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'd really doubt that changset caused the issue, but ccing ehsan.

Jaroslav, what are the mercurial changeset IDs (the HTTP URIs from about:buildconfig) for the two builds you mention in comment 11?

Comment 13

[Jaroslav Benc]

working 20100727: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/04193f2b9f27
broken 20100728:  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/53d164e4a461

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checkins in that range:

http://hg.mozilla.org/releases/mozilla-1.9.2/pushloghtml?fromchange=04193f2b9f27&tochange=53d164e4a461

Looks like a regression from bug 336104; I bet in this case aNode is a text node, so we get a null elementStyle, whereas before we used to get the style context from the textframe...

I think if |content| is not an element we should check whether its parent is, and if so use its style.  Ehsan, thoughts?

Comment 15

[(no longer active)]

Attachment: Patch (v1)

(In reply to comment #14)
> I think if |content| is not an element we should check whether its parent is,
> and if so use its style.  Ehsan, thoughts?

Hmm, yes, it makes sense to me.  Here's a patch which does that, and it fixes the problem.

Comment 16

[Boris Zbarsky [:bzbarsky]]

why not just |content = content->GetParent()|?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What Boris said...

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In fact, why not just do
  if (!content->IsElement()) {
    content = content->GetParent();
  }
  if (content->IsElement()) {
    elementStyle = ...;
  }

Comment 19

[Jaroslav Benc]

Hey guys, thanks for your quick response, I was thinking if the 3.6.9 version will be released without this bug fix as it's marked blocking 10+. We'd need to warn users to not update 3.6.9 if so as it breaks our product. 
Thanks

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Yes, that's unfortunate, but unavoidable at this point. Firefox 3.6.9 went into code freeze on August 12 (well before this bug was even discovered) and is set for release tomorrow. The patch on this bug has not even been reviewed yet. At least it'll be fixed for the next release.

Comment 21

[Jaroslav Benc]

Hmm, that's unfortunate, do you already have a release date for 3.6.10 version so we could include it in the email?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

No schedule is set for 3.6.10 yet. I would suggest keeping any eye on the weekly updates on Planet Mozilla for more information as it becomes available. I would expect that a tentative schedule will be made over the next week or so once 3.6.9 is out the door.

Comment 23

[Jaroslav Benc]

Thanks, I'll keep eye on it. Hopefully 3.6.10 will come soon as it might break other web editors using designMode.  

There is another symptom of this bug when using delete/backspace - it deletes all spaces in a row instead of just one. Is there any way we could prevent default backspace/delete and implement it in Javascript? Something like moveStart('character',-1) method in IE so we could select last character and call deleteContents(), Firefox has selection.modify only in gecko 2.0, can't see anything else usable. I have even tried syntetic events but they won't fire using navigation keys in designMode. Any quick help would be very helpful at the moment.

Comment 24

[(no longer active)]

(In reply to comment #23)
> There is another symptom of this bug when using delete/backspace - it deletes
> all spaces in a row instead of just one. Is there any way we could prevent
> default backspace/delete and implement it in Javascript? Something like
> moveStart('character',-1) method in IE so we could select last character and
> call deleteContents(), Firefox has selection.modify only in gecko 2.0, can't
> see anything else usable. I have even tried syntetic events but they won't fire
> using navigation keys in designMode. Any quick help would be very helpful at
> the moment.

Do you have a test case which demonstrates this issue, so that I can make sure that this patch fixes that problem as well?

Comment 25

[(no longer active)]

Attachment: Patch (v1.1)

Comment 26

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1499c617241b

Comment 27

[(no longer active)]

Backed out because of test failure on Mac:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283900831.1283901230.4131.gz
13554 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_texteditor_keyevent_handling.html | non-tabbable <textarea>: Tab - got "a\t", expected "a "

I haven't seen this failure locally, so I'll need to investigate.

Comment 28

[(no longer active)]

Attachment: Part 2: fix the test

The test failure is coming from an incorrect behavior being tested.  The reason that I had not seen this locally was that the test was corrected in attachment 457572 [details] [diff] [review] which I had applied locally.  This patch cherry-picks the necessary part of that attachment so that we can land this before bug 240933 lands.

Comment 29

[Jaroslav Benc]

Try the third attachment 471036 [details] with multiple spaces - place your caret after the second space inside the "> <" and press delete. This would delete all spaces and place your caret in the middle of word "here". 

(In reply to comment #24)
> 
> Do you have a test case which demonstrates this issue, so that I can make sure
> that this patch fixes that problem as well?

Comment 30

[(no longer active)]

(In reply to comment #29)
> Try the third attachment 471036 [details] with multiple spaces - place your caret after
> the second space inside the "> <" and press delete. This would delete all
> spaces and place your caret in the middle of word "here". 

Thanks!  This patch also fixes that problem, but I'll add a test case to make sure that it won't regress in the future.

Comment 31

[(no longer active)]

Attachment: Part 3: add a test for the back-space behavior.

The test promised in comment 30.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 472720 [details] [diff] [review]
Patch (v1.1)

Clearing approval until the test situation is resolved. Do we need a combined patch? Approval requests on several attachments?

Comment 33

[(no longer active)]

(In reply to comment #32)
> Comment on attachment 472720 [details] [diff] [review]
> Patch (v1.1)
> 
> Clearing approval until the test situation is resolved. Do we need a combined
> patch? Approval requests on several attachments?

We don't need approval to land tests, right?  The second and third patches are merely test changes.

Comment 34

[(no longer active)]

Relanded:

http://hg.mozilla.org/mozilla-central/rev/b6a390cdb03c
http://hg.mozilla.org/mozilla-central/rev/c25ac83f57bd
http://hg.mozilla.org/mozilla-central/rev/bbd54981403b

Comment 35

[LegNeato]

Comment on attachment 472720 [details] [diff] [review]
Patch (v1.1)

a=LegNeato for 1.9.2.10

Comment 36

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/124f8e9c499d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3e443cd66408

The 2nd part does not apply on 1.9.2 because the test in question doesn't exist there.

Comment 37

[(no longer active)]

This bug might also be worth taking on the relbranch for Thunderbird and SeaMonkey.

Comment 39

[Jaroslav Benc]

Thanks for fixing this bug, I've noticed 1.9.2 status was changed to .11+, does it mean it's not going to be released in FF 3.6.10? We're about to release a special build for FF 3.6.9 where we need to test user navigator - do we have to include 3.6.10 in the test as well? Also any info when this fix will be released would be appreciated.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Jaroslav, the release that was going to be 3.6.10 got renamed to 3.6.11 earlier today with no changes to its schedule otherwise.  3.6.10 is being shipped in the next several days at most as an update to 3.6.9 to fix one particular crash issue 3.6.9 introduced.

So yes, you should include 3.6.10 in your test too.  :(

The current tentative plan for 3.6.11 is to ship around mid-October, but the timing is not set in stone...

Comment 41

[Mark Banner (:standard8)]

(In reply to comment #37)
> This bug might also be worth taking on the relbranch for Thunderbird and
> SeaMonkey.

Yep, we've got it on the Thunderbird relbranch for 3.1.4:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/92c5cc223402
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b5ec733076fe

Note that SeaMonkey don't need this change as they haven't got a release series based off of 1.9.2.

Comment 44

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.11pre) Gecko/20100921 Namoroka/3.6.11pre using attached testcase (and bad behavior verified on 1.9.2.10).

Comment 46

[tgvrs_santhosh]

Hi,
I have tested in 3.6.11. The delete/backspace issue is solved. Thanks for that.

But, still one of the bugs mentioned in the defect 602163 is reproducible.

Try the following scenario with the testpage attached for the bug 602163

keep the indent selection just after the four spaces, then enter some character, it will be inserted in the nextline instead of in the same line.

Comment 47

[(no longer active)]

(In reply to comment #46)
> Hi,
> I have tested in 3.6.11. The delete/backspace issue is solved. Thanks for that.
> 
> But, still one of the bugs mentioned in the defect 602163 is reproducible.
> 
> Try the following scenario with the testpage attached for the bug 602163
> 
> keep the indent selection just after the four spaces, then enter some
> character, it will be inserted in the nextline instead of in the same line.

I could not reproduce this.  Could you please file a new bug about that with exact steps to reproduce?

Comment 48

[tgvrs_santhosh]

Hi Ehsan,

I have created another defect for this.
The URL of the defect is https://bugzilla.mozilla.org/show_bug.cgi?id=603863

I have attached the web-ex record for the bug 603863.