Select all + copy on textarea doesn't copy linebreaks

VERIFIED FIXED in Firefox 27

Status

()

Firefox for Android
Text Selection
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: kats, Assigned: capella)

Tracking

28 Branch
Firefox 28
Other
Android
Points:
---

Firefox Tracking Flags

(firefox27 verified, firefox28 verified)

Details

(URL)

Attachments

(1 attachment)

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

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

Comment 2

5 years ago
Created attachment 8334588 [details] [diff] [review]
bug938563

If that's the proper behavior for textareas, we can change a function call this way ...
Attachment #8334588 - Flags: review?(margaret.leibovic)

Comment 3

5 years ago
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

5 years ago
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.
Attachment #8334588 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 5

5 years ago
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
(Assignee)

Comment 6

5 years ago
Due diligence push to TRY
https://tbpl.mozilla.org/?tree=Try&rev=68f7b2deed53
https://hg.mozilla.org/mozilla-central/rev/f3d60f872576
Assignee: nobody → markcapella
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
This might be low-risk enough to uplift to aurora. Ask for approval?
(Assignee)

Comment 10

5 years ago
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
Attachment #8334588 - Flags: approval-mozilla-aurora?
> Testing completed (on m-c, etc.): ... needs Q/A
Status: RESOLVED → VERIFIED
status-firefox28: --- → verified

Updated

5 years ago
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
status-firefox27: fixed → verified
You need to log in before you can comment on or make changes to this bug.