Closed
Bug 938563
Opened 7 years ago
Closed 7 years ago
Select all + copy on textarea doesn't copy linebreaks
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Tracking
(firefox27 verified, firefox28 verified)
VERIFIED
FIXED
Firefox 28
People
(Reporter: kats, Assigned: capella)
References
()
Details
Attachments
(1 file)
1.47 KB,
patch
|
Margaret
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Maybe capella would want to look into this (or make it into a mentor bug :)
Assignee | ||
Comment 2•7 years ago
|
||
If that's the proper behavior for textareas, we can change a function call this way ...
Attachment #8334588 -
Flags: review?(margaret.leibovic)
Comment 3•7 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•7 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•7 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•7 years ago
|
||
Due diligence push to TRY https://tbpl.mozilla.org/?tree=Try&rev=68f7b2deed53
Assignee | ||
Comment 7•7 years ago
|
||
then to fx-team https://hg.mozilla.org/integration/fx-team/rev/f3d60f872576
Comment 8•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f3d60f872576
Assignee: nobody → markcapella
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Comment 9•7 years ago
|
||
This might be low-risk enough to uplift to aurora. Ask for approval?
Assignee | ||
Comment 10•7 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?
Comment 11•7 years ago
|
||
> Testing completed (on m-c, etc.): ... needs Q/A
Updated•7 years ago
|
Attachment #8334588 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b9bc63061c9c
status-firefox27:
--- → fixed
Comment 13•7 years ago
|
||
Verified fixed on: Build: Firefox for Android 27.0a2 (2013-12-01) Device: LG Nexus 4 OS: Android 4.2.2
Updated•2 months ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•