888839 - JavaScript alert loses formatting when copied and pasted

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Christian Menzel]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130625125232

Steps to reproduce:

Since Update to 23.
Copy the Text of an JS Alert seems to delete linebreaks in buffer.
After Past all Linebreaks are gone.

I Need this, so it is important for me.
And It works, very well before and in Chrome, Safari, ...

System is OSX 10.6.8.

Comment 1

[Loic]

Could you test with a clean profile, please:
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Comment 2

[Christian Menzel]

I did, and it is the same.

Comment 3

[Christian Menzel]

(In reply to Loic from comment #1)
> Could you test with a clean profile, please:
> https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-
> firefox-profiles
I did, and it is the same.

Comment 4

[mjh563]

This appears to be a regression in Firefox 23.

Steps to Reproduce:

1. Open the Web Console (Ctrl+Shift+K)
2. Type alert("1\n2\n3\n4") and press Enter
3. An alert box appears. Copy the text of the alert and paste it into a text editor

Expected Result:

1
2
3
4

Actual Result:

1 2 3 4

Comment 5

[mjh563]

Last good nightly: 2013-04-03
First bad nightly: 2013-04-04

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=97cfc16ba5dc&tochange=c232bec6974d

Comment 6

[Loic]

My guess goes to:
Drew Willcoxon — Bug 723163 - Improve about:crashes copy & paste by improving XHTML text/plain encoding. r=hsivonen,njn

Comment 7

[Christian Menzel]

First BUG I ever announced on firefox.
I am really impressed.

Your awesome in speed and working prozess!

(what do you think would happen if is about IE ;-) )

Comment 8

[Alex Keybl [:akeybl]]

adw - can you take a look at this FF23 regresssion? We've got a couple of more weeks to take a safe fix, so we'd really appreciate giving this bug priority. Thanks!

Comment 9

[Christian Menzel]

Problem seems to be gone in 23b3

Comment 10

[Loic]

