Closed Bug 1731934 Opened 3 years ago Closed 3 years ago

Copy action in a text field is not working, but cut and paste actions are

Categories

(Thunderbird :: Preferences, defect)

Product:
Thunderbird
Component:
Preferences
Type:
defect

Tracking

(thunderbird_esr91 fixed, thunderbird98 verified)

Status:
VERIFIED FIXED
Milestone:
99 Branch
Tracking Flags:
Tracking Status
thunderbird_esr91 --- fixed
thunderbird98 --- verified

People

(Reporter: darktrojan, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

1731934-copy-only-from-messages.patch - even better patch ;-)
(deleted), patch
darktrojan
: review+
wsmwk
: approval-comm-beta+
wsmwk
: approval-comm-esr91+
Details | Diff | Splinter Review

(I'm filing this in Thunderbird because I can't think of a good way to reproduce it in Firefox, but assume it's a toolkit problem.)

If you go to (for example) the preferences tab, type some text into the Find in Preferences field, then try to copy the text, nothing is copied to the clipboard. The copy action happens, but the contents of the clipboard are now empty.

Cutting or pasting in the same text field works.

The same thing happens in other text fields. The same thing happens whether you use the keyboard or the context menu (from editMenuOverlay.js). I tried it on my Mac VM and could reproduce it, but on my Windows VM I could not reproduce it. It goes back to at least as early as 91.

Masayuki, I assume there's something odd going on in the core copy command. Possibly related to these pages being content not chrome. Any ideas?

Flags: needinfo?(masayuki)

Thanks Alice!

Keywords: regressionwindow-wanted
status-thunderbird94: --- → affected
Component: General → Preferences

On Windows, we had similar issue but randomly (bug 1726269). I have no idea what's going on. Do you see some warnings under editor/libeditor? Otherwise, I have no idea.

Flags: needinfo?(masayuki) → needinfo?(geoff)

No, I don't see any warnings. It's interesting that you say it was random before. When this was first reported, it wasn't happening to me, but now it is.

Flags: needinfo?(geoff)

Strange thing is ,
In version 91 of the release build, removing the img.handlerSortHeaderIcon element of Files & Attachments section via the Developer toolbox solves the problem.
However, in the latest Daily94a1 the problem persists.

(In reply to Geoff Lankow (:darktrojan) from comment #4)

No, I don't see any warnings. It's interesting that you say it was random before. When this was first reported, it wasn't happening to me, but now it is.

when I do ctrl+C I see "This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”."

In 91 on Windows, it's not random at all. It fails every time. If you look at the Windows clipboard after the copy operation, you get:

Version:0.9
StartHTML:00000097
EndHTML:00000298
StartFragment:00000131
EndFragment:00000262
<html><body>
<!--StartFragment--><html:input xmlns:html="http://www.w3.org/1999/xhtml" id="mailnewsStartPageUrl" type="text" preference="mailnews.start_page.url" /><!--EndFragment-->
</body>
</html>

So instead of copying the content of the text box, it copies the HTML of the text box onto the clipboard.

There is something fundamentally different between the preferences page in FF 91 and TB 91. In FF you can select all the text of the page, in TB you can't select any (which is impracticable if you want to quote text for support cases). So the page is structured differently and hence the copy behaviour is different.

The difference is that FX has

* {
  user-select: text;
}

Which needs on other places a reversion to still work normally. Adding this to TB's preferences.css doesn't fix the copy issue.

Does this repro on Linux by any chance?

On TB 91 on Linux, the copy/paste is equally broken and reproducible 100%. Right now we can't find a tool equivalent to the one we have for Windows (https://freeclipboardviewer.com/) that would show all the different flavours of the clipboard. Generally we'd expect the failure to be the same, that is, the text/html (generic) flavour (and on Windows, the HTML flavour (CF_HTML, Windows specific)) to be filled and the a paste-able text flavour to be cleared.

Even without an external clipboard viewer, you can use the "Mozilla toolbox". Issue:

console.log(Services.clipboard.hasDataMatchingFlavors(["text/html"], Ci.nsIClipboard.kGlobalClipboard))

and you'll see that after a copy a text/html flavour is present, after a cut, it is not. Surely there is a way to get to the content of that flavour, too, but by the looks of it, that is no longer a one-liner.

It's easy enough to reproduce the failure. In the TB preferences/General tab, just copy anything from the Start Page Location box or the Find box at the very top.

I can try to repro on daily when I have some time, thanks. Fwiw KDE has s clipboard viewer (Klipper) which should do.

Flags: needinfo?(emilio)

Here's the poor man's version of a clipboard viewer (adapted from here):

function getHTMLFromClipboard() {
  let trans = Cc["@mozilla.org/widget/transferable;1"].createInstance(
    Ci.nsITransferable
  );
  trans.init(null);
  trans.addDataFlavor("text/html");
  Services.clipboard.getData(trans, Services.clipboard.kGlobalClipboard);
  let str = {};
  trans.getTransferData("text/html", str);
  if (str) {
    str = str.value.QueryInterface(Ci.nsISupportsString);
  }
  if (str) {
    console.log(str.data);
  }
}

So STR:

  1. Paste this code into the console.
  2. Select text in the the preferences pane, copy.
  3. Execute function (without copy/pasting its name!).

Result on Linux:

<html:input xmlns:html="http://www.w3.org/1999/xhtml" id="mailnewsStartPageUrl" type="text" preference="mailnews.start_page.url" />

I can't repro on a clean daily build from current central. Can someone repro on daily? I see some console warnings in selection code, but maybe it's been fixed already?

Flags: needinfo?(emilio)

Thanks for looking, Emilio, and big apologies for sending you on a wild goose chase :-(
We tried an "old" Daily of 12-Jan-2022 and there the problems was already fixed. It still happens in TB 97 beta, and that was branched off on 10-Jan-2022. However, using mozregression, all the Dailies we tried, for example 97.0a1 (2022-01-06) (64-bit) or even from last year after the bug was reported, were good. Looks like a beta/ESR-only issue.

Alice, could you please look at this. STR:
Open the preferences pane in TB.
Copy (not cut) the text from the Start Page location.
Paste into Notepad/Notepad++.
Good result: The copied text is pasted. Bad result: Nothing is pasted.

Not working on TB 91, 97 beta, working on various Dailies we tried, even the 97 ones.

Flags: needinfo?(alice0775)

On Daily98.0a1(20220131105552) Windows10,
After starting with a newly created profile, if none of mail accounts are set up, the copy function works as expected with the STR in comment#16.
However, if you set up even a single email account, the problem is reproduced. It is very strange.

Flags: needinfo?(alice0775)

Thanks, Alice. We ran mozregression without setting up a profile. That pointed to the following for the fix, curiously enough related to running without account:
https://hg.mozilla.org/comm-central/rev/f4b2344a503d564c64a15e51e3ccd009bbb46ba3 (bug 942615).

However, Alice is right, when you set up an e-mail account, the problem is reproduced even in the newest Daily.

Emilio, can you reproduce the issue if you set up an account? Hopefully you haven't trashed your build yet.

Flags: needinfo?(emilio)

Building is the least of the problems. I can try to take a look tomorrow :)

Wow. That explains comment #6 that removing the back then only image "fixed" the issue since this runs in a loop over the images:
https://searchfox.org/comm-central/rev/4053c87e5ec3808d1f10a63b7678429334114391/mail/base/content/mailWindow.js#104
Can you share your debugging technique on how you located this? Somehow watch the clipboard?

Attached patch patch (obsolete) (deleted) — — Splinter Review

This is a hopefully trivially-correct fix for the most common situation.

We could also check something like e.target.editor and bail out, but I'm not sure whether that piece of code should handle contenteditable / html editing in some sense. Presumably it doesn't because it won't use the right selection (getSelection() won't be the editor's selection). But anyways I digress.

Assignee: nobody → emilio
Attachment #9262000 - Flags: review?(richard.marti)

(In reply to Rachel Martin from comment #21)

Can you share your debugging technique on how you located this? Somehow watch the clipboard?

MOZ_LOG=WidgetClipboard:5 mach run --debugger=rr

After I reproduced the bug, rr replay -a -M >log 2>&1. The log file has stuff like:

[rr 266986 107897][Parent 266986: Main Thread]: D/WidgetClipboard nsClipboard::SetData (primary)
[rr 266986 107901][Parent 266986: Main Thread]: D/WidgetClipboard     processing target text/unicode
[rr 266986 107905][Parent 266986: Main Thread]: D/WidgetClipboard     adding TEXT targets
[rr 266986 107972][Parent 266986: Main Thread]: D/WidgetClipboard doing iteration 2 msec 12 ...
[rr 266986 107976][Parent 266986: Main Thread]: D/WidgetClipboard wayland_clipboard_contents_received_async() selection_data = 7fffc8aa4810
[rr 266986 107980][Parent 266986: Main Thread]: D/WidgetClipboard nsRetrievalContextWayland::TransferClipboardData(), aSelectionData = 7fffc8aa4810
[rr 266986 107984][Parent 266986: Main Thread]: D/WidgetClipboard     request number matches
[rr 266986 107988][Parent 266986: Main Thread]: D/WidgetClipboard     getting 24 bytes of clipboard targets.
[rr 266986 107992][Parent 266986: Main Thread]: D/WidgetClipboard     Clipboard content (target nums 3):
[rr 266986 107996][Parent 266986: Main Thread]: D/WidgetClipboard         MIME text/plain
[rr 266986 108000][Parent 266986: Main Thread]: D/WidgetClipboard         MIME text/html
[rr 266986 108004][Parent 266986: Main Thread]: D/WidgetClipboard         MIME text/plain;charset=utf-8
[rr 266986 108008][Parent 266986: Main Thread]: D/WidgetClipboard     Asking for content:
[rr 266986 108012][Parent 266986: Main Thread]: D/WidgetClipboard         MIME text/unicode
[rr 266986 108016][Parent 266986: Main Thread]: D/WidgetClipboard     has kUnicodeMime

Then go to the relevant nsClibpoard::SetData call (via rr replay -p 266986 -g 107897).

Inspect the relevant DataTransfer object put a watchpoint on the relevant DataTransferItem s data member (`watch -l , then reverse-continue). I got to this stack:

#0  nsCOMPtr<nsIVariant>::assign_assuming_AddRef(nsIVariant*) (this=0x7f4edd5c5090, aNewPtr=0x7f4ed499e150) at /home/emilio/src/moz/mozilla-unified/obj-debug/dist/include/nsCOMPtr.h:423
#1  0x00007f4f210d5e44 in nsCOMPtr<nsIVariant>::assign_with_AddRef(nsISupports*) (this=0x7f4edd5c5090, aRawPtr=0x7f4ed499e150) at /home/emilio/src/moz/mozilla-unified/obj-debug/dist/include/nsCOMPtr.h:1179
#2  nsCOMPtr<nsIVariant>::operator=(nsIVariant*) (this=0x7f4edd5c5090, aRhs=0x7f4ed499e150) at /home/emilio/src/moz/mozilla-unified/obj-debug/dist/include/nsCOMPtr.h:690
#3  mozilla::dom::DataTransferItem::SetData(nsIVariant*) (this=0x7f4edd5c5040, aData=0x7f4ed499e150) at /home/emilio/src/moz/mozilla-unified/dom/events/DataTransferItem.cpp:102
#4  0x00007f4f210d8611 in mozilla::dom::DataTransferItemList::AppendNewItem(unsigned int, nsTSubstring<char16_t> const&, nsIVariant*, nsIPrincipal*, bool)
    (this=this@entry=0x7f4ed4dfca60, aIndex=0, aType=<optimized out>, aData=0x7f4ed499e150, aPrincipal=<optimized out>, aHidden=false) at /home/emilio/src/moz/mozilla-unified/dom/events/DataTransferItemList.cpp:406
#5  0x00007f4f210d3d9e in mozilla::dom::DataTransferItemList::SetDataWithPrincipal(nsTSubstring<char16_t> const&, nsIVariant*, unsigned int, nsIPrincipal*, bool, bool, mozilla::ErrorResult&)
    (this=0x7f4ed4dfca60, aType=<optimized out>, aData=0x7f4f2bb22c00, aData@entry=0x7f4ed499e150, aIndex=3566854480, aIndex@entry=0, aPrincipal=aPrincipal@entry=0x7f4f09b04500, aInsertOnly=<optimized out>, aHidden=false, aRv=...)
    at /home/emilio/src/moz/mozilla-unified/dom/events/DataTransferItemList.cpp:385
#6  0x00007f4f210ceb06 in mozilla::dom::DataTransfer::SetDataWithPrincipal(nsTSubstring<char16_t> const&, nsIVariant*, unsigned int, nsIPrincipal*, bool) (this=this@entry=0x7f4ed4d2f200, aFormat=..., aData=0x7f4f2bb22c00, 
    aData@entry=0x7f4ed499e190, aIndex=1574111656, aIndex@entry=3366601456, aPrincipal=0x4e905ebac4e9, aPrincipal@entry=0x7f4ed499e150, aHidden=false) at /home/emilio/src/moz/mozilla-unified/dom/events/DataTransfer.cpp:1245
#7  0x00007f4f210d032d in mozilla::dom::DataTransfer::SetDataAtInternal(nsTSubstring<char16_t> const&, nsIVariant*, unsigned int, nsIPrincipal*)
    (this=this@entry=0x7f4ed4d2f200, aFormat=<optimized out>, aData=<optimized out>, aIndex=aIndex@entry=0, aSubjectPrincipal=aSubjectPrincipal@entry=0x7f4f09b04500)
    at /home/emilio/src/moz/mozilla-unified/dom/events/DataTransfer.cpp:721
#8  0x00007f4f210c0e10 in mozilla::dom::DataTransfer::SetData(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIPrincipal&, mozilla::ErrorResult&)
    (this=0x7f4edd5c5090, aFormat=..., aData=<optimized out>, aSubjectPrincipal=..., aRv=...) at /home/emilio/src/moz/mozilla-unified/dom/events/DataTransfer.cpp:396
#9  0x00007f4f20b38b1c in mozilla::dom::DataTransfer_Binding::setData(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)Traceback (most recent call last):
  File "/home/emilio/src/moz/mozilla-unified/js/src/gdb/mozilla/Root.py", line 55, in to_string
    ptr = ptr.dereference()
gdb.error: value has been optimized out
 (cx=0x7f4f09623200, obj=
, void_self=0x7f4edd5c5090, args=<optimized out>) at DataTransferBinding.cpp:503
#10 0x00007f4f20dcbb75 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) (cx=0x7f4edd5c5090, 
    cx@entry=0x7f4f09623200, argc=<optimized out>, vp=<optimized out>) at /home/emilio/src/moz/mozilla-unified/dom/bindings/BindingUtils.cpp:3306
#11 0x00007f4f23b228c0 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)
    (cx=cx@entry=0x7f4f09623200, native=native@entry=0x7f4f20dcb9b5 <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, reason@entry=js::CallReason::Call, args=...) at /home/emilio/src/moz/mozilla-unified/js/src/vm/Interpreter.cpp:425
#12 0x00007f4f23b14815 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
    (cx=cx@entry=0x7f4f09623200, args=..., construct=construct@entry=js::NO_CONSTRUCT, reason=(unknown: 0x2bb3b280), reason@entry=js::CallReason::Call) at /home/emilio/src/moz/mozilla-unified/js/src/vm/Interpreter.cpp:512
#13 0x00007f4f23b15510 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) (cx=0x7f4f09623200, args=..., reason=reason@entry=js::CallReason::Call) at /home/emilio/src/moz/mozilla-unified/js/src/vm/Interpreter.cpp:572
#14 0x00007f4f23b15339 in js::CallFromStack(JSContext*, JS::CallArgs const&) (cx=0x7f4edd5c5090, args=...) at /home/emilio/src/moz/mozilla-unified/js/src/vm/Interpreter.cpp:576
#15 0x00007f4f245370fd in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)
    (cx=0x7f4edd5c5090, frame=<optimized out>, stub=0x7f4ef80ed248, argc=2, vp=0x7fffc8aa4a48, res=$JS::UndefinedValue()) at /home/emilio/src/moz/mozilla-unified/js/src/jit/BaselineIC.cpp:1595
... (jit garbage)

Then it was just a matter of DumpJSStack():

(rr) call DumpJSStack()
0 onCopyOrDragStart(e = "[object ClipboardEvent]") ["chrome://messenger/content/mailWindow.js":170:20]
    this = [object HTMLDocument]

Oh, rr, long live :roc, but that's out of reach for Windows developers. Your solution doesn't cover the case of dragging the text out of the text field. We'll ship this for now:
https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1731934-pref-pane-copy.patch
That covers about:preferences, about:support, about:addons, etc.

A fix geared more towards copying from a message would involve checking the source doc URL a little harder like is done here:
https://searchfox.org/comm-central/rev/4053c87e5ec3808d1f10a63b7678429334114391/mail/components/compose/content/MsgComposeCommands.js#9554-9563
If the protocol can't fetch parts, it's not a mail protocol (mailbox:, imap;, etc.). Maybe the TB team prefer that more proper fix.

Attached patch Better patch (obsolete) (deleted) — — Splinter Review

Comment on attachment 9262014 [details] [diff] [review]
Better patch

This is probably a better fix. Should also not cause regressions as document.getSelection() can't be an editor selection.

Attachment #9262014 - Attachment description: patch → Better patch
Attachment #9262014 - Attachment is patch: true
Attachment #9262014 - Attachment mime type: application/octet-stream → text/plain
Attachment #9262014 - Flags: review?(richard.marti)
Attachment #9262000 - Attachment is obsolete: true
Attachment #9262000 - Flags: review?(richard.marti)

Comment on attachment 9262014 [details] [diff] [review]
Better patch

I'll accept this – it's more my sort of thing than Richard's anyway.

Attachment #9262014 - Flags: review?(richard.marti) → review+

The check on the images' source is probably sufficient. The only other sort of image that could be copied normally is a file: image, which would have to be a very obscure case, and would be copied as a data: image anyway so the behaviour doesn't really change.

But yeah, I agree, we could short-circuit this function if the document isn't a message. I don't think we need to though.

onCopyOrDragStart() was initially written as part of the "stolen file bug":
https://hg.mozilla.org/comm-central/rev/14bd3b70a7fda2bfb80b0905adb05ceef296e2d4#l1.16
The intent was to convert pictures referenced via mailbox: and imap: URLs into data URLs. Surprisingly the code is also triggered on about:* pages including details pages of individual add-ons. On a page for an individual add-on there is the add-on icon which triggers the function when part of the selection, leading to a broken paste. Emilio's patch doesn't repair that since the copy source isn't input/textarea/editable.

We'll submit a patch with the purist check from comment #24 in a minute.

Comment on attachment 9262061 [details] [diff] [review]
1731934-copy-only-from-messages.patch - even better patch ;-)

Yeah, alright, we'll do this instead.

Attachment #9262061 - Flags: review?(geoff) → review+
Attachment #9262014 - Attachment is obsolete: true
status-thunderbird94: affected → ---
Keywords: checkin-needed-tb
Target Milestone: --- → 99 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/e15809d07c87
Only hit messages when preparing images for copying. r=darktrojan

Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

Comment on attachment 9262061 [details] [diff] [review]
1731934-copy-only-from-messages.patch - even better patch ;-)

[Approval Request Comment]
Regression caused by (bug #): Unclear, faulty code added in bug 1151366, now triggered by images in the pref pane.
User impact if declined: Can't copy from about:* pages which have images (about:preferences, about:addons).
Testing completed (on c-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky):
Not risky, adding missing check already present in MsgComposeCommands.js.

BIG THANKS to Emilio for tracking down the issue, you're a champion!!

Attachment #9262061 - Flags: approval-comm-esr91?
Attachment #9262061 - Flags: approval-comm-beta?

Comment on attachment 9262061 [details] [diff] [review]
1731934-copy-only-from-messages.patch - even better patch ;-)

[Triage Comment]
Approved for beta

Attachment #9262061 - Flags: approval-comm-beta? → approval-comm-beta+

Verified testing the 98.0b2 release candidate on Windows 10 and Fedora 35 Workstation.

Status: RESOLVED → VERIFIED
status-thunderbird98: fixed → verified

Comment on attachment 9262061 [details] [diff] [review]
1731934-copy-only-from-messages.patch - even better patch ;-)

[Triage Comment]
Approved for esr91

Attachment #9262061 - Flags: approval-comm-esr91? → approval-comm-esr91+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: