Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File upload gets placed into empty stream or topic box #23284

Closed
alya opened this issue Oct 19, 2022 · 7 comments
Closed

File upload gets placed into empty stream or topic box #23284

alya opened this issue Oct 19, 2022 · 7 comments
Labels

Comments

@alya
Copy link
Contributor

alya commented Oct 19, 2022

Repro:

  1. Open the compose box with an empty stream and/or topic.
  2. Drag a file to the compose box.

Expected: File gets uploaded as normal.
Actual: [Uploading..] text gets placed into the stream box if it's empty, and into the topic box otherwise.

Screen Shot 2022-10-18 at 11 20 11 PM
Screen Shot 2022-10-18 at 11 18 51 PM

@zulipbot
Copy link
Member

Hello @zulip/server-compose, @zulip/server-misc members, this issue was labeled with the "area: compose", "area: uploads" labels, so you may want to check it out!

@JoeyPriceless
Copy link
Collaborator

JoeyPriceless commented Oct 19, 2022

I'm having trouble reproducing this issue. The [Uploading..] text always seems to appear in the content box and not the stream/topic boxes even if they're empty. Am I missing something?

chrome_zMfy6jv3AR

@alya
Copy link
Contributor Author

alya commented Oct 19, 2022

Huh, and now this stopped happening for me. I'm very puzzled!

... And now it's happening again.

@alya
Copy link
Contributor Author

alya commented Oct 19, 2022

2022-10-19 07 52 52

@alya alya closed this as completed Oct 19, 2022
@alya alya reopened this Nov 2, 2022
@alya
Copy link
Contributor Author

alya commented Nov 2, 2022

Hrm, I don't remember why I closed this issue earlier.

@timabbott
Copy link
Sponsor Member

I can reliably reproduce this issue in Chrome on Linux. It's a bit finicky; it works best if you have a split-screen window (so you don't need to alt-tab in order before initiating the drag, which likely affects the focus state, which is important for this reproducer) and open the compose box using the c keyboard shortcut while looking at "All messages". Then just drag an item into the compose area.

The code path doing the insertion is compose_ui.insert_syntax_and_focus being called from upload.js.

With this logging code added:

$ git diff
diff --git a/static/js/compose_ui.js b/static/js/compose_ui.js
index a5d7578499..3acf1c5f7d 100644
--- a/static/js/compose_ui.js
+++ b/static/js/compose_ui.js
@@ -60,14 +60,18 @@ export function smart_insert($textarea, syntax) {
         syntax += " ";
     }
 
+    console.log("Triggering focus", $textarea, $(":focus"));
     $textarea.trigger("focus");
+    console.log("Triggered focus", $textarea, $(":focus"));
 
     // We prefer to use insertText, which supports things like undo better
     // for rich-text editing features like inserting links.  But we fall
     // back to textarea.caret if the browser doesn't support insertText.
     if (!document.execCommand("insertText", false, syntax)) {
+        console.log("Fail inserted text", $(":focus"));
         $textarea.caret(syntax);
     }
+    console.log("Success inserted text",  $textarea, $(":focus"));
 
     autosize_textarea($textarea);
 }

I get this output:

image

What appears to be happening is that somehow the focus stays on the wrong element just before the insertText command executes; leading to the insertion happening in the wrong place.

The right fix likely involves just replacing insertText, which is deprecated anyway, completely. @amanagr I think you were able to use the text-field-edit library for this class of string replacement preserving Undo elsewhere in this file; want to try porting this to use that?

@timabbott
Copy link
Sponsor Member

Opened #23499 attempting that.

amanagr pushed a commit to timabbott/zulip that referenced this issue Nov 9, 2022
Fixes zulip#23284
The basic approach used by `text-area-edit` is same as we were
using, so there is no real change. There are some nice checks
in `text-area-edit` which we don't do that helps us avoid
common bugs.
Ujjawal3 pushed a commit to Ujjawal3/zulip that referenced this issue Nov 12, 2022
Fixes zulip#23284
The basic approach used by `text-area-edit` is same as we were
using, so there is no real change. There are some nice checks
in `text-area-edit` which we don't do that helps us avoid
common bugs.
Ujjawal3 pushed a commit to Ujjawal3/zulip that referenced this issue Nov 12, 2022
Fixes zulip#23284
The basic approach used by `text-area-edit` is same as we were
using, so there is no real change. There are some nice checks
in `text-area-edit` which we don't do that helps us avoid
common bugs.
Taiki-Choda pushed a commit to Taiki-Choda/zulip that referenced this issue Nov 13, 2022
Fixes zulip#23284
The basic approach used by `text-area-edit` is same as we were
using, so there is no real change. There are some nice checks
in `text-area-edit` which we don't do that helps us avoid
common bugs.
vnhanai pushed a commit to vnhanai/zulip that referenced this issue Nov 28, 2022
Fixes zulip#23284
The basic approach used by `text-area-edit` is same as we were
using, so there is no real change. There are some nice checks
in `text-area-edit` which we don't do that helps us avoid
common bugs.
lagraham337 pushed a commit to lagraham337/zulip that referenced this issue Nov 30, 2022
Fixes zulip#23284
The basic approach used by `text-area-edit` is same as we were
using, so there is no real change. There are some nice checks
in `text-area-edit` which we don't do that helps us avoid
common bugs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants