Make tabs block titles inline-editable with RichText#199
Make tabs block titles inline-editable with RichText#199jnealey-godaddy wants to merge 2 commits intomainfrom
Conversation
Users can now click directly on tab text to edit it in place, instead of having to use the sidebar settings panel. Uses RichText (plain-text mode) on the active tab while inactive tabs remain static and clickable for switching. Keyboard navigation is preserved with contentEditable awareness. https://claude.ai/code/session_015GgNeB8zHXsLigtEUzkaBw
|
Claude finished @jnealey88's task in 2m 15s —— View job Code Review Complete ✅I've completed a comprehensive security and code quality review of this PR. Here's what I found: SummaryIssues Found: 7 total
Critical Issues (Must Fix)
High Priority Issues (Strongly Recommended)
Medium Priority Issues (Should Fix)
Low Priority Issues (Nice to Have)
What I Checked✅ Security Review
✅ Logic and Correctness
✅ WordPress Best Practices
✅ Code Quality
Overall AssessmentThis PR adds a useful feature (inline tab editing), but has one critical accessibility issue that must be addressed before merging. The button→div change violates semantic HTML and WCAG 2.1 AA standards. Recommendation: Fix the high-priority accessibility issue by keeping The inline comments above contain detailed fixes for each issue. Please review and address at least the Critical and High priority items before merging. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/blocks/tabs/edit.js
Outdated
| // Handle keyboard navigation | ||
| const handleKeyDown = (e, index) => { | ||
| // Don't interfere with text editing in RichText | ||
| if (e.target.isContentEditable) { |
There was a problem hiding this comment.
SEVERITY: Medium
Issue: The check e.target.isContentEditable may not be reliable for RichText components.
Problem: RichText wraps content in a contenteditable element, but the event target might be a child element. This could cause keyboard navigation to still interfere with text editing if the user is editing deeply nested content.
Fix: Use a more robust check:
if (e.target.isContentEditable || e.target.closest('[contenteditable="true"]')) {
return;
}This ensures that if the target or any parent is contenteditable, we don't interfere with editing.
|
|
||
| return ( | ||
| <button | ||
| <div |
There was a problem hiding this comment.
SEVERITY: High - Accessibility Issue
Issue: Changing from <button> to <div> breaks semantic accessibility.
Problem: While ARIA attributes (role="tab") are present, divs are not keyboard-focusable by default and don't convey the same semantics to assistive technologies. This violates WCAG 2.1 AA standards mentioned in the PR checklist. Screen readers expect tabs to be implemented with native interactive elements or proper focus management.
Fix: Keep the <button> element and nest the RichText inside:
<button
className={`dsgo-tabs__tab ${isActive ? 'is-active' : ''}`}
role="tab"
aria-selected={isActive}
// ... other props
>
{isActive ? (
<RichText
tagName="span"
value={title}
onChange={(value) => updateBlockAttributes(block.clientId, { title: value })}
onClick={(e) => e.stopPropagation()}
onFocus={(e) => e.stopPropagation()}
/>
) : (
<span>{title || `Tab ${index + 1}`}</span>
)}
</button>You'll need to stop propagation on RichText click/focus events to prevent the button from handling those events.
src/blocks/tabs/edit.js
Outdated
| tagName="span" | ||
| className="dsgo-tabs__tab-title" | ||
| value={title} | ||
| onChange={(value) => |
There was a problem hiding this comment.
SEVERITY: Critical - XSS Vulnerability
Issue: Missing sanitization on RichText output.
Problem: The title value from block.attributes is rendered directly without sanitization. While RichText with allowedFormats={[]} prevents formatting, it doesn't prevent malicious HTML/script injection if the attribute is manipulated through REST API or direct database modification.
WordPress Security Best Practice: All dynamic content should be sanitized before rendering, even in the editor.
Fix: While RichText handles basic sanitization internally, you should ensure the stored value is sanitized. Add validation to the block's attributes definition in block.json:
"title": {
"type": "string",
"default": "",
"source": "text", // This ensures only text is extracted, no HTML
"selector": ".dsgo-tabs__tab-title"
}If using source: "html", you must sanitize with wp_kses_post() in PHP or use sanitizeHTML() in JavaScript.
| return; | ||
| } | ||
|
|
||
| // Handle Enter/Space for tab activation (divs need explicit handling unlike buttons) |
There was a problem hiding this comment.
SEVERITY: Medium - Logic Issue
Issue: Space key handling conflicts with text editing.
Problem: If a user wants to type a space character while editing the tab title, this handler will prevent it and try to activate the tab (though the contentEditable check should catch it, it's fragile).
Fix: Be more explicit about when to handle Space:
// Handle Enter/Space for tab activation (only when NOT editing)
if (!e.target.isContentEditable && (e.key === 'Enter' || e.key === ' ')) {
handleTabClick(index);
e.preventDefault();
return;
}Move the contentEditable check into the condition rather than relying on early return for all subsequent keyboard handling.
| tabIndex={isActive ? 0 : -1} | ||
| data-tab-index={index} | ||
| onClick={() => handleTabClick(index)} | ||
| onClick={() => { |
There was a problem hiding this comment.
SEVERITY: Low - Code Quality
Issue: Inconsistent event handling pattern.
Problem: The onClick handler checks if (!isActive) before calling handleTabClick, but this prevents users from clicking an active tab to focus it (which might be expected behavior for keyboard users or screen reader users).
Observation: This is intentional per the PR description ("prevent unnecessary re-selection"), but it creates an inconsistency where clicking an inactive tab selects its content block, but clicking an active tab does nothing.
Suggestion: Consider if this is the desired UX. Users might expect clicking the active tab to focus the RichText for editing. If that's the intent, the current implementation is correct.
| {title || `Tab ${index + 1}`} | ||
| </span> | ||
| {isActive ? ( | ||
| <RichText |
There was a problem hiding this comment.
SEVERITY: Low - WordPress Best Practice
Issue: Missing internationalization for placeholder text.
Problem: The placeholder Tab ${index + 1} is not wrapped in a translation function, but similar text elsewhere uses __('Tab 1', 'designsetgo').
WordPress i18n Best Practice: All user-facing strings should be translatable.
Fix:
```javascript 'designsetgo') + ${index + 1}}
Or use sprintf for better translation control:
```javascript
import { sprintf, __ } from '@wordpress/i18n';
%d', 'designsetgo'), index + 1)}
src/blocks/tabs/editor.scss
Outdated
| background: var(--dsgo-tab-bg-active, transparent); | ||
| color: var(--dsgo-tab-color-active, var(--wp--preset--color--accent-2, #2563eb)); | ||
| font-weight: 600; | ||
| cursor: text; |
There was a problem hiding this comment.
SEVERITY: Low - Accessibility
Issue: Setting cursor: text on the entire tab element may be confusing.
Problem: When hovering over icon areas or padding, the cursor shows text insertion cursor even though those areas aren't editable. This creates a mismatch between visual affordance and actual functionality.
Fix: Apply cursor: text only to the editable title element:
&.is-active {
background: var(--dsgo-tab-bg-active, transparent);
color: var(--dsgo-tab-color-active, var(--wp--preset--color--accent-2, #2563eb));
font-weight: 600;
// Remove cursor: text from here
}
// Inline-editable tab title (RichText)
&.is-active .dsgo-tabs__tab-title {
cursor: text; // Keep only here
// ...
}There was a problem hiding this comment.
Pull request overview
This PR updates the Tabs Gutenberg block editor experience by enabling inline editing of tab titles using RichText, replacing the previous inspector-only title editing workflow.
Changes:
- Adds
RichTextrendering for the active tab title and updates the child tab block title viaupdateBlockAttributes. - Switches tab triggers from
<button>to<div role="tab">and extends keyboard handling to support Enter/Space activation. - Adds editor styling to support inline-edit cursor/focus behavior for the active tab title.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/blocks/tabs/edit.js |
Introduces inline RichText editing for active tab titles and updates keyboard/click behavior for tab triggers. |
src/blocks/tabs/editor.scss |
Adds editor-only styles for the editable tab title (cursor + focus/outline rules). |
src/blocks/tabs/edit.js
Outdated
|
|
||
| // Handle Enter/Space for tab activation (divs need explicit handling unlike buttons) | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| handleTabClick(index); |
There was a problem hiding this comment.
The new Enter/Space handler always calls handleTabClick(index), even when this tab is already active. That’s now inconsistent with the click handler (which skips re-selection when active) and can cause unnecessary selectBlock calls / focus churn while the active tab is focused. Consider guarding Enter/Space the same way (no-op when index === activeTab) or passing isActive into handleKeyDown and skipping activation when already selected.
| handleTabClick(index); | |
| if (index !== activeTab) { | |
| handleTabClick(index); | |
| } |
src/blocks/tabs/edit.js
Outdated
| placeholder={`Tab ${index + 1}`} | ||
| allowedFormats={[]} | ||
| withoutInteractiveFormatting |
There was a problem hiding this comment.
placeholder={Tab ${index + 1}} introduces a new user-facing string that isn’t localized. Since this file already uses __() and the repo localizes placeholders elsewhere, this should use __()/sprintf() (with a translators comment) so the default tab label can be translated.
| [contenteditable] { | ||
| outline: none; | ||
| cursor: text; | ||
| } | ||
|
|
||
| &:focus-within { | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
The new [contenteditable] { outline: none; } and :focus-within { outline: none; } remove the visible focus indicator for the inline-editable tab title. This makes keyboard navigation/editing harder and can fail WCAG 2.1 focus-visible expectations. Please keep a visible focus style (or replace the outline with an equivalent high-contrast focus ring) instead of removing it entirely.
- Use closest('[contenteditable="true"]') instead of isContentEditable
for more reliable nested RichText detection
- Guard Enter/Space handler to skip when tab is already active,
matching the click handler behavior
- Wrap placeholder text with sprintf/__() for i18n support
- Strip HTML tags in onChange to ensure plain text storage
- Move cursor:text from entire tab to title span only
- Restore WCAG 2.1 focus-visible indicator on tabs instead of
removing outlines entirely
- Add subtle dashed outline on title focus-within for editing feedback
https://claude.ai/code/session_015GgNeB8zHXsLigtEUzkaBw
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| value={title} | ||
| onChange={(value) => { | ||
| // Strip any residual HTML to ensure plain text storage | ||
| const plainText = value.replace(/<[^>]*>/g, ''); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 18 days ago
In general, the right fix is to avoid hand‑rolled HTML “sanitization” via regex and instead either: (a) rely on WordPress’s built‑in escaping/sanitization, or (b) convert the input to plain text using a proper HTML parser/converter. For this specific case, the intent (per the comment) is “ensure plain text storage” for tab titles. We can achieve that by converting from HTML to plain text before storing, instead of trying to strip tags with a single regex.
Within src/blocks/tabs/edit.js, on line 497 we currently do:
// Strip any residual HTML to ensure plain text storage
const plainText = value.replace(/<[^>]*>/g, '');The best, minimal‑behavior‑change fix is to use a robust HTML‑to‑text utility. Since we can only add well‑known libraries and not alter other files, we can import a small, widely‑used HTML parsing/conversion package (e.g. striptags) and delegate the tag removal to it. striptags correctly handles nested tags and does not suffer from the multi‑character replacement issue that CodeQL warns about.
Concrete changes:
- Add an import for
striptagsat the top ofsrc/blocks/tabs/edit.js. - Replace the
value.replace(/<[^>]*>/g, '')call withstriptags(value), keeping the rest of the logic intact.
This keeps the functionality (“store only plain text titles”) while removing the insecure, fragile regex.
| @@ -25,6 +25,7 @@ | ||
| import { useSelect, useDispatch } from '@wordpress/data'; | ||
| import { useEffect } from '@wordpress/element'; | ||
| import { getIcon } from '../icon/utils/svg-icons'; | ||
| import striptags from 'striptags'; | ||
|
|
||
| const ALLOWED_BLOCKS = ['designsetgo/tab']; | ||
|
|
||
| @@ -494,7 +495,7 @@ | ||
| value={title} | ||
| onChange={(value) => { | ||
| // Strip any residual HTML to ensure plain text storage | ||
| const plainText = value.replace(/<[^>]*>/g, ''); | ||
| const plainText = striptags(value); | ||
| updateBlockAttributes( | ||
| block.clientId, | ||
| { title: plainText } |
| @@ -74,7 +74,8 @@ | ||
| "@wordpress/i18n": "^5.0.0", | ||
| "@wordpress/icons": "^10.0.0", | ||
| "classnames": "^2.3.2", | ||
| "countup.js": "^2.9.0" | ||
| "countup.js": "^2.9.0", | ||
| "striptags": "^3.2.0" | ||
| }, | ||
| "lint-staged": { | ||
| "*.js": [ |
| Package | Version | Security advisories |
| striptags (npm) | 3.2.0 | None |
Description
This PR enhances the tabs block by making tab titles directly editable inline using WordPress's RichText component. Previously, tab titles could only be edited through the block inspector. Now, users can click on the active tab title to edit it directly, improving the editing experience.
Type of Change
Related Issue
Closes #
Changes Made
RichTextcomponent from@wordpress/block-editorupdateBlockAttributesto the dispatch actions fromblockEditorStore<button>to<div>to allow nested RichText editingTesting
Screenshots/Videos
Checklist
__()or_e()Additional Notes
The change from
<button>to<div>maintains accessibility through proper ARIA attributes (role="tab",aria-selected,aria-controls) and keyboard navigation support. RichText is only active on the selected tab to prevent multiple simultaneous edits and maintain a clean UX.https://claude.ai/code/session_015GgNeB8zHXsLigtEUzkaBw