Skip to content

Fix: Preserve HTML anchors in Edit as HTML and Classic block conversion (7.0 regression) - #82836

Open
hbhalodia wants to merge 41 commits into
WordPress:trunkfrom
hbhalodia:fix/issue-82683
Open

hbhalodia wants to merge 41 commits into
WordPress:trunkfrom
hbhalodia:fix/issue-82683

Conversation

@hbhalodia

@hbhalodia hbhalodia commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

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 source from the anchor attribute, which is now just:

// packages/block-editor/src/hooks/anchor.jsx
anchor: {
	type: 'string',
},

Nothing can read an id out 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.

User flow Before 7.0 Trunk This PR
Type an id in Edit as HTML works recovery mode fixed
Delete an id in Edit as HTML works recovery mode fixed
Convert a Classic block containing <h2 id="x"> anchor kept anchor silently dropped fixed

class and aria-label travel 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.jsx

applyBuiltInValidationFixes is exposed through the existing locked privateApis. BlockHTML's onChange applies it when validation fails and then re-validates — the same sequence parser/index.ts uses, with the same shape (originalContent = the raw markup).

2. Let the fixes remove an attribute, not only assign one — fix-global-attribute.ts

if ( attributeValue ) {
	modifiedBlockAttributes[ supportKey ] = attributeValue;
} else if ( innerHTML ) {
	delete modifiedBlockAttributes[ supportKey ];
}

This mirrors fixCustomClassname, which already had the branch. The innerHTML guard is the analogue of its else 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.jsx

The attributes reducer for UPDATE_BLOCK merges ({ ...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 as undefined. Without this the block reports isValid: true while the store still holds the stale attribute.

This is also why deleting a class by hand in Edit as HTML never cleared className, even though fixCustomClassname has always had its delete branch. That works now too.

4. Keep the id on raw HTML conversion — html-to-blocks.ts (#83118)

htmlToBlocks never calls the fixes at all. It already hand-copied class onto className for the same underlying reason, so the id is carried onto anchor next to it, guarded by hasBlockSupport so a block without anchor support does not gain an undeclared attribute.

The class copy is hoisted out of the if ( 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. class keeps no hasBlockSupport guard because it is pre-existing code moved unchanged - harmless today, since the only raw-transform blocks with customClassName: false are core/more and core/nextpage, matched on elements specialCommentConverter generates 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, and ariaLabel.
  • 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 an id in HTML mode and then deleting it.

Testing Instructions

Typing an anchor

  1. Add a Heading block with some text.
  2. Block options (⋮) → Edit as HTML.
  3. Add 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

  1. Open Edit as HTML again, remove 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

  1. Open the code editor (Ctrl/Cmd + Shift + Alt + M) and paste:
<!-- wp:freeform -->
<h2 id="section-one">Section one</h2>
<p id="intro">Intro text</p>
<hr id="divider" class="is-fancy">
<!-- /wp:freeform -->
  1. Switch back to the visual editor, select the Classic block, then ⋮ → Convert to blocks.

Before: every anchor and the class are silently dropped.
After: the Heading has section-one, the Paragraph has intro, and the Separator has divider plus the is-fancy class.

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

  • Claude Code, Opus 5.
  • Used for diagnosis, implementation, PR description.
  • The fix was tested and reviewed by me.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

🤖 PR meta 🤖

🎉 Props

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org>
Co-authored-by: drzraf <drzraf@git.wordpress.org>
Co-authored-by: tommusrhodus <tommusrhodus@git.wordpress.org>

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 props-bot label to refresh.

🏷️ Labels

This pull request needs exactly one label indicating its type, and has 2.

  • Required: any label starting with [Type].
  • Found: [Type] Bug, [Type] Regression.

Read more about Type labels in Gutenberg. If you cannot add labels yourself, a reviewer can do it for you.

@github-actions github-actions Bot added [Package] Blocks /packages/blocks [Package] Block editor /packages/block-editor labels Sep 14, 2026
@hbhalodia
hbhalodia marked this pull request as ready for review September 14, 2026 10:42
@hbhalodia
hbhalodia requested a review from ellatrix as a code owner September 14, 2026 10:42
@hbhalodia hbhalodia added [Feature] Block API API that allows to express the block paradigm. [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Sep 14, 2026
@hbhalodia hbhalodia self-assigned this Sep 14, 2026

@t-hamano t-hamano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 ],
		} );

@hbhalodia

Copy link
Copy Markdown
Contributor Author

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.

@hbhalodia
hbhalodia requested a review from t-hamano September 14, 2026 13:16
@hbhalodia

Copy link
Copy Markdown
Contributor Author

Thanks, I have used that only when the block is invalid. It also mirrors the implmentation here in the parser

function applyBlockValidation(

@t-hamano t-hamano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@aaronrobertshaw

Copy link
Copy Markdown
Contributor

Thanks for the fix @hbhalodia 👍

✅ I could replicate the original issue
❓This PR partially fixes the Edit as HTML path

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 id prop. Prior to 7.0, an id prop could be removed as well.

I believe fixCustomClassname already handles a similar case for the class prop. Perhaps fixGlobalAttribute needs a branch that removes the attribute when the markup has none.

There should also be an e2e test for this regression as well.

User flow Before 7.0 Trunk After this PR
Type id in Edit as HTML works recovery mode fixed
Delete id in Edit as HTML works recover mode broken
Convert Classic block containing <h2 id="x"> anchor kept anchor silently dropped dropped

Given how closely related these issues are, the entire regression caused by #70993 needs to be addressed before we merge I think.

@hbhalodia

hbhalodia commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor Author

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
@hbhalodia

Copy link
Copy Markdown
Contributor Author

Hi @aaronrobertshaw @t-hamano, Can we move forward on this if everything looks good? I have addressed all the required suggestions and changes.

Thanks,

@t-hamano

Copy link
Copy Markdown
Contributor

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?

@hbhalodia

Copy link
Copy Markdown
Contributor Author

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,

@aaronrobertshaw

Copy link
Copy Markdown
Contributor

@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:

  • Rebase onto trunk, as it now conflicts.
  • Convert the new test to .ts. It does need to be done now and shouldn't take a lot.

After that, I'm happy to give this a final review.

@hbhalodia

Copy link
Copy Markdown
Contributor Author

Thanks @aaronrobertshaw, Will update this and would share for review.

@hbhalodia

Copy link
Copy Markdown
Contributor Author

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 .ts, but seems like the config is not added for the /packages/blocks yet, hence the workflow may fail.

I see /packages/blocks tests in #67691 are still are not converted to ts, hence there won't be any config related to it.

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,

@manzoorwanijk

Copy link
Copy Markdown
Member

I see /packages/blocks tests in #67691 are still are not converted to ts, hence there won't be any config related to it.

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.

Static Analysis CI check won’t let this PR to be merged unless the new files are in .ts

@hbhalodia

Copy link
Copy Markdown
Contributor Author

I see /packages/blocks tests in #67691 are still are not converted to ts, hence there won't be any config related to it.
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.

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 packages/blocks tests to ts and update this branch with that and then merge this PR?

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?

@manzoorwanijk

Copy link
Copy Markdown
Member

Should we first plan to migrate all packages/blocks tests to ts and update this branch with that and then merge this PR?

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.

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?

We could do that but I would prefer to avoid going that route.

@hbhalodia

Copy link
Copy Markdown
Contributor Author

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.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Block API API that allows to express the block paradigm. [Package] Block editor /packages/block-editor [Package] Blocks /packages/blocks [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Projects

None yet

4 participants