Closed Bug 938563 Opened 9 years ago Closed 9 years ago

Select all + copy on textarea doesn't copy linebreaks


(Firefox for Android Graveyard :: Text Selection, defect)

28 Branch
Not set


(firefox27 verified, firefox28 verified)

Firefox 28
Tracking Status
firefox27 --- verified
firefox28 --- verified


(Reporter: kats, Assigned: capella)





(1 file)

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.
Maybe capella would want to look into this (or make it into a mentor bug :)
Attached patch bug938563Splinter Review
If that's the proper behavior for textareas, we can change a function call this way ...
Attachment #8334588 - Flags: review?(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):

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 on attachment 8334588 [details] [diff] [review]

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.
Attachment #8334588 - Flags: review?(margaret.leibovic) → review+
I had decided this was the correct textarea behaviour based on desktop testing, and this bit of doc:

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/)

Fyi, tested cases: shareSelection(), searchSelection(), Find-in-Page / "TextSelection:Get" ... all seems well
Assignee: nobody → markcapella
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
This might be low-risk enough to uplift to aurora. Ask for approval?
Comment on attachment 8334588 [details] [diff] [review]

[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:
Attachment #8334588 - Flags: approval-mozilla-aurora?
> Testing completed (on m-c, etc.): ... needs Q/A
Attachment #8334588 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on:
Build: Firefox for Android 27.0a2 (2013-12-01)
Device: LG Nexus 4
OS: Android 4.2.2
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.