1611442 - Magic copy doesn't copy across HTML tags

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Patrick Cloke [:clokep]]

It seems that magic copy got broken at some point.

Steps to reproduce:

1. Join IRC
2. Have someone ping you: "clokep: Hi!"
3. Reply to them "Hi friend!"
4. Highlight the whole conversation and hit Ctrl+C

Expected result:

My clipboard has something like:

6:39:18 AM - friend: Hi clokep!
6:41:17 AM - clokep: Hi friend!

Actual result:

My clipboard has something like:

6:39:18 AM - friend: Hi

Additional information

It seems that everything works OK if the ping is at the start of the message, but anywhere else it stops the copying.

Comment 1

[Patrick Cloke [:clokep]]

From a bit of experimenting (and a hint from florian) it seems that the copying breaks when it hits an HTML tag (e.g. a link or bold text).

Comment 3

[Jorg K (CEST = GMT+2)]

Sadly that makes TB unusable, since copying blocks of chat content is a frequent operation. Can't do without.

Comment 4

[Alice0775 White]

Regression window(STR of Bug 1646152):
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=79bcd5d1148477ce4b3c611ebd83205eea186598&tochange=a128f9557e9db7951a9d8d6467e553fdccb8a0f5
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=98ff8f3e16f3294bff036ddd17ee16b1fe0c97ad&tochange=98ff8f3e16f3294bff036ddd17ee16b1fe0c97ad

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks, Alice, another regression from bug 1570954. Should be fixable by a simple reversion.

EDIT: Likely some of this needs to be reversed:
https://hg.mozilla.org/comm-central/diff/a128f9557e9db7951a9d8d6467e553fdccb8a0f5/chat/modules/imThemes.jsm

Yay to mass changes without testing :-(

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1611442-chat-copy.patch

This works. I did a trial and error job reverting the said changeset hunk by hunk. Let me know if your want the comment adjusted or adjust it yourself before landing.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 9157221 [details] [diff] [review]
1611442-chat-copy.patch

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

Thanks for figuring this out!

::: chat/modules/imThemes.jsm
@@ +945,5 @@
>        this._otherSelected = true;
>        return;
>      }
>      let startPoint = this._range.comparePoint(spanNode, 0);
> +    // Note that we're dealing with XUL elements here, so we need to use

Can you replace "we're dealing with XUL elements here" with "we are working on the HTML DOM, including text nodes"?

This code has never dealt with XUL elements; it's actually because it was already expecting HTML that the patch doing a mass XUL->HTML conversion broke it.

Comment 8

[Florian Quèze [:florian]]

(In reply to Jorg K (CEST = GMT+2) from comment #5)

EDIT: Likely some of this needs to be reversed:
https://hg.mozilla.org/comm-central/diff/a128f9557e9db7951a9d8d6467e553fdccb8a0f5/chat/modules/imThemes.jsm

Should we actually revert all the changes to this file? I haven't looked in details to guess the impact of the changes, but none of this code was touching XUL. Some is HTML and some is XML (.plist files).

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1611442-chat-copy.patch

Comment adjusted. I refer the other question to Magnus. I don't want to be the assignee here, I'm just mopping up :-(

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9157229 [details] [diff] [review]
1611442-chat-copy.patch

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

It doesn't look like any of the other changes were wrong.

::: chat/modules/imThemes.jsm
@@ +946,5 @@
>        return;
>      }
>      let startPoint = this._range.comparePoint(spanNode, 0);
> +    // Note that we are working on the HTML DOM, including text nodes,
> +    // so we need to use childNodes() here and below.

childNodes, not childNodes()

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: 1611442-chat-copy.patch

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9157611 [details] [diff] [review]
1611442-chat-copy.patch

[Approval Request Comment]
Regression caused by (bug #): 1570954
User impact if declined: I find chat unusable if you can't copy/paste.

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9157611 [details] [diff] [review]
1611442-chat-copy.patch

Approved for beta

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c2c86d40d2a6
Revert part of bug 1570954 (rev. a128f9557e9d) to fix copying from chat conversations. r=mkmelin

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/3b27eba1a57f