75283 - Pasted text from beginning of line always appends a newline

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect, P4)

Description

[Randell Jesup [:jesup] (needinfo me)]

FreeBSD 4.1  Mozilla 20010409xx

When pasting text from a page I browsed to in mozilla, it always has a newline
appended to the paste.  For example, on the page above I select "NSS 3.2 source
and binary distributions" (which is all on one line, and is followed by more
text), and then paste it into an xterm, emacs, etc, and it always has a newline
appended to it.  If I do the same thing from ns4.x, I don't see this.

Probably applies to all Xwindows, maybe all platforms.  Annoying and quite
unexpected (caught me by surprise once when I planned to edit the pasted text
before committing to running the command I'd cut out).  This _could_ cause
dataloss in theory if someone got surprised in just the wrong way.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Nominating for 1.0.  Note: this may be related to bug 55661.

Comment 2

[rubydoo123]

assigning to akkana to help in narrowing it down. This also happens on win98, if 
I select the paste into notepad I get the linefeed. If I paste into Composer, no 
linefeed. If I paste into the plaintext editor, I get a space at the end. I 
don't know if any of that helps or not.

Comment 3

[Akkana Peck]

Here's what appears to be going on: this happens if your selection includes the
beginning of a paragraph, but if you start the selection one character later
(e.g. "SS 3.2 source and binary distributions" you don't get the newline.

I suspect this is a dup of something else Joe is working on, so I'm sending it
to him (Joe, if I'm wrong, feel free to send it back).  I'm trying to
investigate further to be sure, but my current build (from last night) can't
load any urls either from the command line or the urlbar, so I'll have to update
and hope the problem is fixed.

Comment 4

[Travis Ogdon]

I see this for all pasted elements where the selection either started at the
begining of the line, - OR - for elements like this:

<pre class="admin">
Example root command
$lt;/pre>

(I'm not sure if Bugilla munges gt and lt as characters, so the above may look
wonky, but hopefully you'll get the idea...)

Every character from the end of the first > to the closing < will do the
linefeed thing. 

In case it matters here's the appropriate chunk of the stylesheet:

.admin {font-family: fixed; font-weight: bold; color: #00aa00;}

This is especially annoying to me personally since a lot of the documentation I
write uses stylesheets like the one above to illustrate root vs. user level
commands. 4.77 doesn't like the (validated) CSS, and Mozilla now has a hard time
pasting it... :(

Comment 5

[duane voth]

Interesting, Linux build 2001060713 was appending *two* new lines to
each paste, but now Linux build 2001062508 is only apending one, and
it happens only when copying from certain html tags (h2 is one of them).
Btw, I originally complained about this in the MailNews reader but I now
see it is ubiquitous.   Will the mailnews GUI folks get this report too?

Comment 6

[Kathleen :Brade]

*** Bug 90932 has been marked as a duplicate of this bug. ***

Comment 7

[Joe Francis]

It doesn't always add a newline.  You copied an entire html paragraph.  

the serializer needs to change: instead of emitting a br when you get to the end
of a block, it needs to set some state and only emit a br if other content gets
serialized after that.

over to serializer.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

For selection at the end of a paragraph, I agree - we need to see if anything is
added after it before adding the newline/etc.

However, I've found that some other selections append a newline even if they end
in the middle of a word.  Go to www.mozilla.org, and select (say) "Bugzilla 2.1"
(from the header of a paragraph in bold).  Paste it.  Note that a newline was
appended.  Select "ugzilla 2.1", paste it.  Note that there's no appended
newline.  Odd.

Go into a <pre> section (I used a jprof.html file).  Select some text in the
middle of a word.  Paste it.  2 newlines are appended.

So, we have _lots_ of problems with selections....

Upping severity because this has screwed me over innumerable times trying to
paste data out of <pre> blocks into search widgets or command-lines.
OS->All; it happens everywhere.

Comment 9

[Akkana Peck]

The newlines resulting from copying out of a pre block are covered by another
bug (jfrancis) and are a different issue.

As regards this bug: why aren't we doing what we do for many other tags, saying
that we only include the enclosing <p> tag if the entire contents of the tag is
selected?  Wouldn't that be a better fix?

If it is fixed in the serializer, EnsureVerticalSpace() might do the job.  But
let's make sure that's what we want to do, and that we really want to include
the enclosing <p> in a case where only the first bits of a paragraph are copied.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Nominating for 0.9.6.  This (and the associated <pre> bug) are a MASSIVE
annoyance to using mozilla as a primary browser.  I have to paste text into a
text editor, then clip it out of there to paste into other places.  This also
can cause dataloss when the user is constructing command lines using paste
(since the newline pasted in starts the command before you expected it to.)

Comment 11

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 12

[Tanu Mutreja]

-->myself

Comment 13

[Tanu Mutreja]

really 
-->myself

Comment 14

[Joe Francis]

Yhere are things we can do in the serializer (see my earlier comment) to improve 
the situation.  However, there are other things which probably have to happen in 
the document encoder to fix some of the other issues raised here.  In particular 
Randell's "Bugzilla" vs "ugzilla" result.  The only way the serializer would 
treat these differently is if the encoder was passing the H1 (or whatever block 
tag it is) to the serializer.  I agree with Akkana: that seems wrong.  

After you do what you can in the serializer you may need to assign the remaining 
issues to me.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Thanks for taking over this bug, Tanu.

Removing old target and nomination to get this back on radar (since we have a
new owner).  This _is_ a dataloss bug (potentially), and a truely, intensely
annoying bug in all cases.  I for one would not accept as finished a browser
with this bug in it.  I live with the bug for now because I know about it and
can work around it (with pain), but I wouldn't let this loose on unsuspecting
people with this bug.

Web pages often have shell/command-window commands in them that people select
and paste, expecting to be able to edit them.  Not to mention constantly having
to delete extra newlines when pasting jprof data into things, etc, etc.

Comment 16

[Tanu Mutreja]

Following diff solves this bug. I tried it for few more test cases but still 
need to make sure it's not breaking anything else.

Index: nsPlainTextSerializer.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsPlainTextSerializer.cpp,v
retrieving revision 1.39
diff -u -r1.39 nsPlainTextSerializer.cpp
--- nsPlainTextSerializer.cpp   2002/01/10 14:06:15     1.39
+++ nsPlainTextSerializer.cpp   2002/01/20 14:46:44
@@ -836,7 +836,7 @@
     // This is hard. Sometimes 0 is a better number, but
     // how to know?
     EnsureVerticalSpace((mFlags & nsIDocumentEncoder::OutputFormatted)
-                        ? 1 : 0);
+                        ? mEmptyLines + 1 : mEmptyLines);
   }

Comment 17

[Akkana Peck]

I don't understand the reasoning behind the patch.  In copy/paste, the formatted
flag should not be set, so we would have been calling EnsureVerticalSpace with
an argument of 0.  What is mEmptyLines set to in the example?  What happens if
you make a selection that includes the beginning of a paragraph, then goes
through the end of it and partway into the next paragraph, then paste into
plaintext?  Do we get a proper break between the two paragraphs?

Comment 18

[Tanu Mutreja]

The reason why I replaced hard coded '0' with mEmptyLines was to avoid the 
situations where we are already in the middle of a line and hence we have 
mEmptyLines as -1. In that case, EnsureVerticalSpace(0) calls EndLine() once 
which adds a linebreak to the content.

However, I've some concerns here:
o In general <p> is kinda autoclose tag. So in cases where only the partial 
content of tag <p> is selected and copied, I think during serialization also 
the parser should close the "p" tag as it does for the content being passed by 
the netlib.

o Now as the HTML specifications at 
http://www.w3.org/TR/html4/appendix/notes.html#notes-line-breaks says that 
"The following two HTML examples must be rendered identically:
<P>Thomas is watching TV.</P>

<P>
Thomas is watching TV.
</P>", we should be able to ensure a line break before we close the "p" tag (in 
formatted cases) but without effecting the content of the p tag. In EndLine() 
we see that we add a linebreak to the current content by "  mCurrentLine.Append
(mLineBreak);" which actually adds a line break to the content of the tag. This 
cause quite a few bugs including this one too. The only solution seems to be 
able to find some way which actually attaches the linebreak to the closing tag 
but not to the current content i.e. mCurrentLine here. This seems to effect 
only the DOM-TXT serialization but not otherwise probably because tokenizer 
takes care of it(not sure!) in other cases.
Additionally, it will not cause any problem for the situations where we cross a 
complete "p" tag and select some part of the next tag too as in that case the 
line break attached to the closing tag will also come in the picture and 
provide a separation between the two tags.

o Another solution for this bug may be obtained by treating "p" tag in a 
distinct manner in DoCloseContainer(). There if we can check that we have some 
more content after this "p" tag(which so far I don't know how to do :-)), we 
should ensure a line break otherwise we should call EnsureVerticalSpace
(mEmptyLines).

Comment 19

[Akkana Peck]

The p tags will be autoclosed for us by the parser -- a DOM representation has
no way of expressing a p tag that doesn't close.

Regarding html source that has newlines in it: there shouldn't be any difference
to us -- we should ignore newlines in html source, as they aren't significant
except as whitespace (i.e. they implement a word break and are otherwise
equivalent to other whitespace).  If we're ever putting out a linebreak in the
plaintext serializer just because we saw a line break in the html source (not in
a <pre> block), that's a bug and should be fixed.  I don't know of any cases
where we're doing that, but maybe you do.

Thanks for the explanation about mEmptyLines -- it makes sense, and your patch
works well in all the cases I've tested.  r=akkana

Comment 20

[Johnny Stenback (:jst)]

sr=jst

PS. In the future, please attach the diff files to bugs, even if they're tiny.

Comment 21

[Vinay Badami]

Fixed with checkin
C:\mozilla\content\base\src>cvs commit
cvs commit: Examining .
Checking in nsPlainTextSerializer.cpp;
/cvsroot/mozilla/content/base/src/nsPlainTextSerializer.cpp,v  <--  nsPlainTextS
erializer.cpp
new revision: 1.42; previous revision: 1.41
done

Comment 22

[Peter Van der Beken [:peterv]]

This checkin caused some of the dom to text conversion tests to fail
(simplecopy.out simplefmt.out  mailquote.out). See
http://tinderbox.mozilla.org/showlog.cgi?tree=SeaMonkey&errorparser=unix&logfile=1014040740.29214.gz&buildtime=1014040740&buildname=comet%20Linux%20Depend&fulltext=1
 , look for ----------- Output from DomToTextConversionTest ------------. The
tests are in mozilla/source/htmlparser/tests/outsinks/.

Comment 23

[Tanu Mutreja]

1. For failure of simplefmt.out, accroding to Akkana's comments(#19) it looks 
that Test Cases should be changed to behave properly . I would expect "H" 
instead of a new line here. Full report for this failure is as below: 

Testing simple html to plaintext formatting ...
Type Manifest File: 
D:\mozilla_Feb17\mozilla\dist\WIN32_D.OBJ\bin\components\xpti.dat
nsNativeComponentLoader: autoregistering begins.
nsNativeComponentLoader: autoregistering succeeded
nNCL: registering deferred (0)
Comparison failed at char 521: generated was 72, file had 10
Comparison failed at char 521:
-----

  Simple html page

Here is a link to the mozilla.org <http://www.mozilla.org> page. Here
is some _underlined and *bold*ened_ified text plus some <angle bracket
entities>.

Here is a line ending with a space followed by a line break. Plaintext
output should contain only one space (and no line breaks) between
"space" and "followed".

Here is a /list/:

    * An item
    * A nested ordered list:
    *
         1. item one
         2. item two
    * last item

Here is a paragraph after the list.
H
-----
Simple formatting test failed.

2. For failure of SimpleCopy.out I feel that we need to treat <p> tag a bit 
different from other BlockLevel tags(for which IsBlockLevel() returns true). In 
other cases we may still need to ensure one line break after the tag closes. 
Though we still need to change this TestCase for the same reason as above i.e. 
at "Here is a paragraph after the list.
H" I would expect "H" instead of a new line.
 
Testing simple copy case ...
Type Manifest File: 
D:\mozilla_Feb17\mozilla\dist\WIN32_D.OBJ\bin\components\xpti.dat
nsNativeComponentLoader: autoregistering begins.
nsNativeComponentLoader: autoregistering succeeded
nNCL: registering deferred (0)
Comparison failed at char 17: generated was 32, file had 10
Comparison failed at char 17:
-----
Simple html page
-----
Simple copy test failed.

3. For mailquote.out also, I think we need to change the test case. 

Will upload a proper patch tomorrow.

Comment 24

[Johnny Stenback (:jst)]

Um, shouldn't this be backed out until we have a fix for the tests? We don't
want tinderbox to be orange all day because of this...

Comment 25

[Vinay Badami]

Looks like this has already been abcked out by rev 1.43.
Thanks and sorry for the confusion.

Comment 26

[Tanu Mutreja]

Attachment: Fixing differently...

I tested the behavior of IE and found that earlier patch was not providing a
compatible solution, in the sense that using that patch -->

<p>abc</p>
<p>xyz</p> 

was getting pasted as:

abc
xyz

whereas IE does it as:

abc

xyz

So tried a different approach to solve this bug and make the behavior
compatible with IE. Now with every closing <p> tag, setting a new member
variable to TRUE. Later, before handling the next tag, checking if that
variable is TRUE, which actually means that addition of a line break is due. If
TRUE, adding a line break before going any further. 

With this fix, I need to modify just one TestCase "mailquote.out". In
"mailquote.out", <P> is the last tag before </body></html>. So as there is no
content after <P>, there should be just one new line at the end. Next I shall
be attaching diff for "mailquote.out" too.

Comment 27

[Tanu Mutreja]

Attachment: Diff for testcase "mailquote.out"

Removing a new line from the end.

Comment 28

[Daniel Bratell]

Comment on attachment 70285 [details] [diff] [review]
Fixing differently...

This is something that should have been fixed long ago. I'm glad that you're
looking at it. 

But this problem isn't limited to <p>, is it? The same problem should be there
for all block level tags, so it would better to not seperate out <p>. Instead
you should use the same solution for all blocks. I never see a reason to add a
newline unless there's something else coming afterwards, or am I mistaken?

Comment 29

[Joe Francis]

Tamu, you may want to look at what I did for bug 98572, which is a little more
general and allows for arbitrary amounts of vertical whitespace.

Comment 30

[Tanu Mutreja]

Attachment: Generalising the solution...

Generalizing the solution. 
As other closing tags are handled in specific way based on the mFlag value,
this is the only way I could find for generic solution.
(It passes the test cases successfully)

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

I like the generalized version, the code is pretty straightforward.  For
whatever it's worth, r=[e-mail]

Comment 32

[Daniel Bratell]

Comment on attachment 71662 [details] [diff] [review]
Generalising the solution...

Yes, this looks fine. There is only one thing that I'm not sure of. You set
mFloatingLines to 0 after a </li> and then, if it's already set to 1 after
</pre> (for instance), you miss one line break. If that is so (I think it is),
you should check and set mFloatingLines only if it is less than 0. That would
also require you to reset mFloatingLines to -1 after using it.

Otherwise, and with the above considered r=[e-mail]

Comment 33

[Tanu Mutreja]

Daniel, I'm using mFloatingLines just to pass a varying value to 
EnsureVerticalSpace(). Inside EnsureVerticalSpace(), noOfRows are compared to 
mEmptyLines. So it should not miss required new lines.
Please see if I'm missing something!!!

Comment 34

[Tanu Mutreja]

*** Bug 98572 has been marked as a duplicate of this bug. ***

Comment 35

[Daniel Bratell]

Scenario:
You get a </pre> : mLineBreakIsDue = TRUE, mFloatingLines = 1

Then, directly you get a </li> : mLineBreakIsDue = TRUE, mFloatingLines = 0

Then you get a start tag, since mLineBreakIsDue = TRUE, you call
EnsureVerticalSpace(mFloatingLines), i.e. EnsureVerticalSpace(0). 

EnsureVerticalSpace compares the number of current line breaks to 0 and ensures
that we are on a new line. But since we had a </pre> we should have called
EnsureVericalSpace(1). The </li> made us forget that.

Comment 36

[Tanu Mutreja]

Attachment: Including Daniel's Suggestions.

o. Before setting mFloatingLines to 0, included a check if (mFloatingLines <
0).
o. Setting mFloatingLines to -1 everytime it's job is done for a new element.

Comment 37

[Tanu Mutreja]

Attachment: Test case to verify latest patch(#71863)

Comment 38

[Tanu Mutreja]

Attachment: Correct Testcase to Verify Latest patch (#71863)

Sorry, attached a wrong test case last time.

Comment 39

[Daniel Bratell]

Comment on attachment 71863 [details] [diff] [review]
Including Daniel's Suggestions.

r=[e-mail]

Comment 40

[Joe Francis]

This is going to make a lot of people happy :-)

Comment 41

[Johnny Stenback (:jst)]

Comment on attachment 71863 [details] [diff] [review]
Including Daniel's Suggestions.

sr=jst

Comment 42

[Asa Dotzler [:asa]]

Comment on attachment 71863 [details] [diff] [review]
Including Daniel's Suggestions.

a=asa (on behalf of drivers) for checkin to 0.9.9 and the trunk!

Comment 43

[Brendan Eich [:brendan]]

   // The wrap column is how many standard sized chars (western languages)
   // should be allowed on a line. There could be less chars if the chars
@@ -195,6 +196,11 @@
   PRPackedBool     mInWhitespace;
   PRPackedBool     mPreFormatted;
   PRPackedBool     mStartedOutput; // we've produced at least a character
+
+  /*While handling a new tag, this variable should remind if any line break
+  is due because of a closing tag. Setting it to "TRUE" while closing the tags.
+  Hence opening tags are guaranteed to start with appropriate line breaks.*/
+  PRPackedBool     mLineBreakDue; 
 
Use a C++ // ... comment style above, with a space between each // and the first
word after it -- in other words, imitate the multi-line // comment shown above,
in the "The wrap column ..." comment.

Thanks for fixing this, let's get it into 0.9.9.

/be

Comment 44

[Tanu Mutreja]

Attachment: With Brendan's Suggestions.

Comment 45

[Tanu Mutreja]

Fixed for trunk with details as:

C:\moz099\mozilla\content\base\src>cvs commit
cvs commit: Examining .
Checking in nsPlainTextSerializer.cpp;
/cvsroot/mozilla/content/base/src/nsPlainTextSerializer.cpp,v  <--  
nsPlainTextSerializer.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in nsPlainTextSerializer.h;
/cvsroot/mozilla/content/base/src/nsPlainTextSerializer.h,v  <--  
nsPlainTextSerializer.h
new revision: 1.17; previous revision: 1.16
done


Fixed for 099 branch with details as:

C:\moz099\mozilla\content\base\src>cvs commit
cvs commit: Examining .
Checking in nsPlainTextSerializer.cpp;
/cvsroot/mozilla/content/base/src/nsPlainTextSerializer.cpp,v  <--  
nsPlainTextSerializer.cpp
new revision: 1.45.2.1; previous revision: 1.45
done
Checking in nsPlainTextSerializer.h;
/cvsroot/mozilla/content/base/src/nsPlainTextSerializer.h,v  <--  
nsPlainTextSer
ializer.h
new revision: 1.16.10.1; previous revision: 1.16
done

Comment 46

[Håkan Waara]

*** Bug 128890 has been marked as a duplicate of this bug. ***

Comment 47

[Peter Lockhart]

Tanu: will this also fix bug 128890 ?

Comment 48

[Tanu Mutreja]

Yes Peter, it fixes 128890 also and that has been marked as a dupe of it. 

Comment 49

[sujay]

Reporter: please verify and mark verified-fixed...thanks!

Comment 50

[duane voth]

[e-mail] suggests:
> Reporter: please verify and mark verified-fixed...thanks!

wish I could!  But the Linux version (of RC2 even) has another
annoying little paste problem - after mozilla has been running
for a few days, I can only paste to xclipboard and no where
else!

Comment 51

[Matthias Versen [:Matti]]

verified fixed