432415 - Strip trailing newlines when editor.singleline.pasteNewlines == 2 (Copy/pasting a cell from an xls/ods file adds an extra space at the end (Excel (Microsoft Office) / Calc (Open Office)))

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Thomas Goldstein]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050506 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050506 Minefield/3.0pre

Copying and pasting a cell from Excel or Calc to Firefox results in an extra space added at the end of the copied data.
To reproduce this, make sure to copy the cell as a whole, not just the text inside of it. Then paste it in any Firefox field (search bar, site fields...). Note that this doesn't work in the URL bar, though (that is to say the URL bar doesn't have this bug, as no extra space is added after the pasted text).
If you copy a blank cell, than a single space will be pasted.

This doesn't happen with IE.

Reproducible: Always

Steps to Reproduce:
1. Open/create an Excel/Calc document
2. Copy a cell (that contains text or not)
3. Paste it in any Firefox field (except URL bar)
Actual Results:  
The cell content is pasted with an extra space at its end

Expected Results:  
The cell content is pasted with no extra space

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050506 Firefox/3.0.0.0 ID:2008050506, MS Excel 2003 and MSIE7

Comment 2

[Thomas Goldstein]

By the way, I just noticed this is a regression. It works fine in Firefox 2.
I think that bug can be rather problematic, as I almost got an account temporarily locked because of it. It was on the Dell site, where you have limited amount of chances to input the right password. By having copied and pasted the password from an Excel file, an extra space was appended to the password, making it incorrect.

Comment 3

[Roma]

confirmed on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0

This bug is unacceptable for people who store passwords in Excel.

Comment 4

[Roma]

the same issue has new Opera 9.5, except it creates TWO spaces after pasted word.

Comment 5

[Joshua Burshaw]

confirmed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

I have been using Excel 2003 sp3
and I am experiencing the exact same thing,
copy a cell in excel and paste into an field in Firefox 3 results in an added space at the end of whatever was being copied

this bug does not exsist in either IE7 or Firefox 2

Comment 8

[Wayne Mery (:wsmwk)]

xref bug 85649 newline POP3 password -- which needs some QA love

Comment 11

[Colin Cody]

re: MS WORD
The addition of ASCII 32 during the paste function can also be simulated when copying text from MS Word.   

Using MS Word cut both a LF AND a CR at the end of the text string (or a CR AND a LF). Using this MS Word test I get 2 ASCII 32 (spaces) using Firefox. Whereas, MS IE 6.0 handles the paste normally (no spaces). 

If you cut just one from MS Word  i.e. either one CR   or one LF, then Firefox correctly will ignore that during the paste function.


re: MS EXCEL
The bug is very consistent.  Therefore I have learned that the temporary work-around for User ID's and passwords stored in Excel - is to [backspace] exactly one character after each paste.  Then the user ID and password will be accepted at the web site.

Comment 12

[dieselviulu]

This particular bug is a show-stopper for me, it is preventing me from upgrading from firefox 2 to 3. I'm just a regular user so all I can do is just announce that there are some people who can't use firefox 3 because of this bug. It's not passwords that I copy, but a big part of my job is to enter existing data (from excel) into one EU-project's database through a web portal consisting of web forms. It will take months before I'm finished and I'm looking at 700 items each consisting of about 20 cells of data, so one more keypress to 14000 copy-paste operations is not only a lot of work but also generates a huge opportunity for error.

ps. I know it would all be avoided with a mass import of data through a format conversion, but we have begged for that for a year now and it just is not going to happen. So please fix this one! I decided to comment this after I got a bad virus infection on friday through the latest 2.x version and had to restore entire system partition.

Comment 13

[FERNANDO FERNALD]

I have 30 years working with PCs. This extra space is very annoying. My whole department pastes part numbers on intranet programs all the time. Now we have an extra-space that generates an error. Many other uses for the copy-and-paste are also affected.  PLEASE!!!  Give some time to fix this error A.S.A.P. !!!
Thanks

Comment 16

[Thomas Goldstein]

Any chance of this being fixed for Firefox 3.1?
Can we do anything to help, like find a regression range?

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Jim, any chance you can take a look at this?

Thomas, a regression range would be extremely helpful!

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> Jim, any chance you can take a look at this?
> 
> Thomas, a regression range would be extremely helpful!

yep!

Comment 19

[Jim Mathies [:jimm]]

Ok, so there is a carriage return on the end of these cells when they are pasted. In standard text edits, the current rules require these be converted to spaces. I found this in nsTextEditRules.cpp if your looking for past discussion.

// People have lots of different ideas about what text fields
// should do with multiline pastes.  See bugs 21032, 23485, 23485, 50935.
// The six possible options are:
// 0. paste newlines intact
// 1. paste up to the first newline (default)
// 2. replace newlines with spaces
// 3. strip newlines
// 4. replace with commas
// 5. strip newlines and surrounding whitespace
// So find out what we're expected to do:

The short answer is, this has come up before. The default is 2, but you can change it to 3 using the following pref - 

editor.singleLine.pasteNewlines

That should solve the problem. I think we can close this out.

Comment 20

[Jim Mathies [:jimm]]

bug 406062 was where it was added, although i can't find the bug that made the change to text

Comment 21

[Roma]

Jim,

Thank you for your solution. I'm sure a lot of people will benefit form this.
Just so it was clear for everyone here are the steps to fix the bug:
1. Open file with editor at C:\Program Files\Mozilla Firefox\defaults\pref\firefox.js
2. Search for editor.singleLine.pasteNewlines", 2
3. Replace it with editor.singleLine.pasteNewlines", 3
4. Restart Firefox.

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #21)
> Jim,
> 
> Thank you for your solution. I'm sure a lot of people will benefit form this.
> Just so it was clear for everyone here are the steps to fix the bug:
> 1. Open file with editor at C:\Program Files\Mozilla
> Firefox\defaults\pref\firefox.js
> 2. Search for editor.singleLine.pasteNewlines", 2
> 3. Replace it with editor.singleLine.pasteNewlines", 3
> 4. Restart Firefox.

Or just use 'about:config'! In the text edit, type in 'pasteNewlines', that should filter down to the individual pref.

Comment 23

[Colin Cody]

