235937 - window.getSelection() loses newlines in preformatted text

Status: NEW

(Core :: DOM: Selection, defect)

Description

[Joel Peshkin]

window.getSelection() will return a string with embedded newlines if the
selection contains multiple paragraphs or lines seperated by <br>

However, if multiple lines of a <pre> section (such as a Bugzilla comment) are
selected, window.getSelection() changes the embedded newlines to whitespace.

This differs from the behavior when copying the same text to the windows clipboard.

Problem seen on:
Mozilla 1.5
Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.5) Gecko/20031007

Comment 1

[Joel Peshkin]

Incientally, IE5's document.selection.createRange().text also does this properly.

Comment 2

[Joel Peshkin]

Attachment: Example showing problem

Comment 3

[Joel Peshkin]

The example showing the problem is now attached.  Note that it works just fine
in IE.

Comment 4

[Joel Peshkin]

Same problem on..

Mozilla 1.6
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

Comment 5

[Christian Reis]

When using document.getSelection() on Netscape4, the testcase works as expected:
preformatted lines are wrapped at their line breaks.

Moving OS to All since I can reproduce this on Linux too.

Comment 6

[Christian Reis]

It's also interesting that the X clipboard pasting works as you'd expect, so
window.getSelection() behaves differently from it (and I can't figure out why)

Comment 7

[James Ross]

Had a little go with the attachement, and noticed something interesting: if I
start my selection in the non-pre bit, and include the pre, it gets it right
(newlines). It's only if I only include the pre bit in my selection that it
flattens them to whitespace. Build IDs 2003120808 and 2004031208.

Comment 8

[[e-mail]]

OK, so the issue is that nsDocumentEncoder doesn't take context into account.
I wonder what will break if I change that...

Comment 9

[[e-mail]]

Attachment: Proposed patch

Comment 10

[Christian Reis]

Comment on attachment 143721 [details] [diff] [review]
Proposed patch

>Index: nsDocumentEncoder.cpp
>@@ -795,10 +798,13 @@ nsDocumentEncoder::SerializeRangeContext
> 
>     if (IncludeInContext(node)) {
>       rv = SerializeNodeEnd(node, aString);
>-
>-      if (NS_FAILED(rv))
>-        break;
>+    } else {
>+      nsString dummyString;
>+      rv = SerializeNodeEnd(node, aString);

You might as well use the dummyString you're creating..

What's the rationale in providing the dummy to SerializeNode*?

Comment 11

[[e-mail]]

Attachment: Proposed patch

Oops :-[

The idea is that the serializer needs to know the context in order to correctly
serialize, although the context should not appear in the output.

Comment 12

[Christian Reis]

I guess it might be cleaner if we could somehow tell SerializeNode* that we
didn't want any output generated..

Akkana, how does this look to you? 

Comment 13

[Akkana Peck]

I'm bothered by the idea of doing this extra serialization that we're going to
throw away.  What is it about SerializeNodeStart and SerializeNodeEnd that they
have to be called even though their output isn't used?

Comment 14

[[e-mail]]

akk: the serializer is only being shown the text node, but it needs to know
about the pre in the context so that it doesn't do whitespace conversion.

Comment 15

[Akkana Peck]

Talked with Neil briefly this morning ... turns out that mPreLevel is the side
effect we're after, but the calling code can't just set mPreLevel since it's
general and doesn't know whether it spans a pre.

I'm amenable to this idea, I just want to get clear on a few points.
I'm still unclear on what goes into the dummy string (if anything) and then gets
thrown away.  If the pre is very large, and the selection only spans part of it,
are we going to end up allocating tons of memory for text we don't actually need?

Also, it would be nice to see a comment before the dummyString lines explaining
briefly what they're for.

Comment 16

[[e-mail]]

(In reply to comment #15)
>If the pre is very large, and the selection only spans part of it, are we
>going to end up allocating tons of memory for text we don't actually need?
I only send the context elements to the dummy string, not the whole text.

>Also, it would be nice to see a comment before the dummyString lines
>explaining briefly what they're for.
Can do.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 143738 [details] [diff] [review]
Proposed patch

+      nsString dummyString;
+      rv = SerializeNodeStart(node, 0, -1, dummyString);

dummyString should only contain the opening tag for node here, right? If so,
seems likely that that would fit in an nsAutoString, any reason not to use
nsAutoString here? Same when closing this at the end...

sr=jst

Comment 18

[Akkana Peck]

Comment on attachment 143738 [details] [diff] [review]
Proposed patch

r=akkana provided that the promised comment materializes.

Comment 19

[[e-mail]]

Attachment: Fix context issues

Aargh! I am plagued with regressions :-( Why didn't I notice them before?

Comment 20

[[e-mail]]

Comment on attachment 145088 [details] [diff] [review]
Fix context issues

Sigh, so there were some issues with the previous patch...

>-  virtual PRBool IncludeInContext(nsIDOMNode *aNode);
>+  static PRBool IncludeInContext(nsIDOMNode *aNode);
>+
>+  virtual PRBool CopyContext();
The plain text encoder doesn't like being given too much context, so now I give
it the same context nodes as the HTML copy encoder. Instead I created a new
virtual function which is used to determine whether this is the HTML copy
encoder or the plain text encoder.

>+  // The HTML Copy encoder needs to save the context.
>+  // The plain text encoder only needs to be aware of the context,
>+  // so that it can correctly serialize preformatted text.
>+  nsAutoString dummyString;
>+  nsAString& contextString = CopyContext() ? aString : dummyString;
So now I serialize to either the input string or the dummy string.

>+
>+  rv = mSerializer->Flush(aOutputString);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
The plain text encoder needs to be poked because it tries to delay the output
until it sees the context, which is no use in our case.

Let me know if I'm going about this in completely the wrong way ;-)

Comment 21

[Vladimir Yeryomin]

This testcase is not work on Firefox 1.5: "window.getSelection()" return empty string. See bug #317914.

Comment 22

[Daniel Veditz [:dveditz]]

Need a reviewed and tested patch checked into the trunk before we would even consider non-critical fixes issues for a .0.1 release.

Comment 23

[Darin Fisher]

Not going to block 1.8.1 for this, but we'd happily consider a patch once it has baked on the trunk.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.