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

Fix clicking on links where text is a code block #11529

Open
timabbott opened this issue Feb 11, 2019 · 17 comments
Open

Fix clicking on links where text is a code block #11529

timabbott opened this issue Feb 11, 2019 · 17 comments

Comments

@timabbott
Copy link
Sponsor Member

Apparently, if you do this syntax, e.g.:

Opened [Update TypeScript infrastructure and migrate dict.js](#11513) for review.

We generate a link formatted as a code block, which is nice, but clicking the link just opens compose, probably due to a click handler ordering/precedence issue. See for example:

https://chat.zulip.org/#narrow/stream/65-checkins/topic/Tommy.20Ip/near/697676

This may be a bit tricky to fix, since debugging this sort of click handler ordering issue can be challenging.

@timabbott timabbott added bug area: message feed (uncategorized) difficult Issues which we expect to be quite difficult labels Feb 11, 2019
@kunal-mohta
Copy link
Contributor

@timabbott I don't think there is any bug here, as this worked for me (see gif below).
I guess why it didn't work in the thread that you have linked here is because of the incorrect link provided in the parathesis.
Try a valid link like
Opened [Zulip github](https://github.com/zulip/zulip) for review.
ezgif-5-6ef6a82b6ef8

@timabbott
Copy link
Sponsor Member Author

Try Chrome?

@tommyip
Copy link
Member

tommyip commented Feb 13, 2019

The link didn't get linkified
screenshot from 2019-02-13 22-11-19

@kunal-mohta
Copy link
Contributor

Try Chrome?

Worked for me there too.
ezgif-3-18a47788ea19

@rishig
Copy link
Member

rishig commented Feb 13, 2019

hm, working for me on Chrome as well. (Though it sounds like the original issue is not browser-specific?)

@rishig
Copy link
Member

rishig commented Feb 13, 2019

ok, I reproed. The link has to come from a linkification filter. (Kunal, let me know if that doesn't make sense.)

@kunal-mohta
Copy link
Contributor

@rishig Yes, a brief explanation would be helpful. 😅
How did you reproduce the issue?

@timabbott
Copy link
Sponsor Member Author

@kunal-mohta you need to setup a "Linkifier" in organization settings, and then generate a link like that. You can get the source content for Tommy's message and copy the relevant Linkifier from chat.zulip.org's organization settings to set this up in your development environment.

@rishig
Copy link
Member

rishig commented Feb 14, 2019

I should have linked to https://zulipchat.com/help/add-a-custom-linkification-filter.

@vrongmeal
Copy link
Collaborator

vrongmeal commented Feb 15, 2019

The issue is not specific to links with text as code blocks, it is purely related to the linkifier. Even with simple text, syntax like:

[some text here](#12345)

points to http://localhost:9991/#12345

[edit] The compose box opens maybe because the click propagates to the code block, simply clicking the link doesn't open the compose box.

@tommyip
Copy link
Member

tommyip commented Feb 16, 2019

The problem is the realm filter is expanded before the link, so at some point in the renderer the markdown is transformed into

[some text](<a href="https://github.com/zulip/zulip/pull/123" target="_blank" title="https://github.com/zulip/zulip/pull/123">#123</a>)

@zulipbot claim

@timabbott
Copy link
Sponsor Member Author

@aero31aero FYI since this is a markdown processor ordering bug.

@aero31aero
Copy link
Member

aero31aero commented Feb 16, 2019

Interesting. I believe this was present before the recent changes to ordering as well. It shouldn't be too hard to fix.

@zulipbot claim

Edit: just saw Tommy has claimed it.

@zulipbot
Copy link
Member

Hello @aero31aero, it looks like someone has already claimed this issue! Since we believe multiple assignments to the same issue may cause some confusion, we encourage you to search for other unclaimed issues to work on. However, you can always reclaim this issue if no one is working on it.

We look forward to your valuable contributions!

@timabbott
Copy link
Sponsor Member Author

You can chat with Tommy and collaborate -- he might benefit from your context.

@tommyip
Copy link
Member

tommyip commented Feb 17, 2019

Ok so I fixed the frontend markdown. My solution is a bit of a hack 😬 since I got round the ordering problem by replacing the realm filter with a marker before rendering and inserting the link afterwards.

@aero31aero would you mind reviewing my PR?

@zulipbot
Copy link
Member

zulipbot commented Mar 3, 2019

Hello @tommyip, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants