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 user-defined markup links targets (#29305) #29666

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

GiteaBot
Copy link
Contributor

@GiteaBot GiteaBot commented Mar 8, 2024

Backport #29305 by @DanielMatiasCarvalho

This seeks to fix the bug reported on issue #29196.

Cause:
ID's with custom characters (- , _ , etc.), were not linking correctly in the Markdown file when rendered in the browser because the ID in the respective destinies would be different than the one in anchor, while for IDs with only letters, the ID would be the same.

Fix:
It was suggested that to fix this bug, it should more or less like GitHub does it. While in gitea the anchors would be put in HTML like this:

<p dir="auto"><a href="#user-content-_toc152597800" rel="nofollow">Review</a></p>
<p dir="auto"><a href="#user-content-_toc152597802" rel="nofollow">Staging</a></p>
<p dir="auto"><a href="#user-content-_toc152597803" rel="nofollow">Development</a></p>
<p dir="auto"><a href="#user-content-_toc152597828" rel="nofollow">Testing</a></p>
<p dir="auto"><a href="#user-content-_toc152597829" rel="nofollow">Unit-tests</a></p>

In GitHub, the same anchor's href properties would be the same without "user-content-" trailing behind.

So my code made sure to change those anchors, so it would not include "user-content-" and then add respective Event Listeners so it would scroll into the supposed places.

Fixes: #29196

This seeks to fix the bug reported on issue go-gitea#29196. 

Cause: 
ID's with custom characters (- , _ , etc.), were not linking correctly
in the Markdown file when rendered in the browser because the ID in the
respective destinies would be different than the one in anchor, while
for IDs with only letters, the ID would be the same.

Fix:
It was suggested that to fix this bug, it should more or less like
GitHub does it. While in gitea the anchors would be put in HTML like
this:
```
<p dir="auto"><a href="#user-content-_toc152597800" rel="nofollow">Review</a></p>
<p dir="auto"><a href="#user-content-_toc152597802" rel="nofollow">Staging</a></p>
<p dir="auto"><a href="#user-content-_toc152597803" rel="nofollow">Development</a></p>
<p dir="auto"><a href="#user-content-_toc152597828" rel="nofollow">Testing</a></p>
<p dir="auto"><a href="#user-content-_toc152597829" rel="nofollow">Unit-tests</a></p>

```
In GitHub, the same anchor's href properties would be the same without
"user-content-" trailing behind.

So my code made sure to change those anchors, so it would not include
"user-content-" and then add respective Event Listeners so it would
scroll into the supposed places.

Fixes: go-gitea#29196

---------

Co-authored-by: silverwind <[email protected]>
@GiteaBot GiteaBot added this to the 1.21.8 milestone Mar 8, 2024
@GiteaBot GiteaBot requested a review from delvh March 8, 2024 09:53
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 8, 2024
@GiteaBot GiteaBot requested a review from silverwind March 8, 2024 09:53
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 8, 2024
if (!href.startsWith('#user-content-')) continue;
const originalId = href.replace(/^#user-content-/, '');
a.setAttribute('href', `#${encodeURIComponent(originalId)}`);
if (document.getElementsByName(originalId).length !== 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, getElementsByName?
Isn't that the wrong call?
Shouldn't it be getElementsByID?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, we use that method because we are considering the cases that we use <a name="link"><\a>. In this condition, we are making it so that those anchors that do not use this notation work as intended by having the event listener.

Copy link
Member

@silverwind silverwind Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's correct. It targets specifically links with name. A likely better and more specific selector could be document.querySelectorAll('.markup a[name]') so we don't accidentially select other things, but lets not do this in a backport.

Copy link
Member

@silverwind silverwind Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better selector: a.closest('.markup').querySelectorAll('a[name]')

This will ensure it will only search for the target in the current markup document which is important because there can be multiple documents being rendered. I can follow up with this change targeting main branch later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup: #29679

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 8, 2024
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per @silverwind

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 8, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 8, 2024
@silverwind silverwind enabled auto-merge (squash) March 8, 2024 16:48
@silverwind silverwind merged commit bfc7c8a into go-gitea:release/v1.21 Mar 8, 2024
27 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 8, 2024
silverwind added a commit that referenced this pull request Mar 8, 2024
Followup #29305. As per discussion
in #29666 (comment),
make this selector only search in the current `.markup` document, as
there can be multiples displayed at the same time.

@DanielMatiasCarvalho maybe you can review.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 8, 2024
Followup go-gitea#29305. As per discussion
in go-gitea#29666 (comment),
make this selector only search in the current `.markup` document, as
there can be multiples displayed at the same time.

@DanielMatiasCarvalho maybe you can review.
silverwind added a commit that referenced this pull request Mar 9, 2024
Backport #29679 by @silverwind

Followup #29305. As per discussion
in #29666 (comment),
make this selector only search in the current `.markup` document, as
there can be multiples displayed at the same time.

@DanielMatiasCarvalho maybe you can review.

Co-authored-by: silverwind <[email protected]>
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Mar 13, 2024
Backport #29679 by @silverwind

Followup go-gitea/gitea#29305. As per discussion
in go-gitea/gitea#29666 (comment),
make this selector only search in the current `.markup` document, as
there can be multiples displayed at the same time.

@DanielMatiasCarvalho maybe you can review.

Co-authored-by: silverwind <[email protected]>
(cherry picked from commit 25b0c99a41e9024d439deaa55be7ba87f6cd557f)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Mar 13, 2024
Followup go-gitea/gitea#29305. As per discussion
in go-gitea/gitea#29666 (comment),
make this selector only search in the current `.markup` document, as
there can be multiples displayed at the same time.

@DanielMatiasCarvalho maybe you can review.

(cherry picked from commit baeb2511741aa70d24a48fd46db936b52be9d9dd)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants