Closed Bug 888839 Opened 6 years ago Closed 6 years ago

JavaScript alert loses formatting when copied and pasted

Categories

(Core :: Serializers, defect)

23 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox22 --- unaffected
firefox23 + verified
firefox24 + verified
firefox25 + verified
firefox26 + verified

People

(Reporter: it, Assigned: adw)

References

Details

(Keywords: regression)

Attachments

(3 files, 4 obsolete files)

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.
Component: Untriaged → General
Hardware: x86 → x86_64
Whiteboard: JS Alert, Clipboard, Linebreak loss
Component: General → Untriaged
Could you test with a clean profile, please:
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles
Flags: needinfo?(it)
Whiteboard: JS Alert, Clipboard, Linebreak loss
I did, and it is the same.
Flags: needinfo?(it)
(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.
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
Last good nightly: 2013-04-03
First bad nightly: 2013-04-04

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=97cfc16ba5dc&tochange=c232bec6974d
My guess goes to:
Drew Willcoxon — Bug 723163 - Improve about:crashes copy & paste by improving XHTML text/plain encoding. r=hsivonen,njn
Status: UNCONFIRMED → NEW
Component: Untriaged → Serializers
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
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 ;-) )
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!
Assignee: nobody → adw
Problem seems to be gone in 23b3
(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?
(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');
yes.
Ok, so the bug isn't fixed.
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.
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?
Summary: Java Script Alert losses formatting when copy and past → JavaScript alert loses formatting when copied and pasted
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #775080 - Flags: review?(bugs)
Depends on: 723163
Comment on attachment 775080 [details] [diff] [review]
patch

This really needs some tests, so that we don't regress the behavior.
Attachment #775080 - Flags: review?(bugs) → review-
But other than that, this should be ok.
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.
Flags: needinfo?(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.
Flags: needinfo?(adw)
Attached patch patch with XUL test (obsolete) — Splinter Review
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.
Attachment #775080 - Attachment is obsolete: true
Attachment #777513 - Flags: review?(bugs)
(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.
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.
Attachment #778222 - Flags: review?(bugs)
How is that alternative then? Looks like it has additional test, but is otherwise the same.
I guess I should review the latter patch.
Attachment #777513 - Flags: review?(bugs)
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.
Attachment #778222 - Flags: review?(bugs) → review+
Attached patch updated patch with tests (obsolete) — Splinter Review
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.
Attachment #777513 - Attachment is obsolete: true
Attachment #778222 - Attachment is obsolete: true
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7d1f4496ff
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
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.
Flags: needinfo?(adw)
Blocks: 723163
No longer depends on: 723163
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).
THANK YOU.
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
Attachment #778747 - Attachment is obsolete: true
Flags: needinfo?(adw)
https://hg.mozilla.org/mozilla-central/rev/cb05c6b8ae8e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
[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
Attachment #788459 - Flags: approval-mozilla-beta?
[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).
Attachment #788460 - Flags: approval-mozilla-aurora?
Can we have a bug to enable those tests on b2g/android, and can we have that bug listed in the manfiests?
Flags: needinfo?(adw)
Attachment #788459 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #788460 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 904183
(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.
Flags: needinfo?(adw)
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).
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).
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
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.