Fix escape key not working to close compose edit state.
#30086
Conversation
|
|
||
| // Scroll to keep the top of the message content text in the same | ||
| // place visually, adjusting for border and padding. | ||
| const edit_top = $message_edit_content[0].getBoundingClientRect().top; | ||
| const scroll_by = edit_top - content_top + 5 - 14; | ||
|
|
||
| edit_obj.scrolled_by = scroll_by; | ||
| message_viewport.scrollTop(message_viewport.scrollTop() + scroll_by); |
There was a problem hiding this comment.
I would have written something similar to restore the scroll position but it doesn't seem like we need it. Also, looks like this was causing the message list to scroll when we didn't need to. LIkely these values are outdated too.
There was a problem hiding this comment.
Yeah, it seems to just work; maybe this was a quirk of the pre-grid design that we needed this.
| currently_editing_messages.set(message.id, edit_obj); | ||
| message_lists.current.show_edit_message($row, edit_obj); | ||
| const $message_edit_content = $form.find("textarea.message_edit_content"); | ||
| assert($message_edit_content.length === 1); |
There was a problem hiding this comment.
Should this just be doing a [0]! on the previous line rather than this assert, to get a Jquery<HTMLElement>?
| const $message_edit_content = $form.find("textarea.message_edit_content"); | ||
| assert($message_edit_content.length === 1); | ||
| currently_editing_messages.set(message.id, $message_edit_content); | ||
| message_lists.current.show_edit_message($row, $form); | ||
|
|
||
| $form.on("keydown", handle_message_row_edit_keydown); |
There was a problem hiding this comment.
So it might also not be a bad idea to just move these handlers to be globally registered as body.on("keydown", ".message_edit_form", ...?
I think these other cleanups are worth it regardless, but it can be less fragile to not be constantly adding/removing handlers on smaller elements.
| $(edit_id).on("keydown", function (event) { | ||
| compose_ui.handle_keydown(event, $(this).expectOne()); | ||
| resize.watch_manual_resize_for_element($message_edit_content[0]); | ||
| composebox_typeahead.initialize_compose_typeahead($message_edit_content[0]); |
There was a problem hiding this comment.
This function expects a selector, not an element, so I expect this to break typeahead.
There was a problem hiding this comment.
I guess it doesn't, because this works:
export function initialize_compose_typeahead(selector) {
const bootstrap_typeahead_input = {
$element: $(selector),
type: "textarea",
};
Probably we should change that API but given there's a current TS migration PR for that file, I'll defer it. @evykassirer FYI.
web/src/message_edit.js
Outdated
| @@ -434,12 +436,14 @@ function create_copy_to_clipboard_handler($row, source, message_id) { | |||
| } | |||
|
|
|||
| function edit_message($row, raw_content) { | |||
| // NOTE: It is possible that the message list hasn't been rendered yet, | |||
| // so we can't query DOM directly, keep all the selector queries and events | |||
| // to `$row` or `$form`. | |||
There was a problem hiding this comment.
Tweaked this to serve as a proper docstring for the function.
diff --git a/web/src/message_edit.js b/web/src/message_edit.js
index b072aa3d12..d703aad990 100644
--- a/web/src/message_edit.js
+++ b/web/src/message_edit.js
@@ -436,9 +436,11 @@ function create_copy_to_clipboard_handler($row, source, $message_edit_content) {
}
function edit_message($row, raw_content) {
- // NOTE: It is possible that the message list hasn't been rendered yet,
- // so we can't query DOM directly, keep all the selector queries and events
- // to `$row` or `$form`.
+ // Open the message-edit UI for a given message.
+ //
+ // Notably, when switching views, this can be called for a row
+ // that hasn't been added to the DOM yet so, keep all the selector
+ // queries and events to operate on `$row` or `$form`.
assert(message_lists.current !== undefined);
const message = message_lists.current.get(rows.id($row));
$row.find(".message_reactions").hide();
Makes it easier to search and verify changes related to currently_editing_messages.
Doesn't make sense to hide the upload button button later when we can just not show it if the browser doesn't has support for it.
Since we are already selecting the previously selected id in `reselect_selected_id` there is no need to pass the message id here.
To avoid event handlers being lost when we restore a message edit form we go through the process of creating the edit form for the message again. This has the additional benefit of updating the status of buttons based on org settings like GIF or upload button.
timabbott
force-pushed
the
message_edit_form_bug
branch
from
9f643d4
to
f57fb08
Compare
|
Merged this after the one tweak noted above; I guess I forgot about #30086 (comment), but it doesn't seem super important and probably will come up naturally when moving this file to TS anyway. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This was due to event handlers being lost when restoring a message edit state. To avoid it, we recreate the form and attach event handlers to it which also correctly update the current state of the edit form buttons based on settings.
Just edit a message -> go to a different narrow -> click browser back button -> focus on the edit form and try to press escape (it doesn't work)
discussion: #29383 (comment)