Notes: Carry mention user IDs in a data attribute instead of classes - #80496
Notes: Carry mention user IDs in a data attribute instead of classes#80496adamsilverstein wants to merge 8 commits into
Conversation
The `@` mention completer stored the mentioned user's ID in a `class="wp-note-mention user-N"` on the anchor. A class allowance lets note authors persist arbitrary classes on links, widening the CSS and JavaScript selector blast radius in the admin. Switch the payload to a single purpose-specific `data-wp-note-mention-user` attribute, which is inert outside the notes sidebar. Drop the `wp-note-mention` class from newly inserted mentions: with the class allowance gone (see the kses change), classes would be stripped server-side for authors without `unfiltered_html`, making draft and saved markup inconsistent. The chip styles now target `[data-wp-note-mention-user]` while keeping the legacy `.wp-note-mention` selector so notes saved before this change stay styled.
The mention allowance previously extended the comment kses allowlist to permit any `class` on links, so it had to be armed only while a `note` comment was filtered and disarmed immediately after, via a chain of `preprocess_comment` / `rest_preprocess_comment` arming hooks and a self-removing disarm on `pre_comment_content` at `PHP_INT_MAX` plus a `rest_request_after_callbacks` backstop. Now that mentions carry the user ID in a single inert `data-wp-note-mention-user` attribute rather than a class, that machinery is unnecessary. Replace it with one always-registered `wp_kses_allowed_html` filter that allows only that exact attribute on `a` in the `pre_comment_content` context. Note: `data-*` attributes are only allowed by default in the `post` kses context, not `pre_comment_content`, so a narrow allowance is still required. Because the attribute is inert (no styling or JS hook outside the notes sidebar, and notification parsing only reads `note` comments), allowing it globally in comment content is acceptable. The `function_exists` core-deference guard is kept so this disables once WP core ships an equivalent.
Update the fixtures to the `data-wp-note-mention-user` markup and the new always-on model. Add a baseline test proving the comment kses context strips the mention data attribute without the notes filter, documenting the load-bearing fact that `pre_comment_content` (unlike the `post` context) does not allow `data-*` by default. Assert the attribute survives in note content and, now, in regular comment content too, documenting the intended behavior change; keep the check that every other link attribute (`class`, event handlers, styles, other data attributes) is still stripped. Drop the arm/disarm leak-prevention and REST-arming tests, whose machinery no longer exists.
Replace the `toHaveClass( /^wp-note-mention user-N$/ )` assertions on the draft and saved mention chips with `toHaveAttribute( 'data-wp-note-mention-user', String( mentionedUserId ) )`, matching the switch from a class to a data attribute. The `href` `/author/` assertion is kept.
Add a presentational story rendering the note-content mention chip in both the current `data-wp-note-mention-user` form and the legacy `wp-note-mention` class form inside the collab-sidebar panel structure, so the built editor styles apply. It confirms both forms render as an identical chip, which is why the chip styles keep the legacy selector.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
The kses simplification in this PR changes what wordpress-develop#12503 backports, so the Core backport changelog check requires listing it against that Core PR.
|
Size Change: +33 B (0%) Total Size: 7.74 MB 📦 View Changed
|
The class was never shipped in a stable WordPress release, so no saved notes carry it. Removing it now avoids keeping a legacy selector forever. The plain-link carve-out now excludes mentions by the data attribute alone, per review feedback.
|
Flaky tests detected in 23c8c17. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29807083373
|
|
IIRC, the whole point of using classes was to get Link-format UI for free, without needing to add faux formatting. The discussion can be found in PR. While I understand the concerns about data manipulation from WordPress/wordpress-develop#12503 (review), it's not clear what the desired UX/UI outcomes should be. Have we tried using |
There was a problem hiding this comment.
Probably useful for testing, but not much different from actually running e2e tests. Plus, everything is hardcoded, so this won't catch any regressions.
I would drop the story file.
|
Here are some interesting behavior(s), I've observed while comparing trunk and this branch
I think we might need stricter checks before notifying users. ScreencastCleanShot.2026-07-21.at.10.55.08.mp4 |
The motivation for considering dropping using classes directly is current approach of allowing any class attributes to be added by less privileged users to note content. Since classes impact display, it would be possible for someone for example to hide or change links by sending the right class. When @westonruter suggested data attributes, he expected they were already allowed by kses and even though that is not the case I thought it would be worth exploring the approach. Perhaps what we can do instead is:
|
What would that look like? making sure the full username is present?
Good point, let's try that.
ok
Reviewing this again, I'm not sure it helps if we still need the classes. Instead, I'll drop the data attribute and focus on adjust the kses callback to see if I can only allow the specific classes we need.
👍🏼 |
|
Here are summarized thoughts:
Don't have a strong opinion here. I'll defer to you and @westonruter. |
|
Thanks for all the feedback. I'll work on an alternate approach in a separate PR. |
|
Superseded by #80528, which lands the approach from the discussion here: mentions become non-interactive @Mamaduka your notification identity-check point is flagged there as a follow-up for #79606, where the mention parsing lives. |
Closes #80502.
What
Switch the Notes
@mention markup from a pair of classes to a single purpose-specific data attribute:and replace the elaborate per-note kses arm/disarm machinery in
lib/compat/wordpress-7.1/block-comments.phpwith a single always-registeredwp_kses_allowed_htmlfilter that allows only that one attribute onain thepre_comment_contentcontext.Since the
wp-note-mentionclass has never shipped in a stable WordPress release, it is removed entirely - no legacy selector or fallback is kept (per review).Why
On the Core backport of the notes mention kses handling (wordpress-develop#12503), @westonruter suggested carrying the mentioned user ID in a data attribute instead of a class. A class allowance lets note authors persist arbitrary classes on links, widening the CSS and JavaScript selector blast radius in the admin. A purpose-specific
data-wp-note-mention-userattribute is inert: it is not a styling or scripting hook anywhere outside the notes sidebar.Because the payload is now a single inert attribute rather than an open
class, the per-note-write arming and disarming (thepreprocess_comment/rest_preprocess_commentarming hooks plus the self-removingpre_comment_contentdisarm atPHP_INT_MAXand therest_request_after_callbacksbackstop) is no longer needed. It collapses to one always-on filter.Correction to the original suggestion
The premise that data attributes "would be allowed by Kses by default" holds only for the post kses context. The comment context (
pre_comment_content) uses the minimal$allowedtagsinwp-includes/kses.php, whereaallows onlyhref/titleanddata-*is not permitted (thedata-*wildcard path inwp_kses_attr_check()requires the allowlist to already containdata-*). So a narrow kses allowance is still required; it just shrinks from "any class" to one inert attribute, and the arm/disarm machinery goes away.This is proven by a new baseline unit test,
test_comment_kses_strips_data_attribute_by_default, which removes the notes filter and assertswp_kses( $mention_markup, 'pre_comment_content' )stripsdata-wp-note-mention-user.Behavior change
Regular (including anonymous) commenters can now persist this one inert
data-wp-note-mention-userattribute on links in any comment content, not just notes. This is acceptable because:note-type comments, so the attribute is meaningless anywhere else.Every other link attribute (
class, event handlers, styles, otherdata-*attributes) is still stripped exactly as in core's defaults, covered bytest_only_the_mention_attribute_and_default_link_attributes_are_allowed.Coordination follow-ups
user-Nclass in PHP (preg_match( '/^user-([1-9][0-9]*)$/' )). It should switch to reading thedata-wp-note-mention-userattribute; no legacy-class fallback is needed since the class never shipped in a stable release.pre_comment_contentdata-wp-note-mention-userallowance), at which point thefunction_exists( '_wp_kses_allow_note_mention_attributes' )guard here disables this plugin copy.Testing instructions
unfiltered_html(e.g. an author), open a post, select some text or a block, and add a note.@and pick a teammate. The mention renders as a chip.data-wp-note-mention-user="<user id>"(inspect the element) and links to the author page. This is the case the kses allowance exists for.Automated coverage:
phpunit/tests/notes-mention-kses-test.phpMentions in the note formdescribe intest/e2e/specs/editor/various/block-notes.spec.jsEditor/CollabSidebar/NoteMentionrenders the mention chip.