copy_and_paste: Fix CSS selector injection bug, convert module to TypeScript #30091
Conversation
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
andersk
added
integration review
andersk
force-pushed
the
copy_and_paste.ts
branch
from
03a55f3
to
d460620
Compare
Signed-off-by: Anders Kaseorg <anders@zulip.com>
andersk
force-pushed
the
copy_and_paste.ts
branch
from
d460620
to
521f587
Compare
| @@ -293,63 +313,71 @@ function get_end_tr_from_endc($endc) { | |||
| // we can use the last message from the previous recipient_row. | |||
| if ($endc.parents(".message_header").length > 0) { | |||
| const $overflow_recipient_row = $endc.parents(".recipient_row").first(); | |||
| return $overflow_recipient_row.prev(".recipient_row").last(".message_row"); | |||
| return $overflow_recipient_row.prev(".recipient_row").last(); | |||
There was a problem hiding this comment.
Is this a functional change? I guess not, both because last ignores parameters and because the last item in a recipient_row is always a message_row; I'm just not sure that's guaranteed to be an invariant.
<div class="recipient_row" id="{{message_group_id}}">
{{> recipient_row use_match_properties=../use_match_properties}}
{{#each message_containers}}
{{#with this}}
{{> single_message use_match_properties=../../use_match_properties message_list_id=../../message_list_id}}
{{/with}}
{{/each}}
</div>
But probably we meant to write .filter(".message_row").last()?
There was a problem hiding this comment.
Actually this is really quite confusing -- the comments think it's getting a message_row, but it's always been getting a recipient_row
There was a problem hiding this comment.
I don’t know what the intention was, but the jQuery .last() method does not accept any arguments.
There was a problem hiding this comment.
Yeah I'll let @N-Shar-ma clean this up, since this is ancient code but I think she has a good test setup for verifying changes. The old code is quite clearly wrong but it's not clear what is the correct version... and this doesn't regress anything.
| @@ -265,7 +285,7 @@ export function analyze_selection(selection) { | |||
| }; | |||
| } | |||
|
|
|||
| function get_end_tr_from_endc($endc) { | |||
| function get_end_tr_from_endc($endc: JQuery<Node>): JQuery { | |||
| if ($endc.attr("id") === "bottom_whitespace" || $endc.attr("id") === "compose_close") { | |||
| // If the selection ends in the bottom whitespace, we should | |||
| // act as though the selection ends on the final message. | |||
There was a problem hiding this comment.
The logic here looks buggy as well; $(".message_row").last(); does not filter to the currently visible message list, if the combined feed is still present/invisible.
@N-Shar-ma perhaps you can do a follow-up PR fixing this and the other apparent logic bug this TS migration reveals? I don't think there's any regressions in this PR.
Fixed 2 bugs in the detection of the last message row copied. This fixes the issue where when the mouse was released on the message header after copying multiple messages, the pasted text was not formatted correctly. Follow up to zulip#30091.
Fixed 2 bugs in the detection of the last message row copied. This fixes the issue where when the mouse was released on the message header after copying multiple messages, the pasted text was not formatted correctly. Follow up to #30091.
No description provided.