-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix URLs in search results #14058
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
base: master
Are you sure you want to change the base?
Fix URLs in search results #14058
Conversation
| } | ||
| let linkEl = listItem.appendChild(document.createElement("a")); | ||
| linkEl.href = linkUrl + anchor; | ||
| const encodedLinkUrl = linkUrl.split("/").map(encodeURIComponent).join("/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for linkUrl to contain other components that we would not want to URL-encode? (for example, protocol scheme - http://, or similar?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
If linkUrl contains something like http://, the colon would be encoded to %3A, so it would become http%3A//.
At least in the examples shown in the related issue, it doesn’t seem to contain any protocol scheme.
I’m checking the behavior of linkUrl, but if you know of any cases where it may contain a protocol scheme, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hntk03 - I think in particular it is the behaviour and values of the content_root setting that we should inspect for this.
Currently I don't think that it could contain a URL scheme -- however perhaps we should write this part of the code defensively even so (because this JavaScript code may exist and run for a long duration of time in various documentation contexts).
cc @AA-Turner (in case you can add any more thoughts about the content_root, re: 8e730ae)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
It seems that content_root does not contain a protocol scheme,
but I understand your concern.
To confirm — should we encode only the pathname part of the URL and leave any protocol (if present) untouched?
AA-Turner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thanks for the feedback. I’ll look into the suggested approach. |
Purpose
This PR ensures that URLs in search results are properly encoded.
Previously, search result URLs containing characters such as
#or?were not encoded correctly.This change adds URL encoding in the
_displayItemof search results to fix this issue.References