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]: Broken links in search #23309

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

[FIX]: Broken links in search #23309

wants to merge 2 commits into from

Conversation

abhinaypandey02
Copy link

Added a more precise regex to check and replace the searched text with a highlight. (Ignoring the links in href which was causing the issue)

Proposed changes (including videos or screenshots)

The following image is taken from the issue #23285.
image
As the user mentioned the <a> tag doesn't seem to get the correct URL in the href attribute.
The reason being that if a searched text is present in the search result it gets replaced by <mark>text<mark>. But this also affectes the attributes. For instance, if we searched for google.com, href="google.com" would become href="<mark>google.com</mark>", which results in wrong destination.

This was fixed using a more narrow regex search. Here I look for only those matches which does not come between href=" and "
Following this change the URL points to the right link.

Issue(s)

#23285

Steps to test or reproduce

Reproduction steps mentioned in the issue

Further comments

My first PR, please suggest better approaches, if any.

Added a more precise regex to check and replace the searched text with a highlight. (Ignoring the links in href which was causing the issue)
@TBG-FR
Copy link
Contributor

TBG-FR commented Oct 7, 2021

What's the difference with #20878 ?

A lot of issues are referencing that problem, and now we have 2 PRs, but still no answers from RocketChat maintainers....

@abhinaypandey02
Copy link
Author

What's the difference with #20878 ?

A lot of issues are referencing that problem, and now we have 2 PRs, but still no answers from RocketChat maintainers....

I am sorry I didn't see that PR, I guess it's better maintained and I should close this one

@TBG-FR
Copy link
Contributor

TBG-FR commented Oct 8, 2021

What's the difference with #20878 ?
A lot of issues are referencing that problem, and now we have 2 PRs, but still no answers from RocketChat maintainers....

I am sorry I didn't see that PR, I guess it's better maintained and I should close this one

No problem ! It hasn't been updated for a while though... 😅

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2021

CLA assistant check
All committers have signed the CLA.

@Yopai
Copy link

Yopai commented May 24, 2022

Rather than trying to fix the regexp, wouldn't it be more advisable to accept the fact highlighting cannot be simply done with regular expressions ?

See this SO question and answer :

https://stackoverflow.com/a/8193700/2955802

That handle the problem stated here, which is not highlighting everything that is inside tags.

@TBG-FR
Copy link
Contributor

TBG-FR commented May 24, 2022

Rather than trying to fix the regexp, wouldn't it be more advisable to accept the fact highlighting cannot be simply done with regular expressions ?

See this SO question and answer :

https://stackoverflow.com/a/8193700/2955802

That handle the problem stated here, which is not highlighting everything that is inside tags.

I don't get your point, we were able to fix that highlighting issue, using (not only, right) better regexes 🤷‍♂️ Wether it is that PR #23309 or mine #20878 . The regex/code just need to be upgraded for any new/specific element that can break highlighting

@TBG-FR
Copy link
Contributor

TBG-FR commented Apr 30, 2023

Can be closed, fixed by #20878 (merged) 😉

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

Successfully merging this pull request may close these issues.

None yet

5 participants