938563 - Select all + copy on textarea doesn't copy linebreaks

Status: VERIFIED FIXED

Description

[Kartikaya Gupta (email:[e-mail])]

What the summary says. Note that using the "Copy All" context menu item *does* copy linebreaks, it's just select all followed by a copy that doesn't.

Comment 1

[:Margaret Leibovic]

Maybe capella would want to look into this (or make it into a mentor bug :)

Comment 2

[Mark Capella [:capella]]

Created attachment 8334588 [details] [diff] [review]
bug938563

If that's the proper behavior for textareas, we can change a function call this way ...

Comment 3

[:Margaret Leibovic]

It looks like the "Copy All" functionality does this by copying element.value (which it can do since we only show copy all for editable elements):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6236

I was wondering if we could do something similar in SelectionHandler, but I now realize we can't do this when copying a selection, unless we did some check to make sure everything is selected.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8334588 [details] [diff] [review]
bug938563

Review of attachment 8334588 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/SelectionHandler.js
@@ +335,5 @@
> +      return "";
> +
> +    if (this._targetElement instanceof Ci.nsIDOMHTMLTextAreaElement) {
> +      return selection.QueryInterface(Ci.nsISelectionPrivate).
> +        toStringWithFormat("text/plain", Ci.nsIDocumentEncoder.OutputPreformatted | Ci.nsIDocumentEncoder.OutputRaw, 0);

I've never seen this toStringWithFormat API used before, but this seems reasonable to me. I didn't test this, so please double check this works with things like shareSelection, which also call _getSelectedText() :)

I wonder if we can check the contents of the clipboard in a robocop test... it could be interesting to write a little test HTML page to load in a robocop test, then choose "select all" followed by "copy". We could also check the result of "copy all" to make sure it's the same.

Comment 5

[Mark Capella [:capella]]

I had decided this was the correct textarea behaviour based on desktop testing, and this bit of doc:
http://www.w3.org/TR/2011/WD-html5-20110525/the-button-element.html#concept-textarea-raw-value

I found the method by reading through the nsISelection code, and initially thought I'd have to expose it in the .idl alongside toString(), but realized that since it returns a NS_IMETHODIMP it had to be exposed somewhere already ... found it in nsISelectionPrivate.idl (\o/)
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp?mark=965-972#951

Fyi, tested cases: shareSelection(), searchSelection(), Find-in-Page / "TextSelection:Get" ... all seems well
:-D

Comment 6

[Mark Capella [:capella]]

Due diligence push to TRY
https://tbpl.mozilla.org/?tree=Try&rev=68f7b2deed53

Comment 7

[Mark Capella [:capella]]

then to fx-team
https://hg.mozilla.org/integration/fx-team/rev/f3d60f872576

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

This might be low-risk enough to uplift to aurora. Ask for approval?

Comment 10

[Mark Capella [:capella]]

Comment on attachment 8334588 [details] [diff] [review]
bug938563

[Approval Request Comment] Low risk impact, easy backout
Bug caused by (feature/regressing bug #): Original design
User impact if declined: Small pool of affected / annoyed / confused users
Testing completed (on m-c, etc.): Local GS3, N7, also pulled from m-c and re-tested, needs Q/A
Risk to taking this patch (and alternatives if risky): Low risk either way, nice to fix
String or IDL/UUID changes made by this patch: None

Test Case: https://www.dropbox.com/s/b7ui34myn0y78gw/bug938563.html

Comment 11

[Aaron Train [:aaronmt]]

> Testing completed (on m-c, etc.): ... needs Q/A

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b9bc63061c9c

Comment 13

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 27.0a2 (2013-12-01)
Device: LG Nexus 4
OS: Android 4.2.2