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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Editor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jaroslav Benc, Assigned: Ehsan)

Tracking

({regression, verified1.9.2})

Trunk
mozilla2.0b7
regression, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .11+, status1.9.2 .11-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [tb31needed][fixed tb3.1.4])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 471025 [details]
testcase
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?
(Reporter)

Comment 3

7 years ago
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
(Reporter)

Comment 4

7 years ago
Created attachment 471031 [details]
test case with white-space:pre-wrap;
"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?
(Reporter)

Comment 6

7 years ago
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.
(Reporter)

Comment 7

7 years ago
Created attachment 471036 [details]
test case with multipe spaces - FF4 doesn't collapse them however returns innerHTML with the  
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?
(Reporter)

Comment 9

7 years ago
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.
> 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
Summary:   is returned by innerHTML when user inserts a space (designMode on) → Non-breaking spaces inserted when multiple spaces are typed, even in pre-wrap-styled text
(Reporter)

Comment 11

7 years ago
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.
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?
(Reporter)

Comment 13

7 years ago
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
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?
Blocks: 336104
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Created attachment 471562 [details] [diff] [review]
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #471562 - Flags: review?(roc)
Keywords: regressionwindow-wanted
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
why not just |content = content->GetParent()|?
What Boris said...
In fact, why not just do
  if (!content->IsElement()) {
    content = content->GetParent();
  }
  if (content->IsElement()) {
    elementStyle = ...;
  }
blocking1.9.2: ? → .10+
status1.9.1: --- → unaffected
status1.9.2: --- → wanted
(Reporter)

Comment 19

7 years ago
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
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.
(Reporter)

Comment 21

7 years ago
Hmm, that's unfortunate, do you already have a release date for 3.6.10 version so we could include it in the email?
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.
(Reporter)

Comment 23

7 years ago
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.
(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?
Created attachment 472720 [details] [diff] [review]
Patch (v1.1)
Attachment #471562 - Attachment is obsolete: true
Attachment #472720 - Flags: review?(roc)
Attachment #471562 - Flags: review?(roc)
Attachment #472720 - Flags: review?(roc) → review+
Attachment #472720 - Flags: approval2.0?
Attachment #472720 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/1499c617241b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Attachment #472720 - Flags: approval1.9.2.10?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 472899 [details] [diff] [review]
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.
Attachment #472899 - Flags: review?(roc)
Status: REOPENED → ASSIGNED
Attachment #472899 - Flags: review?(roc) → review+
(Reporter)

Comment 29

7 years ago
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?
blocking2.0: ? → final+
(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.
Created attachment 473106 [details] [diff] [review]
Part 3: add a test for the back-space behavior.

The test promised in comment 30.
Attachment #473106 - Flags: review?(roc)
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?
Attachment #472720 - Flags: approval1.9.2.10?
(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.
Attachment #473106 - Flags: review?(roc) → review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Attachment #472720 - Flags: approval1.9.2.10?
Depends on: 594759

Comment 35

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

a=LegNeato for 1.9.2.10
Attachment #472720 - Flags: approval1.9.2.10? → approval1.9.2.10+
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.
status1.9.2: wanted → .10-fixed
This bug might also be worth taking on the relbranch for Thunderbird and SeaMonkey.
Duplicate of this bug: 595197
Whiteboard: [tb31needs]
(Reporter)

Comment 39

7 years ago
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.
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...
(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.
Whiteboard: [tb31needs] → [tb31needed][fixed tb3.1.4]
Duplicate of this bug: 597017
Duplicate of this bug: 597548
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).
Keywords: verified1.9.2
Blocks: 596001
Duplicate of this bug: 602163

Comment 46

7 years ago
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.
(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

7 years ago
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.
You need to log in before you can comment on or make changes to this bug.