Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 55 additions & 9 deletions src/blocks/tabs/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
* Parent block that manages tab navigation and panels
*/

import { __ } from '@wordpress/i18n';
import { __, sprintf } from '@wordpress/i18n';
import {
useBlockProps,
InspectorControls,
useInnerBlocksProps,
RichText,
store as blockEditorStore,
// eslint-disable-next-line @wordpress/no-unsafe-wp-apis
__experimentalColorGradientSettingsDropdown as ColorGradientSettingsDropdown,
Expand Down Expand Up @@ -76,8 +77,8 @@
[clientId]
);

// Get dispatch actions for selecting blocks
const { selectBlock } = useDispatch(blockEditorStore);
// Get dispatch actions for selecting blocks and updating child attributes
const { selectBlock, updateBlockAttributes } = useDispatch(blockEditorStore);

Check failure on line 81 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.1)

Replace `·` with `⏎↹↹`

Check failure on line 81 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8)

Replace `·` with `⏎↹↹`

Check failure on line 81 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.2)

Replace `·` with `⏎↹↹`

// Handle tab click - set active tab and select the Tab block to show its settings
const handleTabClick = (index) => {
Expand All @@ -91,6 +92,20 @@

// Handle keyboard navigation
const handleKeyDown = (e, index) => {
// Don't interfere with text editing in RichText
if (e.target.closest('[contenteditable="true"]')) {
return;
}

// Handle Enter/Space for tab activation (divs need explicit handling unlike buttons)
Copy link

Choose a reason for hiding this comment

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

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.

if (e.key === 'Enter' || e.key === ' ') {
if (index !== activeTab) {
handleTabClick(index);
}
e.preventDefault();
return;
}

let newIndex = index;

if (orientation === 'horizontal') {
Expand Down Expand Up @@ -442,7 +457,7 @@
const isActive = index === activeTab;

return (
<button
<div
Copy link

Choose a reason for hiding this comment

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

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.

key={block.clientId}
className={`dsgo-tabs__tab ${isActive ? 'is-active' : ''} ${
icon ? `has-icon icon-${iconPosition}` : ''
Expand All @@ -453,7 +468,11 @@
aria-controls={`panel-${tabId}`}
tabIndex={isActive ? 0 : -1}
data-tab-index={index}
onClick={() => handleTabClick(index)}
onClick={() => {
Copy link

Choose a reason for hiding this comment

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

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.

if (!isActive) {
handleTabClick(index);
}
}}
onKeyDown={(e) => handleKeyDown(e, index)}
>
{icon && iconPosition === 'left' && (
Expand All @@ -468,16 +487,43 @@
</span>
)}

<span className="dsgo-tabs__tab-title">
{title || `Tab ${index + 1}`}
</span>
{isActive ? (
<RichText
Copy link

Choose a reason for hiding this comment

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

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)}

tagName="span"
className="dsgo-tabs__tab-title"
value={title}
onChange={(value) => {
// Strip any residual HTML to ensure plain text storage
const plainText = value.replace(/<[^>]*>/g, '');

Check failure on line 497 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.1)

Replace `/<[^>]*>/g,·''` with `⏎↹↹↹↹↹↹↹↹↹↹↹↹/<[^>]*>/g,⏎↹↹↹↹↹↹↹↹↹↹↹↹''⏎↹↹↹↹↹↹↹↹↹↹↹`

Check failure on line 497 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8)

Replace `/<[^>]*>/g,·''` with `⏎↹↹↹↹↹↹↹↹↹↹↹↹/<[^>]*>/g,⏎↹↹↹↹↹↹↹↹↹↹↹↹''⏎↹↹↹↹↹↹↹↹↹↹↹`

Check failure on line 497 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.2)

Replace `/<[^>]*>/g,·''` with `⏎↹↹↹↹↹↹↹↹↹↹↹↹/<[^>]*>/g,⏎↹↹↹↹↹↹↹↹↹↹↹↹''⏎↹↹↹↹↹↹↹↹↹↹↹`

Check failure

Code scanning / CodeQL

Incomplete multi-character sanitization High

This string may still contain
<script
, which may cause an HTML element injection vulnerability.

Copilot Autofix

AI 19 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:

  1. Add an import for striptags at the top of src/blocks/tabs/edit.js.
  2. Replace the value.replace(/<[^>]*>/g, '') call with striptags(value), keeping the rest of the logic intact.

This keeps the functionality (“store only plain text titles”) while removing the insecure, fragile regex.


Suggested changeset 2
src/blocks/tabs/edit.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/blocks/tabs/edit.js b/src/blocks/tabs/edit.js
--- a/src/blocks/tabs/edit.js
+++ b/src/blocks/tabs/edit.js
@@ -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 }
EOF
@@ -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 }
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -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": [
EOF
@@ -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": [
This fix introduces these dependencies
Package Version Security advisories
striptags (npm) 3.2.0 None
Copilot is powered by AI and may make mistakes. Always verify output.
updateBlockAttributes(
block.clientId,
{ title: plainText }
);
}}
placeholder={sprintf(
/* translators: %d: tab number */
__('Tab %d', 'designsetgo'),
index + 1
)}
allowedFormats={[]}
withoutInteractiveFormatting
/>
) : (
<span className="dsgo-tabs__tab-title">
{title || sprintf(

Check failure on line 513 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.1)

Replace `·` with `⏎↹↹↹↹↹↹↹↹↹↹↹`

Check failure on line 513 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8)

Replace `·` with `⏎↹↹↹↹↹↹↹↹↹↹↹`

Check failure on line 513 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.2)

Replace `·` with `⏎↹↹↹↹↹↹↹↹↹↹↹`
/* translators: %d: tab number */

Check failure on line 514 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.1)

Insert `↹`

Check failure on line 514 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8)

Insert `↹`

Check failure on line 514 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.2)

Insert `↹`
__('Tab %d', 'designsetgo'),

Check failure on line 515 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.1)

Insert `↹`

Check failure on line 515 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8)

Insert `↹`

Check failure on line 515 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.2)

Insert `↹`
index + 1

Check failure on line 516 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.1)

Insert `↹`

Check failure on line 516 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8)

Insert `↹`

Check failure on line 516 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.2)

Insert `↹`
)}

Check failure on line 517 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.1)

Insert `↹`

Check failure on line 517 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8)

Insert `↹`

Check failure on line 517 in src/blocks/tabs/edit.js

View workflow job for this annotation

GitHub Actions / Build and Test (20.x, 8.2)

Insert `↹`
</span>
)}

{icon && iconPosition === 'right' && (
<span className="dsgo-tabs__tab-icon">
{getIcon(icon)}
</span>
)}
</button>
</div>
);
})}
</div>
Expand Down
23 changes: 20 additions & 3 deletions src/blocks/tabs/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,11 @@
font-weight: 500;
transition: all 0.2s ease;

// Remove focus outline
&:focus,
// Subtle focus indicator for keyboard navigation (WCAG 2.1)
&:focus-visible {
outline: none;
outline: 2px solid var(--dsgo-tab-color-active, var(--wp--preset--color--accent-2, #2563eb));
outline-offset: -2px;
border-radius: 2px;
}

// Subtle hover effect
Expand All @@ -80,6 +81,22 @@
color: var(--dsgo-tab-color-active, var(--wp--preset--color--accent-2, #2563eb));
font-weight: 600;
}

// Inline-editable tab title (RichText) — cursor only on text, not icons/padding
&.is-active .dsgo-tabs__tab-title {
cursor: text;

[contenteditable] {
outline: none;
cursor: text;
}

&:focus-within {
outline: 1px dashed var(--dsgo-tab-color-active, var(--wp--preset--color--accent-2, #2563eb));
outline-offset: 2px;
border-radius: 2px;
}
Comment on lines 89 to 98
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
}

.dsgo-tabs__tab-icon {
Expand Down
Loading