(In reply to comment #19)
> I found this in nsTextEditRules.cpp 
> // The six possible options are:
> // 0. paste newlines intact
> // 1. paste up to the first newline (default)
> // 2. replace newlines with spaces
> // 3. strip newlines
> // 4. replace with commas
> // 5. strip newlines and surrounding whitespace
> // So find out what we're expected to do:
> The short answer is, this has come up before. The default is 2, but you can
> change it to 3 using the following pref - 
> editor.singleLine.pasteNewlines
> That should solve the problem. I think we can close this out.

MY COMMENTS:
As per the nsTextEditRules.cpp the DEFAULT is supposed to be "1" (not a "2") yet Firefox is "shipping" with a "2."   The powers that be should be informed to change the "2" to a "1"

I changed my firefox.js to: editor.singleLine.pasteNewlines", 1  this way I conform to the proper default.  pasting within Firefox now works great!!

Comment 24

[(not currently active) Ted Mielczarek]

The platform and Firefox have different defaults. It's just the way it is. Thunderbird sets a different default as well:
http://mxr.mozilla.org/comm-central/source/mail/app/profile/all-thunderbird.js#211

We could add some special-casing here to strip a trailing newline before converting them to spaces. It's probably not useful to put a space in in that case. In general however, we think it's more useful to paste all the text you have on the clipboard instead of cutting it off at the first newline, which is why we flipped the pref.

Comment 25

[Evan Parker]

One of the benefits of setting this preference to "2" is that it allows multi-line address cut/paste to work with online mapping applications like Google Maps.  Setting the preference to "1" or "3" by default would break this use case.  See bug 406062 for a discussion of this.

Ted - I like your idea of stripping trailing newlines before converting them to spaces.  This would not break Google Maps, and it would seem to address the problems described in this thread.

Comment 26

[(not currently active) Ted Mielczarek]

Morphing this bug to address the problem. This should be a one-line patch, add
tString.Trim(CRLF, PR_FALSE, PR_TRUE);
right before the call to ReplaceChar here:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsTextEditRules.cpp#588

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Clearing flags since this got morphed!

Comment 28

[Colin Cody]

Okay, I'm livid now.
After we finally were given a solution on how to undo the "helpful change" some developer did by quiently changing the default settings - by telling users to perform a user-unfriendly hack of the file: firefox.js.  --  Tonight an automatic upgrade-download happened and the "helpful change" was made again (just in case I didn't really want to get rid of it the first time, no doubt!)

So now users are going to have to remember all their hacks for years to come and continually re-hack after each upgrade?

Luckily I remembered how to get to this web page so I could re-read how to do the edits needed... 


(In reply to comment #23)
> (In reply to comment #19)
> > I found this in nsTextEditRules.cpp 
> > // The six possible options are:
> > // 0. paste newlines intact
> > // 1. paste up to the first newline (default)
> > // 2. replace newlines with spaces
> > // 3. strip newlines
> > // 4. replace with commas
> > // 5. strip newlines and surrounding whitespace
> > // So find out what we're expected to do:
> > The short answer is, this has come up before. The default is 2, but you can
> > change it to 3 using the following pref - 
> > editor.singleLine.pasteNewlines
> > That should solve the problem. I think we can close this out.
> MY COMMENTS:
> As per the nsTextEditRules.cpp the DEFAULT is supposed to be "1" (not a "2")
> yet Firefox is "shipping" with a "2."   The powers that be should be informed
> to change the "2" to a "1"
> I changed my firefox.js to: editor.singleLine.pasteNewlines", 1  this way I
> conform to the proper default.  pasting within Firefox now works great!!

Comment 29

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Uh, why are you editing firefox.js?  Just open about:config, type in 'pasteNewlines' or something to find it, and change the value.

firefox.js is the default prefs; it will always be overwritten.  You should be changing your user prefs via about:config.

Comment 30

[Colin Cody]

Thanks you for the head's up.  Yes, while "about:config" was mentioned above, I didn't have a clue what that meant.  So I edited firefox.js as was also mentioned above.

I presume using "about:config" leaves the user preferences unchanged whenever an upgrade is done?  If so, then that's what I was looking for.  But still pity the poor Firefox users who do not understand that ASCII 32 is being inserted into cut&pastes ;-(

And for others like me who were not familiar with "about:config", here's the instruction I found on the internet:

Type about:config in the Firefox URL address bar and press Enter, and you'll see all the settings currently enumerated in prefs.js, listed in alphabetical order. 

Type in "newline" as the filter, double click the item and change the numeric value, as per above.

Comment 31

[(not currently active) Ted Mielczarek]

We believe that this setting is in the best interest of users. I'm sorry that it breaks your use case, we'll try to implement my suggestion from comment 24 so that it works well for all users. Changing this setting was a conscious decision, and this is the only negative aspect I've heard of it, so I don't see any reason we ought to revert the change.

Comment 32

[(not currently active) Ted Mielczarek]

Attachment: simple patch

Here's a simple patch that implements that suggestion.

Comment 33

[(not currently active) Ted Mielczarek]

Attachment: simple patch + test

Add a chrome mochitest.

Comment 34

[Colin Cody]

Ted, I read the patch, and to me that definitely does seem to answer the issues, and actually, is something I had wondered about decades ago as to why that was not the way a cut & paste would work generally, in all applications!  After testing and studying cut & paste in back in the 1980's (as a user) I came to the conclusion that there must be good reasons that the powers that be had in mind and just left it at that.

Question:: Is this patch up for discussion so that Firefox might use it as a future upgrade?  Or is this some user preference?  If so, then as a user, I don't know where to start to try to implement it.

Comment 35

[(not currently active) Ted Mielczarek]

If this patch gets review then I intend for it to be in Firefox 3.1, and I might try to get it backported for the next release of Firefox 3.0.x (probably 3.0.6).

Comment 36

[[e-mail]]

Comment on attachment 348545 [details] [diff] [review]
simple patch + test

>     case nsIPlaintextEditor::eNewlinesReplaceWithSpaces:
>+      // Strip trailing newlines first so we don't wind up with trailing spaces
>+      tString.Trim(CRLF, PR_FALSE, PR_TRUE);
It's interesting that eNewlinesPasteToFirst and eNewlinesReplaceWithCommas also strip leading newlines too. (Sadly the test doesn't test that...)

Comment 37

[(not currently active) Ted Mielczarek]

I can change the test to test those cases as well. Makes sense to have all our assumptions on the table.

Comment 38

[(not currently active) Ted Mielczarek]

Attachment: patch + updated test

Updated the tests to reflect the stripping of leading and trailing newlines in all varieties.

Requesting approval for b2. This is a small change that fixes an annoying bug for users of spreadsheets, and it has automated test coverage.

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348977 [details] [diff] [review]
patch + updated test

a191b2=beltzner as a deal with the devil to get ted to land a bunch of stuff for me

Comment 40

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/b26e01b47e37

Comment 42

[(not currently active) Ted Mielczarek]

Comment on attachment 348977 [details] [diff] [review]
patch + updated test

Looking for branch approval. This is a very simple change with tests that fixes a small but annoying regression in our copy/paste behavior.

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 348977 [details] [diff] [review]
patch + updated test

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 44

[(not currently active) Ted Mielczarek]

Fixed on 1.9.0 branch:
Checking in editor/libeditor/text/nsTextEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRules.cpp,v  <--  nsTextEditRules.cpp
new revision: 1.212; previous revision: 1.211
done
Checking in toolkit/content/tests/chrome/test_bug253481.xul;
/cvsroot/mozilla/toolkit/content/tests/chrome/test_bug253481.xul,v  <--  test_bug253481.xul
new revision: 1.2; previous revision: 1.1
done

Comment 47

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

Verified fixed for 1.9.0.6 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6pre) Gecko/2009010606 GranParadiso/3.0.6pre and openoffice.