From 5b05cdfbd97017d11f6c19a966e884fa889821a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E9=94=A6=E5=BF=83?=
<41134017+Lhcfl@users.noreply.github.com>
Date: Thu, 25 Jul 2024 13:50:30 +0800
Subject: [PATCH] FIX: Add post id to the anchor to prevent two identical
anchors (#28070)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* FIX: Add post id to the anchor to prevent two identical anchors
We generate anchors for headings in posts. This works fine if there is
only one post in a topic with anchors. The problem comes when you have
two or more posts with the same heading. PrettyText generates anchors
based on the heading text using the raw context of each post, so it is
entirely possible to generate the same anchor for two posts in the same
topic, especially for topics with template replies
Post1:
# heading
context
Post2:
# heading
context
When both posts are on the page at the same time, the anchor will only
work for the first post, according to the [HTML specification](https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-the-fragment-identifier).
> If there is an a element in the document tree whose root is document
> that has a name attribute whose value is equal to fragment, then
> return the *first* such element in tree order.
This bug is particularly serious in forums with non-Latin languages,
such as Chinese. We do not generate slugs for Chinese, which results in
the heading anchors being completely dependent on their order.
```ruby
[2] pry(main)> PrettyText.cook("# 中文")
=> "中文
"
```
Therefore, the anchors in the two posts must be in exactly the same by
order, causing almost all of the anchors in the second post to be
invalid.
This commit solves this problem by adding the `post_id` to the anchor.
The new anchor generation method will add `p-{post_id}` as a prefix when
post_id is available:
```ruby
[3] pry(main)> PrettyText.cook("# 中文", post_id: 1234)
=> "中文
"
```
This way we can ensure that each anchor name only appears once on the
same topic. Using post id also prevents the potential possibility of the
same anchor name when splitting/merging topics.
---
.../discourse-markdown-it/src/features/anchor.js | 6 ++++++
.../discourse-markdown-it/src/options.js | 2 ++
.../discourse/tests/unit/lib/pretty-text-test.js | 9 +++++++++
app/models/post.rb | 1 +
lib/pretty_text.rb | 2 ++
spec/models/post_spec.rb | 16 +++++++++++++---
6 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/app/assets/javascripts/discourse-markdown-it/src/features/anchor.js b/app/assets/javascripts/discourse-markdown-it/src/features/anchor.js
index 78027d6f2f1..d9333da723f 100644
--- a/app/assets/javascripts/discourse-markdown-it/src/features/anchor.js
+++ b/app/assets/javascripts/discourse-markdown-it/src/features/anchor.js
@@ -5,6 +5,8 @@ export function setup(helper) {
helper.registerPlugin((md) => {
md.core.ruler.push("anchor", (state) => {
+ const postId = helper.getOptions().postId;
+
for (
let idx = 0, lvl = 0, headingId = 0;
idx < state.tokens.length;
@@ -45,6 +47,10 @@ export function setup(helper) {
slug = `${slug || "h"}-${++headingId}`;
+ if (postId) {
+ slug = `p-${postId}-${slug}`;
+ }
+
linkOpen.attrSet("name", slug);
linkOpen.attrSet("class", "anchor");
linkOpen.attrSet("href", "#" + slug);
diff --git a/app/assets/javascripts/discourse-markdown-it/src/options.js b/app/assets/javascripts/discourse-markdown-it/src/options.js
index 272c72da816..9ab36231bab 100644
--- a/app/assets/javascripts/discourse-markdown-it/src/options.js
+++ b/app/assets/javascripts/discourse-markdown-it/src/options.js
@@ -10,6 +10,7 @@ export default function buildOptions(state) {
lookupPrimaryUserGroup,
getTopicInfo,
topicId,
+ postId,
forceQuoteLink,
userId,
getCurrentUser,
@@ -48,6 +49,7 @@ export default function buildOptions(state) {
lookupPrimaryUserGroup,
getTopicInfo,
topicId,
+ postId,
forceQuoteLink,
userId,
getCurrentUser,
diff --git a/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js b/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js
index 3fc88d6008f..5641ba0c244 100644
--- a/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js
+++ b/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js
@@ -666,6 +666,15 @@ eviltrout
);
});
+ test("Heading anchors with post id", function (assert) {
+ assert.cookedOptions(
+ "# 1\n\n# one",
+ { postId: 1234 },
+ '1
\n' +
+ 'one
'
+ );
+ });
+
test("bold and italics", function (assert) {
assert.cooked(
'a "**hello**"',
diff --git a/app/models/post.rb b/app/models/post.rb
index cbf00888fac..7b2740e0f1b 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -325,6 +325,7 @@ class Post < ActiveRecord::Base
# is referencing.
options[:user_id] = self.last_editor_id
options[:omit_nofollow] = true if omit_nofollow?
+ options[:post_id] = self.id
if self.should_secure_uploads?
each_upload_url do |url|
diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index 0d5f9fbdcff..57968978a46 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -159,6 +159,7 @@ module PrettyText
# markdown_it_rules - An array of markdown rule names which will be applied to the markdown-it engine. Currently used by plugins to customize what markdown-it rules should be
# enabled when rendering markdown.
# topic_id - Topic id for the post being cooked.
+ # post_id - Post id for the post being cooked.
# user_id - User id for the post being cooked.
# force_quote_link - Always create the link to the quoted topic for [quote] bbcode. Normally this only happens
# if the topic_id provided is different from the [quote topic:X].
@@ -208,6 +209,7 @@ module PrettyText
JS
buffer << "__optInput.topicId = #{opts[:topic_id].to_i};\n" if opts[:topic_id]
+ buffer << "__optInput.postId = #{opts[:post_id].to_i};\n" if opts[:post_id]
if opts[:force_quote_link]
buffer << "__optInput.forceQuoteLink = #{opts[:force_quote_link]};\n"
diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb
index 1f78830c302..86910fa3256 100644
--- a/spec/models/post_spec.rb
+++ b/spec/models/post_spec.rb
@@ -1168,20 +1168,30 @@ RSpec.describe Post do
expect(post.cooked).to match(/noopener nofollow ugc/)
end
- it "passes the last_editor_id as the markdown user_id option" do
+ it "passes the last_editor_id as the markdown user_id option and post_id" do
post.save
post.reload
PostAnalyzer
.any_instance
.expects(:cook)
- .with(post.raw, { cook_method: Post.cook_methods[:regular], user_id: post.last_editor_id })
+ .with(
+ post.raw,
+ {
+ cook_method: Post.cook_methods[:regular],
+ user_id: post.last_editor_id,
+ post_id: post.id,
+ },
+ )
post.cook(post.raw)
user_editor = Fabricate(:user)
post.update!(last_editor_id: user_editor.id)
PostAnalyzer
.any_instance
.expects(:cook)
- .with(post.raw, { cook_method: Post.cook_methods[:regular], user_id: user_editor.id })
+ .with(
+ post.raw,
+ { cook_method: Post.cook_methods[:regular], user_id: user_editor.id, post_id: post.id },
+ )
post.cook(post.raw)
end