Trim footnote anchors from excerpts

[mcsf]

What?

Fixes #52035

Strips footnote anchors from post content whenever excerpts are being computed by WordPress. That is, this removes the <sup><a...></sup> marks, so that a piece of content like "Hello¹, world²" becomes "Hello, world".

Why?

See parent issue.

How?

Hook into the the_content filter, but only when this is part of a wider get_the_excerpt filter.

Question: Is this the best approach to fix the issue?

Testing Instructions

1. Start a post, add some text and anchors.
2. Find a way to grab its excerpt, e.g. by rendering it in a template which uses the Post Excerpt block.

Screenshots or screencast

On the left: a post with footnotes. On the right: the index page with post excerpts stripped of footnote anchors.

[github-actions]

Flaky tests detected in bb26cac.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5522773682
📝 Reported issues:

• [Flaky Test] Block context is reflected in the preview #48826 in specs/editor/plugins/block-context.test.js

[bph]

Tested

• with Twenty-Twenty Three and Björk block themes
• tested with multiple Footnotes in Excerpts as well.
• Footnotes are removed from the excerpt in previews in the template editor as well as the site editor and the frontend.

All 🟢

[andrewserong]

Thanks for working on this one!

Hook into the the_content filter, but only when this is part of a wider get_the_excerpt filter.
Question: Is this the best approach to fix the issue?

Great question! I wasn't too familiar with how all this is hooked together, so took a look through the chain of calls and filters. I think on balance this is a pretty good approach so far, based on the following:

• In core, the default excerpt is grabbed via a default filter on get_the_excerpt which calls wp_trim_excerpt.
• wp_trim_excerpt is responsible for getting the post content, then strips shortcodes and strips blocks that shouldn't be present in the output (excerpt_remove_blocks), before applying the_content filter.
• By the time the wp_trim_excerpt filter is run, wp_trim_words is already called, which strips <sup> but without removing its contents, resulting in stray 1 and 2 (etc) characters being retained in the post excerpt.

So, currently, it seems the only place to hook into removal is either within the_content or render_block callbacks, and the_content means the check will happen fewer times than if we check while rendering blocks 👍. (Technically, it might be possible to do it via a filter on wp_trim_words using the $original_text param, but that would involve re-doing the trimming behaviour, so would be a fair bit hackier IMO)

One potential alternative in core could be to still use the function introduced in this PR, but instead of applying it via the_content filter, add it as a direct call within wp_trim_excerpt. That could potentially make things slightly neater, but with the disadvantage that if anyone wanted to switch off this callback, they couldn't call remove_filter to remove the call to gutenberg_trim_footnoes.

So, all in all, on balance, I think you've found a good approach here. At first I was wondering if a CSS solution could work to set it to display: none, but the stripping of the HTML tags in wp_trim_words renders that impossible.

✅ Tests nicely, stripping the sup tags in excerpts
✅ Rendering is unaffected outside of rendering excerpts
✅ Regex looks solid to me, and is specific enough that it's not going to accidentally remove other markup that shouldn't be removed

When backporting to core, it might be worth adding a couple of unit tests to confirm the removal works as expected? Other than that, this LGTM! ✨

[ramonjd]

This is working pretty well, thanks! I'm not sure what else we could do here.

Looks like it handles most of the inline changes we can make to the footnote sup

I only think with regexes like these, especially for new functionality, it would be great to have unit tests. That way when breaking changes come down the line we can react immediately.

[ramonjd]

Also tested in Chinese and Cyrillic scripts just to make sure no i18n funny business was messing with things.

Looks good!

[ellatrix]

Could we use the HTML tag processor here? Cc @dmsnell

[ramonjd]

Also interesting to know this.

When doing the backport I had a quick look at whether it could remove tags like it could with attributes/classes and I couldn't find anything.

The other option would be a DOM parser to strip the <sup /> and its contents but not sure if that would be a faster way to do it.

[dmsnell]

Is there any other possible tag inside of here besides <sup> and <a>?
The Tag Processor cannot easily remove this pattern, but the HTML Processor would be able to if I added <sup> support and the ability to remove a tag.

This could all plausibly happen for the WordPress 6.4 release, no promises.

[dmsnell]

Also there's an "ugly" way to do it with the Tag Processor by subclassing and accessing internals, but the HTML Processor is working well enough I'm not experimenting with that approach anymore.

[westonruter]

Could we use the HTML tag processor here?

I don't believe so because WP_HTML_Tag_Processor doesn't have a remove_tag method. If it did, you could do something like this:

$p = new WP_HTML_Tag_Processor( $html );
while ( $p->next_tag( array( 'tag_name' => 'SUP' ) ) ) {
	if (
		$p->get_attribute( 'data-fn' ) &&
		$p->get_attribute( 'class' )
	) {
		$p->set_bookmark( 'footer_sup' );
		if (
			$p->next_tag() &&
			'A' === $p->get_tag() &&
			$p->get_attribute( 'href' ) &&
			$p->get_attribute( 'id' )
		) {
			$p->seek( 'footer_sup' );
			$p->remove_tag(); // 👈 This method doesn't exist.
		}
		$p->release_bookmark( 'footer_sup' );
	}
}

But @dmsnell would know better 😄

[westonruter]

(Oops race condition)

[ellatrix]

I'm just flagging this with @dmsnell as another place we're processing HTML 😄

[ramonjd]

When backporting to core, it might be worth adding a couple of unit tests to confirm the removal works as expected?

+1

Also, testing this PR made me notice that footnotes are not restored with previous revisions.

[ramonjd]

Also, testing this PR made me notice that footnotes are not restored with previous revisions.

I'll create an issue 👍🏻

[bph]

Should this be merged in time, it would be great to have in WordPress 6.3 and also in GB 16.2.

[mcsf]

Thanks for the thorough reviewing, everyone, and thanks for the follow-up items, @ramonjd!

@andrewserong:

One potential alternative in core could be to still use the function introduced in this PR, but instead of applying it via the_content filter, add it as a direct call within wp_trim_excerpt. That could potentially make things slightly neater, but with the disadvantage that if anyone wanted to switch off this callback, they couldn't call remove_filter to remove the call to gutenberg_trim_footnoes.

Yeah, I agree that a few such solutions are on the table for Core itself, though I'm glad you're validating the current approach!

@ramonjd:

Looks like it handles most of the inline changes we can make to the footnote sup

Testing concurrent formats is something I completely missed and it was a great call of yours. I was worried for a second about such situations leading to certain markup that the regular expression would then miss, but I believe that in practice this can't happen, because:

• In their current implementation, footnotes can only be inserted as a special RichText object that then gets the <sup><a>...</a></sup> wrapped around it.
  • In particular, users can't apply a footnote as a format around some preexisting piece of rich text, even though from a UX standpoint it feels that way. So we don't end up with <sup><a> <strong>preexisting</strong> </a></sup>.
• Only other formats can be applied to a footnote anchor, causing the markup to look like <strong> <sup><a>...</a></sup> </strong>, which doesn't affect the regex matching.

[ockham]

I just cherry-picked this PR to the release/16.2 branch to get it included in the next release: 0c1a9c6

[tellthemachines]

I just cherry-picked this PR to the update/first-batch-pre-RC1 branch to get it included in the next release: 1591f04

[ellatrix]

Thanks @mcsf!