Repository navigation
Conversation
🤖 PR meta 🤖🎉 PropsIf 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. Updated as activity occurs, without notifying anyone named here. Add the 🏷️ LabelsThis pull request needs exactly one label indicating its type, and has 2.
Read more about Type labels in Gutenberg. If you cannot add labels yourself, a reviewer can do it for you. |
t-hamano
left a comment
There was a problem hiding this comment.
Thanks for the PR. How about only applying applyBuiltInValidationFixes when the block validation fails? Something like this:
const content = html ? html : getSaveContent( blockType, attributes );
const updatedBlock = {
...block,
attributes,
originalContent: content,
};
const [ isValid ] = html ? validateBlock( updatedBlock ) : [ true ];
// `getBlockAttributes` only sources what the save output declares,
// so recover hand-typed `id`/`class`/`aria-label` for invalid blocks.
const fixedBlock = isValid
? updatedBlock
: applyBuiltInValidationFixes( updatedBlock, blockType );
updateBlock( clientId, {
attributes: fixedBlock.attributes,
originalContent: content,
isValid: isValid || validateBlock( fixedBlock )[ 0 ],
} );
Thanks, Let me check on this and confirm. |
|
Thanks, I have used that only when the block is invalid. It also mirrors the implmentation here in the parser |
t-hamano
left a comment
There was a problem hiding this comment.
This PR looks good to me once the merge conflicts are resolved.
@aaronrobertshaw, If you have the bandwidth, I'd appreciate it if you could do a second review.
|
Thanks for the fix @hbhalodia 👍 ✅ I could replicate the original issue Following the test instructions this fix appears to work, and it does, but the regression introduced by #70993 is actually much larger that the scope addressed by this PR. The repair step added here for Edit as HTML and to the parser in #70993 both only account for the addition of an I believe There should also be an e2e test for this regression as well.
Given how closely related these issues are, the entire regression caused by #70993 needs to be addressed before we merge I think. |
|
Thanks @aaronrobertshaw, I have filed an issue related to it here - #83118, I would like to add a fix onto this same PR and link the PR with that issue as well. Let me know if that sounds good? |
… sliently dropping or failed validation error
|
Hi @aaronrobertshaw @t-hamano, Can we move forward on this if everything looks good? I have addressed all the required suggestions and changes. Thanks, |
|
Thanks for the updates, @hbhalodia. All the feedback looks addressed from my side, and my approval still stands. @aaronrobertshaw, the blur guard has been added, and the parser/deprecation case has been split out into #83603. Are you okay with handling that as a follow-up so we can land the regression fix first? |
|
Hi @t-hamano, Sorry for the ping. There is a failing test for static analysis linting - https://github.com/WordPress/gutenberg/actions/runs/39236108529/job/118006006503, hinting to convert the file to typescript. I think it would be better to keep it for another PR instead of updating it here. Let me know if we need to update on this PR, happy to update it. Thanks, |
|
@t-hamano, sorry for the slow reply. Reconsidering this, I'd prefer not to split #83603 out. The nested anchor case isn't part of the #70993 regression. It comes from the new delete branch this PR adds. Merged alone, this PR swaps a visible failure (recovery mode) for a silent one (a lost anchor). The scope is narrow, but silent data loss is what a regression fix shouldn't add. @hbhalodia, you've already written the fix in #83605, and it's a small commit on top of this branch. Could you fold that commit into this PR? Then we can close #83605 and #83603 as covered. Two other things before it's ready:
After that, I'm happy to give this a final review. |
|
Thanks @aaronrobertshaw, Will update this and would share for review. |
|
Hi @aaronrobertshaw @t-hamano, This is updated with all the required changes and tested from my end. For new tests added: I have conveted it to I see Instead of adding the entire config updates here, I plan to just ship this and whenever that is picked up, the config would be added to blocks package as well for tests like the other ones. Thanks, |
Static Analysis CI check won’t let this PR to be merged unless the new files are in .ts |
Thanks @manzoorwanijk for review. What do you think would be the best approach here? Should we first plan to migrate all Or if we can supress the particular file if there is any place, so the one current test in ts and CI can pass, and other tests would go plan as expected in the issue - #67691? |
That is one way to do it. But, I would suggest converting just one test and updating the tsconfig files and then updating this PR. I would be happy to review that PR.
We could do that but I would prefer to avoid going that route. |
Thanks, I would do that and would raise the PR. I would add the comment to the tracking issue about it. So other contibutors can get to know. |
What?
Closes #82683, #83603
Fixes the anchor regression from #70993 across every path that turns HTML into blocks: the "Edit as HTML" field, the code editor, and Classic block conversion.
Also, fixes the issue regressed by fixing the regression in commit - 3e50fab, Details added in issue - #83603
Based on this comment, Closed the PR and port the changes here in the PR.
Why?
#70993 moved the anchor into the block comment delimiter and dropped the
sourcefrom theanchorattribute, which is now just:Nothing can read an
idout of markup any more.applyBuiltInValidationFixes()shipped in that PR to recover it, but only the parser calls it, and it can only ever assign an attribute - it has no way to record that one was removed.idin Edit as HTMLidin Edit as HTML<h2 id="x">classandaria-labeltravel the same code path and had the same gap, so they are covered alongside.How?
1. Reach the fixes from Edit as HTML —
packages/blocks/src/api/index.ts,block-html.jsxapplyBuiltInValidationFixesis exposed through the existing lockedprivateApis.BlockHTML'sonChangeapplies it when validation fails and then re-validates — the same sequenceparser/index.tsuses, with the same shape (originalContent= the raw markup).2. Let the fixes remove an attribute, not only assign one —
fix-global-attribute.tsThis mirrors
fixCustomClassname, which already had the branch. TheinnerHTMLguard is the analogue of itselse if ( serialized ): a block that saves no markup has nothing to compare against, so a dynamic block keeps the anchor held in its delimiter.3. Make that removal survive
updateBlock—block-html.jsxThe
attributesreducer forUPDATE_BLOCKmerges ({ ...current, ...updates.attributes }), so a deleted key is simply absent from the incoming object and the old value survives the spread. Keys the fixes dropped are therefore sent explicitly asundefined. Without this the block reportsisValid: truewhile the store still holds the stale attribute.This is also why deleting a
classby hand in Edit as HTML never clearedclassName, even thoughfixCustomClassnamehas always had its delete branch. That works now too.4. Keep the
idon raw HTML conversion —html-to-blocks.ts(#83118)htmlToBlocksnever calls the fixes at all. It already hand-copiedclassontoclassNamefor the same underlying reason, so theidis carried ontoanchornext to it, guarded byhasBlockSupportso a block without anchor support does not gain an undeclared attribute.The
classcopy is hoisted out of theif ( transform )branch in the process, so it also applies to raw transforms that declare only a selector (core/separator,core/code,core/preformatted). That gap dates to #60331 rather than #70993; it is included here because the two lines now sit together and splitting them would be arbitrary.classkeeps nohasBlockSupportguard because it is pre-existing code moved unchanged - harmless today, since the only raw-transform blocks withcustomClassName: falsearecore/moreandcore/nextpage, matched on elementsspecialCommentConvertergenerates itself.Tests
packages/blocks/src/api/parser/test/fix-global-attribute.jsdom.test.js— new file; the helper had no coverage. Assign, replace, remove, the no-markup guard, non-mutation, andariaLabel.test/integration/blocks-raw-handling.jsdom.test.js— anchors preserved on heading and paragraph, and anchor plus class on a selector-only transform.test/e2e/specs/editor/various/editor-modes.spec.js- typing anidin HTML mode and then deleting it.Testing Instructions
Typing an anchor
id="test"to the heading, then click outside the field.Before: the block drops into recovery mode.
After: the block validates, and Advanced → HTML anchor shows
test.Deleting an anchor
id="test", then click outside.Before: the block drops into recovery mode.
After: the block validates, and Advanced → HTML anchor is empty.
Classic block conversion
Ctrl/Cmd + Shift + Alt + M) and paste:Before: every anchor and the class are silently dropped.
After: the Heading has
section-one, the Paragraph hasintro, and the Separator hasdividerplus theis-fancyclass.Testing Instructions for Keyboard
N/A
Screenshots or screencast
The issue's screencast shows the before state: https://d.pr/v/BTYKGl
After
Screen.Recording.2026-09-14.at.4.06.10.PM.mov
Screen.Recording.2026-09-18.at.1.14.43.PM.mov
Use of AI Tools
🤖 Generated with Claude Code