(In reply to Christian Menzel from comment #9)
> Problem seems to be gone in 23b3

Not in Nightly! Are you able to repro comment #4 in Beta?

Comment 11

[Christian Menzel]

(In reply to Christian Menzel from comment #9)
> Problem seems to be gone in 23b3

Sorry! mistake: 
Some linebreaks seem to work, I don't know why:

alert(unsescape('Sehr geehrte%28r%29 X X%2C%0A%u2028%u2028Sie sind eine Kundin/ ein Kunde%2C');

Comment 12

[Christian Menzel]

yes.

Comment 13

[Loic]

Ok, so the bug isn't fixed.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Hi Christian,

Which dialogs did you notice this with? Can you elaborate on your use cases for copying/pasting text out of dialogs? It seems likely that this is a very rare use case, so I wonder whether this is worth fixing.

Comment 15

[Christian Menzel]

I am a bit confused of your question.

I use this in development prozess of backend software.
Because it is sometimes not usefull to implement ajax or lightbox or whatever in a software in development.
Sometimes these js-alert are the shortest way to do it. And sometimes it keeps this way longer as thought.

But I am confused, cause you say:
Let's keep it this way because there is only one guy using this.
But I dont't think so.
And all Other browsers I tested, don't have this Problem.
Do you Know what I mean.

I don't want highest Priority or something.
But I think this have to be fixed.
Everythink worked till 23 so I don't think this could be a extrem hard thing to fix it?

Comment 16

[Drew Willcoxon :adw]

Attachment: patch

Olli, would you mind reviewing?  I'd ask Henri but he's away until 7/29 and we'd like to backport a fix ASAP.

Bug 723163 changed nsHTMLCopyEncoder::SetSelection slightly so that it's possible for the caller to tell why the selection forced text/plain.  It does this by delaying setting its mime type to text/plain from SetSelection to encoding time when the doc is not an nsIHTMLDocument or not HTML.  The alert() box is XUL, so when SelectionCopyHelper checks the encoder's mime type after SetSelection, it's not text/plain, so selForcedTextPlain is false, and SelectionCopyHelper pretty prints the alert()'s selected text.  This patch adds another check to SelectionCopyHelper after encoding and sets selForcedTextPlain = true if the doc is not an nsIHTMLDocument.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 775080 [details] [diff] [review]
patch

This really needs some tests, so that we don't regress the behavior.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

But other than that, this should be ok.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Would be great to get this into the second beta build this week (going to build tomorrow) if you can get an r+ and land to trunk.

Comment 20

[Drew Willcoxon :adw]

I'll try to finish the test(s) and get an r+ before tomorrow but I'm not confident both those things will happen.

Comment 21

[Drew Willcoxon :adw]

Attachment: patch with XUL test

This includes one test that verifies copying and pasting in a XUL doc results in verbatim, non-pretty-printed text/plain.

There's an existing test_copypaste.html.  I tried making a new test based on it but using XHTML.  Such a test should pass everything the HTML test does except for text/html encodings, but that turned out not to be the case.  There are many divs in that test hidden by display:none or visibility:hidden, and when they're copied and pasted, text/unicode ends up not being on the clipboard because the doc encoder in SelectionCopyHelper produces empty strings.  There must be a difference somewhere between how HTML and XHTML are serialized with regard to hidden content, which is something neither this bug nor bug 723163 touches on, and I don't want to track that down right now.

Comment 22

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #21)
> There must be a difference somewhere between how HTML and XHTML
> are serialized with regard to hidden content

Hmm, that's not quite right.  It's true that when content is hidden, the encoder produces empty strings for text/plain for XHTML, but it's also true for HTML.  SelectionCopyHelper doesn't put text/unicode on the clipboard in either case, but somehow text/unicode ends up on the clipboard for HTML.  It looks like that happens because for HTML, unlike XHTML, SelectionCopyHelper puts text/_moz_htmlcontext and text/_moz_htmlinfo on the clipboard, which causes nsClipboard::HasDataMatchingFlavors to return true for text/unicode, and which nsTransferable::GetTransferData is able to convert to text/unicode.

Comment 23

[Drew Willcoxon :adw]

Attachment: alternative patch with test_copypaste.xhtml also

Here's an alternative patch that's the same as the other one but adds test_copypaste.xhtml, which is just an XHTML version of test_copypaste.html.  It tests bug 723163.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

How is that alternative then? Looks like it has additional test, but is otherwise the same.
I guess I should review the latter patch.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 778222 [details] [diff] [review]
alternative patch with test_copypaste.xhtml also

"not an nsIHTMLDocument or not HTML" is odd.
Just drop the "or not HTML" and the rest of the comment is a bit odd too.

So this patch brings back part of the functionality that got lost in Bug 723163.
(there used to be the QI to nsIHTMLDocument)

Not happy with code duplication for the test. Could you perhaps check if
it was possible to reuse the code, like moving it to a .js file.
But r+, I guess.

Comment 26

[Drew Willcoxon :adw]

Attachment: updated patch with tests

https://tbpl.mozilla.org/?tree=Try&rev=2da1594d024d

(In reply to Olli Pettay [:smaug] from comment #24)
> How is that alternative then? Looks like it has additional test, but is
> otherwise the same.

Like I said.  It's an alternative in that there are two and you can choose the one that suits you.  Thanks.

(In reply to Olli Pettay [:smaug] from comment #25)
> "not an nsIHTMLDocument or not HTML" is odd.

It accurately describes what the code does: !doc->QI(nsIHTMLDocument) || !doc->IsHTML().  But to be clearer I replaced "not HTML" with "XHTML", which it seems is what !IsHTML() practically means.

> Just drop the "or not HTML" and the rest of the comment is a bit odd too.

I simplified the rest, so hopefully it's clearer now.

> Not happy with code duplication for the test. Could you perhaps check if
> it was possible to reuse the code, like moving it to a .js file.

Done.

> But r+, I guess.

Don't let me twist your arm now.

Comment 27

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7d1f4496ff

Comment 28

[Ed Morley [:emorley]]

Backed out for B2G failures in test_copypaste.xhtml:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25523402&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/646cba66eb0c

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Is another attempt in the works? We'll want this in our second-to-last Beta that goes to build this Thursday in order to ensure we don't ship this regression that's new to FF23.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Fixed for 23 by bug 897729

Comment 31

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0

Verified fixed on 23 beta 9 (buildID: 20130725195523).

Comment 32

[Christian Menzel]

THANK YOU.

Comment 33

[Drew Willcoxon :adw]

Attachment: patch with tests disabled on b2g and Android

Oh, the existing test_copypaste.html is disabled on b2g [1], so my new test_copypaste.xhtml should be, too.  And I presume the new xul test also.  test_copypaste.html is disabled on Android as well, so this disables the new tests there also.

I'll land this when I get a chance.

https://tbpl.mozilla.org/?tree=Try&rev=526b4ff5611c

[1] http://mxr.mozilla.org/mozilla-central/search?string=test_copypaste.html

Comment 34

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb05c6b8ae8e

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb05c6b8ae8e

Comment 36

[Drew Willcoxon :adw]

Attachment: beta (mozilla 24) patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 723163
User impact if declined: described in comment 0; if declined, must back out bug 723163 on beta
Testing completed (on m-c, etc.): automated testing on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Only change from the mozilla-inbound patch is to accommodate small changes in b2g and Android mochitest exclude files:

> patching file testing/mochitest/android.json
> Hunk #1 succeeded at 8 with fuzz 2 (offset 0 lines).
> patching file testing/mochitest/androidx86.json
> Hunk #1 FAILED at 6
> 1 out of 1 hunks FAILED -- saving rejects to file testing/mochitest/androidx86.json.rej
> patching file testing/mochitest/b2g.json
> Hunk #1 FAILED at 154
> 1 out of 1 hunks FAILED -- saving rejects to file testing/mochitest/b2g.json.rej

Comment 37

[Drew Willcoxon :adw]

Attachment: Aurora (mozilla 25) patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 723163
User impact if declined: described in comment 0
Testing completed (on m-c, etc.): automated testing on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

This is the same as the beta patch but with one small refresh to make it apply to Aurora squeakily cleanly:

> patching file testing/mochitest/b2g.json
> Hunk #1 succeeded at 158 with fuzz 2 (offset 35 lines).

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can we have a bug to enable those tests on b2g/android, and can we have that bug listed in the manfiests?

Comment 39

[Drew Willcoxon :adw]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8dceda8a125
https://hg.mozilla.org/releases/mozilla-beta/rev/b3050d05dc96

Comment 40

[Drew Willcoxon :adw]

(In reply to :Ms2ger from comment #38)
> Can we have a bug to enable those tests on b2g/android, and can we have that
> bug listed in the manfiests?

I filed bug 904183, but since the manifests are JSON, and JSON doesn't have comments in the usual sense, I didn't list the bug in the manifests.

Comment 41

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0

Verified as fixed on Firefox 24 beta 4 (buildID: 20130819170952) and latest Nightly (buildID: 20130820030206).

Comment 42

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed with latest Aurora 25.0a2 (Build ID: 20130909004001).

Comment 43

[Simona Badau, Desktop QA]

Verified as fixed on the latest Aurora 26.0a2 on Windows 7, Ubuntu 13.04 and Mac OS X 10.8.4:
Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20131014004003