Feat/shared dhtml toolbar - #151
Open
mambax7 wants to merge 2 commits into
Open
Conversation
Clicking Size, Font, Colour or B/I/U/S with no text selected did nothing at all. The no-selection branch called setVisible() on an element with the id held in $_hiddenText (default xoopsHiddenText), but no renderer has ever emitted such an element, so xoopsGetElementById() returned null and the handler threw a TypeError before it could act. For xoopsSetElementAttribute() that branch was dead twice over: besides the missing element, it eval'd a setElementSize/Font/Color() helper and no setElement* function exists in the file. It is removed, so with no selection the tag pair is simply inserted at the caret, which is what clicking the button asks for. xoopsMakeStyle() keeps its branch for any custom renderer that does emit the element, but only takes it when the element and the handler both genuinely exist, and otherwise falls through to inserting the pair. Both eval() calls are gone with it. The remaining dispatch is an explicit map of the four style helpers, which is all the callers ever pass. xoopsGetSelect() returns null on browsers without a selection API, so the selection is normalised to an empty string before use: without that, removing the old length guard would have inserted the literal text "null" between the tags. xoopsCodeText() is a leftover from an older toolbar layout that had a separate "add text" box. Nothing in core or any bundled extension calls it and no renderer emits the elements it reads, so every path through it dereferenced null. It is kept, because it is a global a third-party module could still call, but now returns harmlessly instead of throwing.
The same editor showed a different toolbar in the control panel than on the
front end, and different again between front-end themes. The cause was not
CSS: renderFormDhtmlTAXoopsCode() and renderFormDhtmlTATypography() were
independently hand-written HTML in five renderers, and no admin theme selects
a renderer, so the control panel silently fell back to the Legacy one whose
Bootstrap class names no admin theme defines.
XoopsDhtmlToolbar now produces the toolbar once and all five renderers
delegate to it. Only renderFormDhtmlTextArea() is on XoopsFormRendererInterface;
the other two are protected helpers, so they are kept as thin delegates for any
third-party subclass that calls or overrides them. Each renderer still supplies
its own chrome -- the textarea, the preview fieldset and the script loader.
The markup is framework-neutral with its own xo-edtb-* classes and a
self-contained stylesheet, because the admin themes load no CSS framework at
all; every colour, radius and size is a custom property so a theme can restyle
it without touching markup. Dropdowns are native <details>, grouped per
textarea with name= so opening one closes the others, with a small script for
older engines plus click-outside and Escape handling. The typography row is
emitted server-side rather than written by document.write, and the colour list
is a curated palette in a class constant instead of 216 entries generated by a
nested loop. role="toolbar" and role="group" are now present, which only the
Bootstrap 5 renderer had.
Four contracts are preserved deliberately. The codeicon preload event still
fires with $code by reference at the same point, so modules can keep appending
buttons. TextSanitizer extensions keep their [$html, $js] return shape, and the
three divergent rewrites of their hardcoded btn-default classes become one.
The global JS function names are untouched, since extensions call them by name.
XoopsFormDhtmlTextArea's public surface is unchanged.
The stylesheet also restores resize:vertical on the editor's textarea: the
default and transition admin themes ship a universal `* { resize:none }` reset
which stripped the grip, so the control panel's editor could not be resized
while the front end's could.
A test asserts the substantive property: all five renderers emit an identical
toolbar for the same element.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Reviewer's GuideIntroduce a shared, framework-neutral XoopsDhtmlToolbar for the DHTML textarea editor and refactor all form renderers and JS helpers to use it consistently, with supporting CSS/JS assets and unit tests, plus a small test bootstrap fix for locale registry and constants. Sequence diagram for rendering DHTML textarea with shared toolbarsequenceDiagram
participant Controller
participant Renderer as XoopsFormRenderer*
participant Toolbar as XoopsDhtmlToolbar
participant Element as XoopsFormDhtmlTextArea
Controller->>Renderer: renderFormDhtmlTextArea(Element)
Renderer->>Toolbar: new XoopsDhtmlToolbar()
Renderer->>Toolbar: render(Element)
activate Toolbar
Toolbar->>Toolbar: injectStylesheet()
Toolbar->>Toolbar: renderCodeButtons(Element)
Toolbar->>Toolbar: renderTypography(Element)
Toolbar->>Toolbar: renderCheckLength(Element)
Toolbar-->>Renderer: toolbarHtml
deactivate Toolbar
Renderer->>Renderer: build <textarea> markup
Renderer-->>Controller: toolbarHtml + textareaHtml
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #151 +/- ##
============================================
+ Coverage 19.29% 19.56% +0.27%
+ Complexity 8227 8190 -37
============================================
Files 672 672
Lines 44266 43831 -435
============================================
+ Hits 8539 8576 +37
+ Misses 35727 35255 -472 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In XoopsDhtmlToolbar::renderCodeButtons() the imagemanager and smilies URLs now use $element->getName() directly without rawurlencode(), whereas the Tailwind renderer previously encoded the target ID; consider restoring URL encoding to avoid issues when textarea IDs contain characters that are not URL-safe.
- Each renderer currently creates a new XoopsDhtmlToolbar instance on every renderFormDhtmlTextArea() call; if these toolbars become heavier over time, you may want to share a single instance (e.g. via a static accessor) to avoid repeated construction and keep renderer wiring lean.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In XoopsDhtmlToolbar::renderCodeButtons() the imagemanager and smilies URLs now use $element->getName() directly without rawurlencode(), whereas the Tailwind renderer previously encoded the target ID; consider restoring URL encoding to avoid issues when textarea IDs contain characters that are not URL-safe.
- Each renderer currently creates a new XoopsDhtmlToolbar instance on every renderFormDhtmlTextArea() call; if these toolbars become heavier over time, you may want to share a single instance (e.g. via a static accessor) to avoid repeated construction and keep renderer wiring lean.
## Individual Comments
### Comment 1
<location path="htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.php" line_range="153-154" />
<code_context>
+ $textareaId = $element->getName();
+ $btn = self::BTN_SM;
+
+ $code = '<a name="moresmiley"></a>';
+ $code .= '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _XOOPS_FORM_ALT_IMG . '">';
+ $code .= $this->button($btn, $this->jsCall('xoopsCodeUrl', [$textareaId, _ENTERURL, _ENTERWEBTITLE]), _XOOPS_FORM_ALT_URL, 'fa-solid fa-link');
+ $code .= $this->button($btn, $this->jsCall('xoopsCodeEmail', [$textareaId, _ENTEREMAIL, _ENTERWEBTITLE]), _XOOPS_FORM_ALT_EMAIL, 'fa-solid fa-envelope');
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The aria-label for the code button group is tied to the image alt text, which is misleading for screen readers.
The group’s aria-label `_XOOPS_FORM_ALT_IMG` doesn’t match its contents (URL, email, image, smilies, extension, code, quote), so screen readers treat the whole group as image-related. Please use a more generic label (e.g. a new constant like `_XOOPS_FORM_ARIA_CODEBAR`) or remove the group label and rely on the individual button labels instead.
Suggested implementation:
```
$code = '<a name="moresmiley"></a>';
$code .= '<div class="' . self::GROUP_CLASS . '" role="group">';
```
If you prefer a descriptive group label instead of relying solely on individual button labels, you can:
1. Define a new language constant (e.g. `_XOOPS_FORM_ARIA_CODEBAR`) in the appropriate language file.
2. Replace the edited line with:
`$code .= '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _XOOPS_FORM_ARIA_CODEBAR . '">';`
This keeps the toolbar accessible while making the label semantically accurate.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+153
to
+154
| $code = '<a name="moresmiley"></a>'; | ||
| $code .= '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _XOOPS_FORM_ALT_IMG . '">'; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The aria-label for the code button group is tied to the image alt text, which is misleading for screen readers.
The group’s aria-label _XOOPS_FORM_ALT_IMG doesn’t match its contents (URL, email, image, smilies, extension, code, quote), so screen readers treat the whole group as image-related. Please use a more generic label (e.g. a new constant like _XOOPS_FORM_ARIA_CODEBAR) or remove the group label and rely on the individual button labels instead.
Suggested implementation:
$code = '<a name="moresmiley"></a>';
$code .= '<div class="' . self::GROUP_CLASS . '" role="group">';
If you prefer a descriptive group label instead of relying solely on individual button labels, you can:
- Define a new language constant (e.g.
_XOOPS_FORM_ARIA_CODEBAR) in the appropriate language file. - Replace the edited line with:
$code .= '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _XOOPS_FORM_ARIA_CODEBAR . '">';
This keeps the toolbar accessible while making the label semantically accurate.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared, framework-neutral DHTML editor toolbar (XoopsDhtmlToolbar) and updates all XOOPS form renderers to delegate toolbar markup generation to it, while also modernizing legacy JavaScript helpers and expanding unit test coverage around toolbar behavior and extension integration.
Changes:
- Add
XoopsDhtmlToolbaras the single source of truth for DHTML toolbar markup (buttons, typography dropdowns, check-length) and shared assets (CSS/JS). - Refactor Legacy/Bootstrap3/4/5/Tailwind renderers to delegate toolbar construction to the shared class (keeping protected helpers for compatibility).
- Improve
formdhtmltextarea.jshelpers to avoid null dereferences and removeeval(), plus add tests and test bootstrap constants/cache cleanup.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest.php | Adds unit coverage validating renderer parity, preload event behavior, extension integration, and stylesheet injection. |
| tests/bootstrap.php | Defines missing editor-related language constants for tests and purges LocaleRegistry cache for deterministic runs. |
| htdocs/include/formdhtmltextarea.js | Hardens legacy editor JS helpers (null-safety, removes eval-based dispatch, improves no-selection behavior). |
| htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php | Delegates DHTML textarea toolbar rendering to the shared toolbar. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php | Delegates DHTML textarea toolbar rendering to the shared toolbar. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.php | Delegates DHTML textarea toolbar rendering to the shared toolbar. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.php | Delegates DHTML textarea toolbar rendering to the shared toolbar. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php | Delegates DHTML textarea toolbar rendering to the shared toolbar. |
| htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.php | New shared toolbar implementation (markup generation, extension button class rewrite, stylesheet/script injection). |
| htdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.js | Adds framework-neutral <details> dropdown behavior (mutual exclusivity, click-outside, Escape handling). |
| htdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.css | Adds framework-neutral toolbar styling and targeted CSS fixes for admin-theme resets. |
Suppressed comments (2)
htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.php:208
renderTypography()also uses$element->getName()when generating JS handlers. SincegetName()HTML-escapes quotes, the string passed intoxoopsSetElementAttribute()can diverge from the real textarea id. Use the raw name viagetName(false).
public function renderTypography(XoopsFormDhtmlTextArea $element): string
{
$textareaId = $element->getName();
$hiddenText = (string) $element->_hiddenText;
htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.php:253
renderCheckLength()passes$element->getName()into the JS call builder. As with the other toolbar rows, this can pass an HTML-escaped id into JavaScript and fail to find the textarea. Prefer the raw name (getName(false)) for JS arguments.
$maxlength = (int) $element->configs['maxlength'];
}
$onclick = $this->jsCall('XoopsCheckLength', [$element->getName(), (string) $maxlength, _XOOPS_FORM_ALT_LENGTH, _XOOPS_FORM_ALT_LENGTH_MAX]);
Comment on lines
+72
to
+78
| private const RENDERER_CLASSES = [ | ||
| 'XoopsFormRendererLegacy', | ||
| 'XoopsFormRendererBootstrap3', | ||
| 'XoopsFormRendererBootstrap4', | ||
| 'XoopsFormRendererBootstrap5', | ||
| 'XoopsFormRendererTailwind', | ||
| ]; |
Comment on lines
+357
to
+369
| protected function jsCall(string $fn, array $args): string | ||
| { | ||
| $parts = []; | ||
| foreach ($args as $arg) { | ||
| if (is_int($arg) || is_float($arg)) { | ||
| $parts[] = (string) $arg; | ||
| } else { | ||
| $parts[] = '"' . htmlspecialchars((string) $arg, ENT_QUOTES | ENT_HTML5, 'UTF-8') . '"'; | ||
| } | ||
| } | ||
|
|
||
| return $fn . '(' . implode(', ', $parts) . ');'; | ||
| } |
Comment on lines
+150
to
+159
| $textareaId = $element->getName(); | ||
| $btn = self::BTN_SM; | ||
|
|
||
| $code = '<a name="moresmiley"></a>'; | ||
| $code .= '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _XOOPS_FORM_ALT_IMG . '">'; | ||
| $code .= $this->button($btn, $this->jsCall('xoopsCodeUrl', [$textareaId, _ENTERURL, _ENTERWEBTITLE]), _XOOPS_FORM_ALT_URL, 'fa-solid fa-link'); | ||
| $code .= $this->button($btn, $this->jsCall('xoopsCodeEmail', [$textareaId, _ENTEREMAIL, _ENTERWEBTITLE]), _XOOPS_FORM_ALT_EMAIL, 'fa-solid fa-envelope'); | ||
| $code .= $this->button($btn, $this->jsCall('xoopsCodeImg', [$textareaId, _ENTERIMGURL, _ENTERIMGPOS, _IMGPOSRORL, _ERRORIMGPOS, _XOOPS_FORM_ALT_ENTERWIDTH]), _XOOPS_FORM_ALT_IMG, 'fa-solid fa-file-image'); | ||
| $code .= $this->button($btn, $this->jsCall('openWithSelfMain', [XOOPS_URL . '/imagemanager.php?target=' . $textareaId, 'imgmanager', 400, 430]), _XOOPS_FORM_ALT_IMAGE, 'fa-solid fa-file-image', '<span style="font-size:75%;"> Manager</span>'); | ||
| $code .= $this->button($btn, $this->jsCall('openWithSelfMain', [XOOPS_URL . '/misc.php?action=showpopups&type=smilies&target=' . $textareaId, 'smilies', 300, 475]), _XOOPS_FORM_ALT_SMILEY, 'fa-solid fa-face-smile'); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php (1)
485-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused Tailwind toolbar helper.
renderEditorButton(),BTN_NEUTRAL_SM, andDROPDOWN_MENU_CLSare not called insideXoopsFormRendererTailwind.php; the xoopscode and typography renderers delegate toXoopsDhtmlToolbar. Remove this deadprotectedhelper plus the two private constants, or mark it deprecated if intentional for downstream subclasses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php` around lines 485 - 495, Remove the unused renderEditorButton() helper from XoopsFormRendererTailwind along with the private constants BTN_NEUTRAL_SM and DROPDOWN_MENU_CLS. Do not alter the xoopscode or typography renderer delegation through XoopsDhtmlToolbar.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.css`:
- Around line 19-37: Move the default custom-property declarations from only
`.xo-edtb-toolbar` to `:root`, while retaining the `.xo-edtb-toolbar` block for
toolbar-specific overrides. Ensure standalone `.xo-edtb-group` rows returned by
`renderFormDhtmlTAXoopsCode()` and `renderFormDhtmlTATypography()` resolve the
button sizing, colors, and border variables without a toolbar ancestor.
- Around line 81-91: Add explicit :focus-visible styles for both .xo-edtb-btn
and .xo-edtb-menu-item, providing a clear visible focus indicator that remains
effective despite the admin theme reset. Keep the existing hover, active, and
sizing rules unchanged.
In `@htdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.js`:
- Around line 58-70: Update the Escape key handler in the document keydown
listener to restore focus only for the open dropdown that currently contains the
active element. Continue closing all open dropdowns, but avoid moving focus when
typing outside a dropdown and ensure only the focused dropdown’s summary
receives focus.
In `@htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.php`:
- Around line 212-230: Escape every language-derived aria-label in
XoopsDhtmlToolbar, including the existing call sites near the toolbar rendering
logic and the three group labels shown here, using a small shared esc helper
that applies htmlspecialchars with ENT_QUOTES | ENT_HTML5 and UTF-8. Update only
these aria-label interpolations; keep button() and dropdown() handling
unchanged.
- Around line 209-215: In the toolbar-building method, validate and normalize
both global overrides before use: accept only arrays for formtextdhtml_sizes and
formtextdhtml_fonts, otherwise use safe defaults, and ensure an empty sizes
array does not render an unusable Size dropdown. Keep font handling compatible
with array_combine by passing only a valid non-empty font list, using the
existing default-font symbol where needed.
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php`:
- Around line 309-313: Choose one consistent contract for
renderFormDhtmlTATypography() and apply it to all five renderers:
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php#L309-L313,
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.php#L314-L318,
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.php#L315-L319,
htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php#L224-L228, and
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php#L524-L528. Either
remove renderCheckLength() and provide a separate renderFormDhtmlTACheckLength()
helper, or retain the combined output and update every method docblock to
document both controls; keep the Tailwind change aligned with the dead-helper
cleanup at Line 485.
In `@tests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest.php`:
- Around line 290-299: Make
testStylesheetIsInjectedOnceAcrossMultipleEditorsOnOnePage deterministic by
explicitly controlling the $GLOBALS['xoTheme'] state, preferably clearing it in
setUp() or at the start of the test. Preserve the existing assertions for
first-render injection and second-render suppression.
- Around line 262-265: Update testCheckLengthButtonCarriesConfiguredMaxlength
and the XoopsFormDhtmlTextArea configuration path to avoid dynamically assigning
configs; declare a typed public array $configs initialized to [] on the
appropriate class, or reuse an existing configuration accessor, while preserving
the test’s configured maxlength value.
---
Outside diff comments:
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php`:
- Around line 485-495: Remove the unused renderEditorButton() helper from
XoopsFormRendererTailwind along with the private constants BTN_NEUTRAL_SM and
DROPDOWN_MENU_CLS. Do not alter the xoopscode or typography renderer delegation
through XoopsDhtmlToolbar.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb047ae1-dd84-44c6-bfb8-b2284a65f624
📒 Files selected for processing (11)
htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.phphtdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.csshtdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.jshtdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.phphtdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.phphtdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.phphtdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.phphtdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.phphtdocs/include/formdhtmltextarea.jstests/bootstrap.phptests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest.php
Comment on lines
+19
to
+37
| .xo-edtb-toolbar { | ||
| --xo-edtb-bg: #f8f9fa; | ||
| --xo-edtb-border: #ced4da; | ||
| --xo-edtb-radius: 4px; | ||
| --xo-edtb-gap: 4px; | ||
| --xo-edtb-padding: 4px; | ||
| --xo-edtb-font-size: 0.8125rem; | ||
|
|
||
| --xo-edtb-btn-size: 1.75rem; | ||
| --xo-edtb-btn-bg: #ffffff; | ||
| --xo-edtb-btn-fg: #212529; | ||
| --xo-edtb-btn-border: #ced4da; | ||
| --xo-edtb-btn-hover-bg: #e9ecef; | ||
| --xo-edtb-btn-active-bg: #dee2e6; | ||
|
|
||
| --xo-edtb-menu-bg: #ffffff; | ||
| --xo-edtb-menu-fg: #212529; | ||
| --xo-edtb-menu-border: #ced4da; | ||
| --xo-edtb-menu-shadow: 0 2px 8px rgba(0, 0, 0, 0.15); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Declare the custom properties outside the toolbar wrapper too.
Every variable is declared on .xo-edtb-toolbar. The buttons resolve them by inheritance, which works for render(). It does not work for the compatibility path this PR keeps alive: renderFormDhtmlTAXoopsCode() and renderFormDhtmlTATypography() return .xo-edtb-group rows with no .xo-edtb-toolbar ancestor. A subclass that calls those helpers directly gets unstyled buttons, because min-width, height, background and border all resolve against undefined variables.
Declare the defaults on :root (or on both .xo-edtb-toolbar and .xo-edtb-group) and keep the toolbar block for overrides.
♻️ Proposed fix
-.xo-edtb-toolbar {
+.xo-edtb-toolbar,
+.xo-edtb-group {
--xo-edtb-bg: `#f8f9fa`;
--xo-edtb-border: `#ced4da`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .xo-edtb-toolbar { | |
| --xo-edtb-bg: #f8f9fa; | |
| --xo-edtb-border: #ced4da; | |
| --xo-edtb-radius: 4px; | |
| --xo-edtb-gap: 4px; | |
| --xo-edtb-padding: 4px; | |
| --xo-edtb-font-size: 0.8125rem; | |
| --xo-edtb-btn-size: 1.75rem; | |
| --xo-edtb-btn-bg: #ffffff; | |
| --xo-edtb-btn-fg: #212529; | |
| --xo-edtb-btn-border: #ced4da; | |
| --xo-edtb-btn-hover-bg: #e9ecef; | |
| --xo-edtb-btn-active-bg: #dee2e6; | |
| --xo-edtb-menu-bg: #ffffff; | |
| --xo-edtb-menu-fg: #212529; | |
| --xo-edtb-menu-border: #ced4da; | |
| --xo-edtb-menu-shadow: 0 2px 8px rgba(0, 0, 0, 0.15); | |
| .xo-edtb-toolbar, | |
| .xo-edtb-group { | |
| --xo-edtb-bg: `#f8f9fa`; | |
| --xo-edtb-border: `#ced4da`; | |
| --xo-edtb-radius: 4px; | |
| --xo-edtb-gap: 4px; | |
| --xo-edtb-padding: 4px; | |
| --xo-edtb-font-size: 0.8125rem; | |
| --xo-edtb-btn-size: 1.75rem; | |
| --xo-edtb-btn-bg: `#ffffff`; | |
| --xo-edtb-btn-fg: `#212529`; | |
| --xo-edtb-btn-border: `#ced4da`; | |
| --xo-edtb-btn-hover-bg: `#e9ecef`; | |
| --xo-edtb-btn-active-bg: `#dee2e6`; | |
| --xo-edtb-menu-bg: `#ffffff`; | |
| --xo-edtb-menu-fg: `#212529`; | |
| --xo-edtb-menu-border: `#ced4da`; | |
| --xo-edtb-menu-shadow: 0 2px 8px rgba(0, 0, 0, 0.15); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.css` around lines 19 -
37, Move the default custom-property declarations from only `.xo-edtb-toolbar`
to `:root`, while retaining the `.xo-edtb-toolbar` block for toolbar-specific
overrides. Ensure standalone `.xo-edtb-group` rows returned by
`renderFormDhtmlTAXoopsCode()` and `renderFormDhtmlTATypography()` resolve the
button sizing, colors, and border variables without a toolbar ancestor.
Comment on lines
+81
to
+91
| .xo-edtb-btn:hover { | ||
| background: var(--xo-edtb-btn-hover-bg); | ||
| } | ||
|
|
||
| .xo-edtb-btn:active { | ||
| background: var(--xo-edtb-btn-active-bg); | ||
| } | ||
|
|
||
| .xo-edtb-btn-sm { | ||
| padding: 0 0.3rem; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a visible focus indicator for the buttons and menu items.
The file documents that the admin themes ship a universal reset. Those resets commonly drop outline, and this stylesheet defines :hover and :active but no focus state. Keyboard users then cannot see which toolbar control has focus.
Add an explicit :focus-visible rule for .xo-edtb-btn and .xo-edtb-menu-item.
♿ Proposed fix
.xo-edtb-btn:active {
background: var(--xo-edtb-btn-active-bg);
}
+
+.xo-edtb-btn:focus-visible,
+.xo-edtb-menu-item:focus-visible {
+ outline: 2px solid currentColor;
+ outline-offset: 1px;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .xo-edtb-btn:hover { | |
| background: var(--xo-edtb-btn-hover-bg); | |
| } | |
| .xo-edtb-btn:active { | |
| background: var(--xo-edtb-btn-active-bg); | |
| } | |
| .xo-edtb-btn-sm { | |
| padding: 0 0.3rem; | |
| } | |
| .xo-edtb-btn:hover { | |
| background: var(--xo-edtb-btn-hover-bg); | |
| } | |
| .xo-edtb-btn:active { | |
| background: var(--xo-edtb-btn-active-bg); | |
| } | |
| .xo-edtb-btn:focus-visible, | |
| .xo-edtb-menu-item:focus-visible { | |
| outline: 2px solid currentColor; | |
| outline-offset: 1px; | |
| } | |
| .xo-edtb-btn-sm { | |
| padding: 0 0.3rem; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.css` around lines 81 -
91, Add explicit :focus-visible styles for both .xo-edtb-btn and
.xo-edtb-menu-item, providing a clear visible focus indicator that remains
effective despite the admin theme reset. Keep the existing hover, active, and
sizing rules unchanged.
Comment on lines
+58
to
+70
| document.addEventListener('keydown', function (event) { | ||
| if (event.key !== 'Escape' && event.keyCode !== 27) { | ||
| return; | ||
| } | ||
| var open = document.querySelectorAll(DROPDOWN + '[open]'); | ||
| for (var i = 0; i < open.length; i++) { | ||
| var summary = open[i].querySelector('summary'); | ||
| open[i].removeAttribute('open'); | ||
| if (summary && typeof summary.focus === 'function') { | ||
| summary.focus(); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape moves focus even when the user is typing elsewhere.
The handler closes every open dropdown and calls summary.focus() for each one. Two consequences:
- If a dropdown stays open while the user types in the textarea, Escape pulls focus out of the textarea and onto a toolbar toggle.
- If two dropdowns are open, focus lands on the last one in document order, which is arbitrary.
Restrict the focus restore to the dropdown that currently contains focus.
🐛 Proposed fix
var open = document.querySelectorAll(DROPDOWN + '[open]');
+ var active = document.activeElement;
for (var i = 0; i < open.length; i++) {
var summary = open[i].querySelector('summary');
+ var hadFocus = !!(active && open[i].contains(active));
open[i].removeAttribute('open');
- if (summary && typeof summary.focus === 'function') {
+ if (hadFocus && summary && typeof summary.focus === 'function') {
summary.focus();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| document.addEventListener('keydown', function (event) { | |
| if (event.key !== 'Escape' && event.keyCode !== 27) { | |
| return; | |
| } | |
| var open = document.querySelectorAll(DROPDOWN + '[open]'); | |
| for (var i = 0; i < open.length; i++) { | |
| var summary = open[i].querySelector('summary'); | |
| open[i].removeAttribute('open'); | |
| if (summary && typeof summary.focus === 'function') { | |
| summary.focus(); | |
| } | |
| } | |
| }); | |
| document.addEventListener('keydown', function (event) { | |
| if (event.key !== 'Escape' && event.keyCode !== 27) { | |
| return; | |
| } | |
| var open = document.querySelectorAll(DROPDOWN + '[open]'); | |
| var active = document.activeElement; | |
| for (var i = 0; i < open.length; i++) { | |
| var summary = open[i].querySelector('summary'); | |
| var hadFocus = !!(active && open[i].contains(active)); | |
| open[i].removeAttribute('open'); | |
| if (hadFocus && summary && typeof summary.focus === 'function') { | |
| summary.focus(); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/xoopseditor/dhtmltextarea/assets/toolbar.js` around lines 58 -
70, Update the Escape key handler in the document keydown listener to restore
focus only for the open dropdown that currently contains the active element.
Continue closing all open dropdowns, but avoid moving focus when typing outside
a dropdown and ensure only the focused dropdown’s summary receives focus.
Comment on lines
+209
to
+215
| $sizes = $GLOBALS['formtextdhtml_sizes'] ?? []; | ||
| $fonts = !empty($GLOBALS['formtextdhtml_fonts']) ? $GLOBALS['formtextdhtml_fonts'] : self::DEFAULT_FONTS; | ||
|
|
||
| $ret = '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _SIZE . '/' . _FONT . '/' . _COLOR . '">'; | ||
| $ret .= $this->dropdown($textareaId, $hiddenText, 'size', _SIZE, 'fa-solid fa-text-height', $sizes); | ||
| $ret .= $this->dropdown($textareaId, $hiddenText, 'font', _FONT, 'fa-solid fa-font', array_combine($fonts, $fonts)); | ||
| $ret .= $this->dropdown($textareaId, $hiddenText, 'color', _COLOR, 'fa-solid fa-palette', array_flip(self::COLOR_PALETTE), true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the global font and size overrides before you use them.
$GLOBALS['formtextdhtml_fonts'] is a documented override point. !empty() also passes for a non-empty string or object. array_combine($fonts, $fonts) on Line 214 then throws a TypeError and the whole form render dies. $GLOBALS['formtextdhtml_sizes'] has the same exposure: a non-array value flows straight into dropdown() and foreach emits a warning.
Also note that an empty $sizes renders a dropdown toggle with an empty <ul>, so the Size control appears but does nothing.
🛡️ Proposed fix to normalize both overrides
- $sizes = $GLOBALS['formtextdhtml_sizes'] ?? [];
- $fonts = !empty($GLOBALS['formtextdhtml_fonts']) ? $GLOBALS['formtextdhtml_fonts'] : self::DEFAULT_FONTS;
+ $sizes = isset($GLOBALS['formtextdhtml_sizes']) && is_array($GLOBALS['formtextdhtml_sizes'])
+ ? $GLOBALS['formtextdhtml_sizes']
+ : [];
+ $fonts = !empty($GLOBALS['formtextdhtml_fonts']) && is_array($GLOBALS['formtextdhtml_fonts'])
+ ? array_values(array_filter(array_map('strval', $GLOBALS['formtextdhtml_fonts']), 'strlen'))
+ : self::DEFAULT_FONTS;
+ if ([] === $fonts) {
+ $fonts = self::DEFAULT_FONTS;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $sizes = $GLOBALS['formtextdhtml_sizes'] ?? []; | |
| $fonts = !empty($GLOBALS['formtextdhtml_fonts']) ? $GLOBALS['formtextdhtml_fonts'] : self::DEFAULT_FONTS; | |
| $ret = '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _SIZE . '/' . _FONT . '/' . _COLOR . '">'; | |
| $ret .= $this->dropdown($textareaId, $hiddenText, 'size', _SIZE, 'fa-solid fa-text-height', $sizes); | |
| $ret .= $this->dropdown($textareaId, $hiddenText, 'font', _FONT, 'fa-solid fa-font', array_combine($fonts, $fonts)); | |
| $ret .= $this->dropdown($textareaId, $hiddenText, 'color', _COLOR, 'fa-solid fa-palette', array_flip(self::COLOR_PALETTE), true); | |
| $sizes = isset($GLOBALS['formtextdhtml_sizes']) && is_array($GLOBALS['formtextdhtml_sizes']) | |
| ? $GLOBALS['formtextdhtml_sizes'] | |
| : []; | |
| $fonts = !empty($GLOBALS['formtextdhtml_fonts']) && is_array($GLOBALS['formtextdhtml_fonts']) | |
| ? array_values(array_filter(array_map('strval', $GLOBALS['formtextdhtml_fonts']), 'strlen')) | |
| : self::DEFAULT_FONTS; | |
| if ([] === $fonts) { | |
| $fonts = self::DEFAULT_FONTS; | |
| } | |
| $ret = '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _SIZE . '/' . _FONT . '/' . _COLOR . '">'; | |
| $ret .= $this->dropdown($textareaId, $hiddenText, 'size', _SIZE, 'fa-solid fa-text-height', $sizes); | |
| $ret .= $this->dropdown($textareaId, $hiddenText, 'font', _FONT, 'fa-solid fa-font', array_combine($fonts, $fonts)); | |
| $ret .= $this->dropdown($textareaId, $hiddenText, 'color', _COLOR, 'fa-solid fa-palette', array_flip(self::COLOR_PALETTE), true); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.php` around lines
209 - 215, In the toolbar-building method, validate and normalize both global
overrides before use: accept only arrays for formtextdhtml_sizes and
formtextdhtml_fonts, otherwise use safe defaults, and ensure an empty sizes
array does not render an unusable Size dropdown. Keep font handling compatible
with array_combine by passing only a valid non-empty font list, using the
existing default-font symbol where needed.
Comment on lines
+212
to
+230
| $ret = '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _SIZE . '/' . _FONT . '/' . _COLOR . '">'; | ||
| $ret .= $this->dropdown($textareaId, $hiddenText, 'size', _SIZE, 'fa-solid fa-text-height', $sizes); | ||
| $ret .= $this->dropdown($textareaId, $hiddenText, 'font', _FONT, 'fa-solid fa-font', array_combine($fonts, $fonts)); | ||
| $ret .= $this->dropdown($textareaId, $hiddenText, 'color', _COLOR, 'fa-solid fa-palette', array_flip(self::COLOR_PALETTE), true); | ||
| $ret .= '</div>'; | ||
|
|
||
| $btn = self::BTN_SM; | ||
| $ret .= '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _XOOPS_FORM_ALT_BOLD . '">'; | ||
| $ret .= $this->button($btn, $this->jsCall('xoopsMakeBold', [$hiddenText, $textareaId]), _XOOPS_FORM_ALT_BOLD, 'fa-solid fa-bold'); | ||
| $ret .= $this->button($btn, $this->jsCall('xoopsMakeItalic', [$hiddenText, $textareaId]), _XOOPS_FORM_ALT_ITALIC, 'fa-solid fa-italic'); | ||
| $ret .= $this->button($btn, $this->jsCall('xoopsMakeUnderline', [$hiddenText, $textareaId]), _XOOPS_FORM_ALT_UNDERLINE, 'fa-solid fa-underline'); | ||
| $ret .= $this->button($btn, $this->jsCall('xoopsMakeLineThrough', [$hiddenText, $textareaId]), _XOOPS_FORM_ALT_LINETHROUGH, 'fa-solid fa-strikethrough'); | ||
| $ret .= '</div>'; | ||
|
|
||
| $ret .= '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _XOOPS_FORM_ALT_LEFT . '/' . _XOOPS_FORM_ALT_CENTER . '/' . _XOOPS_FORM_ALT_RIGHT . '">'; | ||
| $ret .= $this->button($btn, $this->jsCall('xoopsMakeLeft', [$hiddenText, $textareaId]), _XOOPS_FORM_ALT_LEFT, 'fa-solid fa-align-left'); | ||
| $ret .= $this->button($btn, $this->jsCall('xoopsMakeCenter', [$hiddenText, $textareaId]), _XOOPS_FORM_ALT_CENTER, 'fa-solid fa-align-center'); | ||
| $ret .= $this->button($btn, $this->jsCall('xoopsMakeRight', [$hiddenText, $textareaId]), _XOOPS_FORM_ALT_RIGHT, 'fa-solid fa-align-right'); | ||
| $ret .= '</div>'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Escape the language constants that land inside aria-label.
The group labels come from language packs. They are interpolated raw into double-quoted attributes here and on Line 154 and Line 254. One translated string containing " breaks the attribute and injects markup. button() and dropdown() already escape their titles, so this is the only unescaped path left in the class.
Route these through the same htmlspecialchars($value, ENT_QUOTES | ENT_HTML5, 'UTF-8') call. A small private helper keeps the five call sites tidy.
As per coding guidelines: "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8') or use Smarty auto-escaping".
🔒️ Proposed fix
- $ret = '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="' . _SIZE . '/' . _FONT . '/' . _COLOR . '">';
+ $ret = '<div class="' . self::GROUP_CLASS . '" role="group" aria-label="'
+ . $this->esc(_SIZE . '/' . _FONT . '/' . _COLOR) . '">';Add the helper and reuse it on Lines 154, 219, 226 and 254:
/**
* Escape a value for an HTML attribute.
*
* `@param` string $value raw value
*
* `@return` string escaped value
*/
protected function esc(string $value): string
{
return htmlspecialchars($value, ENT_QUOTES | ENT_HTML5, 'UTF-8');
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/xoopseditor/dhtmltextarea/XoopsDhtmlToolbar.php` around lines
212 - 230, Escape every language-derived aria-label in XoopsDhtmlToolbar,
including the existing call sites near the toolbar rendering logic and the three
group labels shown here, using a small shared esc helper that applies
htmlspecialchars with ENT_QUOTES | ENT_HTML5 and UTF-8. Update only these
aria-label interpolations; keep button() and dropdown() handling unchanged.
Source: Coding guidelines
Comment on lines
309
to
+313
| protected function renderFormDhtmlTATypography(XoopsFormDhtmlTextArea $element) | ||
| { | ||
| $textarea_id = $element->getName(); | ||
| $hiddentext = $element->_hiddenText; | ||
|
|
||
| $fontarray = !empty($GLOBALS['formtextdhtml_fonts']) ? $GLOBALS['formtextdhtml_fonts'] : [ | ||
| 'Arial', | ||
| 'Courier', | ||
| 'Georgia', | ||
| 'Helvetica', | ||
| 'Impact', | ||
| 'Verdana', | ||
| 'Haettenschweiler', | ||
| ]; | ||
|
|
||
| $colorArray = [ | ||
| 'Black' => '000000', | ||
| 'Blue' => '38AAFF', | ||
| 'Brown' => '987857', | ||
| 'Green' => '79D271', | ||
| 'Grey' => '888888', | ||
| 'Orange' => 'FFA700', | ||
| 'Paper' => 'E0E0E0', | ||
| 'Purple' => '363E98', | ||
| 'Red' => 'FF211E', | ||
| 'White' => 'FEFEFE', | ||
| 'Yellow' => 'FFD628', | ||
| ]; | ||
|
|
||
| $fontStr = '<div class="row"><div class="col-md-12"><div class="btn-group" role="toolbar">'; | ||
| $fontStr .= '<div class="btn-group">' | ||
| . '<button type="button" class="btn btn-default btn-sm dropdown-toggle" title="' . _SIZE . '"' | ||
| . ' data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">' | ||
| . '<span class = "fa-solid fa-text-height"></span><span class="caret"></span></button>' | ||
| . '<ul class="dropdown-menu">'; | ||
| //. _SIZE . ' <span class="caret"></span></button><ul class="dropdown-menu">'; | ||
| foreach ($GLOBALS['formtextdhtml_sizes'] as $value => $name) { | ||
| $fontStr .= '<li><a href="javascript:xoopsSetElementAttribute(\'size\', \'' . $value . '\', \'' | ||
| . $textarea_id . '\', \'' . $hiddentext . '\');">' . $name . '</a></li>'; | ||
| } | ||
| $fontStr .= '</ul></div>'; | ||
|
|
||
| $fontStr .= '<div class="btn-group">' | ||
| . '<button type="button" class="btn btn-default btn-sm dropdown-toggle" title="' . _FONT . '"' | ||
| . ' data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">' | ||
| . '<span class = "fa-solid fa-font"></span><span class="caret"></span></button>' | ||
| . '<ul class="dropdown-menu">'; | ||
| //. _FONT . ' <span class="caret"></span></button><ul class="dropdown-menu">'; | ||
| foreach ($fontarray as $font) { | ||
| $fontStr .= '<li><a href="javascript:xoopsSetElementAttribute(\'font\', \'' . $font . '\', \'' | ||
| . $textarea_id . '\', \'' . $hiddentext . '\');">' . $font . '</a></li>'; | ||
| } | ||
| $fontStr .= '</ul></div>'; | ||
|
|
||
| $fontStr .= '<div class="btn-group">' | ||
| . '<button type="button" class="btn btn-default btn-sm dropdown-toggle" title="' . _COLOR . '"' | ||
| . ' data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">' | ||
| . '<span class = "fa-solid fa-palette"></span><span class="caret"></span></button>' | ||
| . '<ul class="dropdown-menu">'; | ||
| //. _COLOR . ' <span class="caret"></span></button><ul class="dropdown-menu">'; | ||
| foreach ($colorArray as $color => $hex) { | ||
| $fontStr .= '<li><a href="javascript:xoopsSetElementAttribute(\'color\', \'' . $hex . '\', \'' | ||
| . $textarea_id . '\', \'' . $hiddentext . '\');">' | ||
| . '<span style="color:#' . $hex . ';">' . $color . '</span></a></li>'; | ||
| } | ||
| $fontStr .= '</ul></div>'; | ||
| $fontStr .= '</div>'; | ||
|
|
||
| //$styleStr = "<div class='row'><div class='col-md-12'>"; | ||
| $styleStr = "<div class='btn-group' role='group'>"; | ||
| $styleStr .= "<button type='button' class='btn btn-default btn-sm' onclick='xoopsMakeBold(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_BOLD . "' aria-label='Left Align'><span class='fa-solid fa-bold' aria-hidden='true'></span></button>"; | ||
| $styleStr .= "<button type='button' class='btn btn-default btn-sm' onclick='xoopsMakeItalic(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_ITALIC . "' aria-label='Left Align'><span class='fa-solid fa-italic' aria-hidden='true'></span></button>"; | ||
| $styleStr .= "<button type='button' class='btn btn-default btn-sm' onclick='xoopsMakeUnderline(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_UNDERLINE . "' aria-label='Left Align'>" . '<span class="fa-solid fa-underline"></span></button>'; | ||
| $styleStr .= "<button type='button' class='btn btn-default btn-sm' onclick='xoopsMakeLineThrough(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_LINETHROUGH . "' aria-label='Left Align'>" . '<span class="fa-solid fa-strikethrough"></span></button>'; | ||
| $styleStr .= "</div>"; | ||
|
|
||
| $alignStr = "<div class='btn-group' role='group'>"; | ||
| $alignStr .= "<button type='button' class='btn btn-default btn-sm' onclick='xoopsMakeLeft(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_LEFT . "' aria-label='Left Align'><span class='fa-solid fa-align-left' aria-hidden='true'></span></button>"; | ||
| $alignStr .= "<button type='button' class='btn btn-default btn-sm' onclick='xoopsMakeCenter(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_CENTER . "' aria-label='Left Align'><span class='fa-solid fa-align-center' aria-hidden='true'></span></button>"; | ||
| $alignStr .= "<button type='button' class='btn btn-default btn-sm' onclick='xoopsMakeRight(\"{$hiddentext}\", \"{$textarea_id}\");' title='" . _XOOPS_FORM_ALT_RIGHT . "' aria-label='Left Align'><span class='fa-solid fa-align-right' aria-hidden='true'></span></button>"; | ||
| $alignStr .= "</div>"; | ||
|
|
||
| $fontStr .= " {$styleStr} {$alignStr} \n"; | ||
|
|
||
| $maxlength = $element->configs['maxlength'] ?? 0; | ||
| $fontStr .= "<button type='button' class='btn btn-default btn-sm' onclick=\"XoopsCheckLength('" | ||
| . $element->getName() . "', '" . $maxlength . "', '" | ||
| . _XOOPS_FORM_ALT_LENGTH . "', '" . _XOOPS_FORM_ALT_LENGTH_MAX . "');\" title='" | ||
| . _XOOPS_FORM_ALT_CHECKLENGTH . "'><span class='fa-solid fa-square-check' aria-hidden='true'></span></button>"; | ||
| $fontStr .= "</div></div>"; | ||
|
|
||
| return $fontStr; | ||
| $toolbar = new \XoopsDhtmlToolbar(); | ||
|
|
||
| return $toolbar->renderTypography($element) . $toolbar->renderCheckLength($element); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
renderFormDhtmlTATypography() silently absorbed the check-length button in all five renderers. The PR keeps these protected helpers precisely so third-party subclasses can still call them, but the shared root cause is a contract change: each helper now returns renderTypography() plus renderCheckLength() while its docblock still promises "rendered typography controls". A subclass that composed typography with its own length control now renders two length buttons. Pick one resolution and apply it consistently: either drop the renderCheckLength() call and expose a separate renderFormDhtmlTACheckLength() helper, or keep the combined output and state it in every docblock.
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php#L309-L313: decide the contract here first, then update the docblock or drop therenderCheckLength()call.htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.php#L314-L318: apply the identical resolution.htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.php#L315-L319: apply the identical resolution.htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php#L224-L228: apply the identical resolution.htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php#L524-L528: apply the identical resolution, and keep it aligned with the dead-helper cleanup flagged at Line 485.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 311-311: Missing class import via use statement (line '311', column '24'). (undefined)
(MissingImport)
📍 Affects 5 files
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php#L309-L313(this comment)htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.php#L314-L318htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.php#L315-L319htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php#L224-L228htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php#L524-L528
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php` around lines
309 - 313, Choose one consistent contract for renderFormDhtmlTATypography() and
apply it to all five renderers:
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php#L309-L313,
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.php#L314-L318,
htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.php#L315-L319,
htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php#L224-L228, and
htdocs/class/xoopsform/renderer/XoopsFormRendererTailwind.php#L524-L528. Either
remove renderCheckLength() and provide a separate renderFormDhtmlTACheckLength()
helper, or retain the combined output and update every method docblock to
document both controls; keep the Tailwind change aligned with the dead-helper
cleanup at Line 485.
Comment on lines
+262
to
+265
| public function testCheckLengthButtonCarriesConfiguredMaxlength(): void | ||
| { | ||
| $element = $this->makeElement(); | ||
| $element->configs = ['maxlength' => 12345]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether XoopsFormDhtmlTextArea declares configs or allows dynamic properties, and how PHPUnit treats deprecations.
set -euo pipefail
fd -t f 'formdhtmltextarea.php' htdocs/class | while IFS= read -r f; do
echo "===== $f ====="
rg -n -C3 'AllowDynamicProperties|\$configs|class XoopsFormDhtmlTextArea' "$f"
done
echo "===== phpunit config: deprecation handling ====="
fd -t f -g 'phpunit*.xml*' | while IFS= read -r c; do
echo "--- $c ---"
rg -n 'convertDeprecations|failOnDeprecation|failOnWarning|failOnNotice' "$c" || echo "no deprecation flags set"
doneRepository: XOOPS/XoopsCore27
Length of output: 700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== locate XoopsFormDhtmlTextArea and parent ====="
fd -t f 'formdhtmltextarea.php|formtextarea.php|xoopsformphp' htdocs/class/tests htdocs/class/xoopsform | sort
echo "===== formdhtmltextarea outline ====="
ast-grep outline htdocs/class/xoopsform/formdhtmltextarea.php --view expanded || true
echo "===== relevant formdhtmltextarea sources ====="
sed -n '1,220p' htdocs/class/xoopsform/formdhtmltextarea.php
echo "===== XoopsFormTextArea property declarations ====="
fd -t f 'formtextarea.php' htdocs/class | while IFS= read -r f; do
echo "--- $f ---"
rg -n -C2 'AllowDynamicProperties|\$configs|class XoopsFormTextArea|function __construct|property_exists|maxlength' "$f"
done
echo "===== target test context ====="
sed -n '240,275p' tests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest.phpRepository: XOOPS/XoopsCore27
Length of output: 9378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== formtextarea.php ====="
sed -n '1,160p' htdocs/class/xoopsform/formtextarea.php
echo "===== locate XoopsFormElement ====="
fd -t f 'formelement.php|xoopsformelement.php' htdocs/class/xoopsform|sort
echo "===== XoopsFormElement declarations/usages ====="
fd -t f 'formelement.php' htdocs/class/xoopsform | while IFS= read -r f; do
echo "--- $f ---"
rg -n -C3 'AllowDynamicProperties|\$configs|class XoopsFormElement|function __construct|maxlength|getName|setConfig|getConfig' "$f"
done
echo "===== XoopsFormDhtmlToolbar usages of configs ====="
fd -t f 'dhtmltoolbar.php' htdocs/class/xoopsform | while IFS= read -r f; do
echo "--- $f ---"
rg -n -C4 'property_exists|configs|maxlength|renderCheckLength' "$f"
done
echo "===== inspect target property occurrences ====="
rg -n '\$configs|\bconfigs\b' htdocs/class/xoopsform/formdhtmltextarea.php htdocs/class/xoopsform/formtextarea.php tests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest.phpRepository: XOOPS/XoopsCore27
Length of output: 4981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
composer config platform.php --unset >/dev/null 2>&1 || true
echo "===== PHPUnit version ====="
if [ -x vendor/bin/phpunit ]; then
vendor/bin/phpunit --version 2>/dev/null || true
php --version 2>/dev/null | head -1
else
php -v 2>/dev/null | head -1 || true
ls -l vendor/bin/phpunit 2>/dev/null || true
fi
echo "===== Composer config ====="
cat composer.json | python3 - <<'PY'
import json, sys
data=json.load(sys.stdin)
print("platform.php=", data.get("config", {}).get("platform", {}).get("php"))
print("phpunit=", data.get("require-dev", {}).get("phpunit/phpunit"))
PY
echo "===== isolated dynamic-property probe ====="
php -r '
class X { }
$e = new X();
$e->configs = ["maxlength" => 12345];
'
python3 <<'PY'
import subprocess
out = subprocess.run(
["php", "-r", 'error_reporting(E_ALL); class X { } $e = new X(); $e->configs = ["maxlength" => 12345];'],
stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True
)
print("rc=", out.returncode)
print("stderr=", out.stderr)
print("stdout=", out.stdout)
PYRepository: XOOPS/XoopsCore27
Length of output: 926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 <<'PY'
import subprocess
code='error_reporting(E_ALL); class X { } $e = new X(); $e->configs = ["maxlength" => 12345];'
out = subprocess.run(["php", "-r", code], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
print("php_version=", subprocess.run(["php", "-r", "echo PHP_VERSION;"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True).stdout.strip())
print("return_code=", out.returncode)
print("stderr=", repr(out.stderr))
print("stdout=", repr(out.stdout))
PY
echo "===== repo files at root ====="
git ls-files | rg '(^|/)composer\.json$|(^|/)phpunit.*\.xml|htdocs/class/xoopsform/(formdhtmltextarea|formtextarea|formelement)\.php|tests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest\.php' | sortRepository: XOOPS/XoopsCore27
Length of output: 2193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== tests/phpunit.xml.dist first 120 lines ====="
sed -n '1,120p' tests/phpunit.xml.dist
echo "===== XoopsFormElement top/property list with line numbers ====="
rg -n -A8 -B2 'class XoopsFormElement|AllowDynamicProperties|`@var`|public|private|protected|^ {4}\$' htdocs/class/xoopsform/formelement.phpRepository: XOOPS/XoopsCore27
Length of output: 11380
🌐 Web query:
PHP 8.2 creation of dynamic property deprecated documentation
💡 Result:
As of PHP 8.2, the creation of dynamic properties (setting a value to an undeclared property) is deprecated [1][2][3]. When a dynamic property is created in code not opting into this behavior, PHP will emit a deprecation notice during execution [2][3]. This change is intended to help developers catch typos and errors in property access [1][4]. There are three primary ways to avoid this deprecation notice: 1. Declare the properties: The recommended approach is to explicitly declare all properties within the class definition [3][4]. 2. Use the #[\AllowDynamicProperties] attribute: You can mark a class with the #[AllowDynamicProperties] attribute to explicitly opt-in to dynamic property usage [1][2]. This effect is inherited, meaning child classes will also allow dynamic properties [2][5]. 3. Use magic methods: Classes that implement the get and set magic methods are exempt from this deprecation [1][3][4]. Additionally, stdClass and its subclasses are exempt from this deprecation because stdClass is internally marked with the #[AllowDynamicProperties] attribute [1][3]. It is important to note that while this behavior is merely deprecated in PHP 8.2, it is scheduled to be removed in PHP 9.0, at which point creating dynamic properties will result in a fatal Error exception [3][4].
Citations:
- 1: https://www.php.net/releases/8.2/
- 2: https://www.php.net/manual/en/class.allowdynamicproperties.php
- 3: https://php.watch/versions/8.2/dynamic-properties-deprecated
- 4: https://wiki.php.net/rfc/deprecate_dynamic_properties?do=
- 5: https://www.php.net/class.allowdynamicproperties.php
Declare config or avoid dynamic configs assignment.
$element->configs creates a dynamic property on XoopsFormDhtmlTextArea because neither it nor its parent declares configs and the classes do not use #[\AllowDynamicProperties]. PHP 8.2+ emits E_DEPRECATED for this, so this test can be converted into a PHPUnit failure on PHP 8.4+ or older with deprecation-as-error settings. Add a declaration such as public array $configs = []; or use an existing config accessor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest.php` around lines 262
- 265, Update testCheckLengthButtonCarriesConfiguredMaxlength and the
XoopsFormDhtmlTextArea configuration path to avoid dynamically assigning
configs; declare a typed public array $configs initialized to [] on the
appropriate class, or reuse an existing configuration accessor, while preserving
the test’s configured maxlength value.
Source: Path instructions
Comment on lines
+290
to
+299
| public function testStylesheetIsInjectedOnceAcrossMultipleEditorsOnOnePage(): void | ||
| { | ||
| $toolbar = new XoopsDhtmlToolbar(); | ||
|
|
||
| $first = $toolbar->render($this->makeElement('editorOne')); | ||
| $second = $toolbar->render($this->makeElement('editorTwo')); | ||
|
|
||
| $this->assertStringContainsString('toolbar.css', $first); | ||
| $this->assertStringNotContainsString('toolbar.css', $second); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pin $GLOBALS['xoTheme'] for the stylesheet test.
injectStylesheet() branches on $GLOBALS['xoTheme']. When a theme object is present it calls addStylesheet() and returns an empty string. If any earlier test in the process leaves an xoTheme global behind, $first contains no toolbar.css and Line 297 fails. The result then depends on test execution order.
Unset the global in setUp(), or set it explicitly in this test.
As per path instructions: "Review test code for proper assertions, test isolation, and edge case coverage."
💚 Proposed fix
public function testStylesheetIsInjectedOnceAcrossMultipleEditorsOnOnePage(): void
{
+ unset($GLOBALS['xoTheme']);
$toolbar = new XoopsDhtmlToolbar();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function testStylesheetIsInjectedOnceAcrossMultipleEditorsOnOnePage(): void | |
| { | |
| $toolbar = new XoopsDhtmlToolbar(); | |
| $first = $toolbar->render($this->makeElement('editorOne')); | |
| $second = $toolbar->render($this->makeElement('editorTwo')); | |
| $this->assertStringContainsString('toolbar.css', $first); | |
| $this->assertStringNotContainsString('toolbar.css', $second); | |
| } | |
| public function testStylesheetIsInjectedOnceAcrossMultipleEditorsOnOnePage(): void | |
| { | |
| unset($GLOBALS['xoTheme']); | |
| $toolbar = new XoopsDhtmlToolbar(); | |
| $first = $toolbar->render($this->makeElement('editorOne')); | |
| $second = $toolbar->render($this->makeElement('editorTwo')); | |
| $this->assertStringContainsString('toolbar.css', $first); | |
| $this->assertStringNotContainsString('toolbar.css', $second); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/htdocs/class/xoopsform/XoopsDhtmlToolbarTest.php` around lines 290
- 299, Make testStylesheetIsInjectedOnceAcrossMultipleEditorsOnOnePage
deterministic by explicitly controlling the $GLOBALS['xoTheme'] state,
preferably clearing it in setUp() or at the start of the test. Preserve the
existing assertions for first-render injection and second-render suppression.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a shared, framework-neutral DHTML editor toolbar for XoopsFormDhtmlTextArea and update all form renderers to use it, while hardening related JavaScript helpers and test infrastructure.
New Features:
Enhancements:
Tests:
Summary by Gitar
XoopsDhtmlToolbarclass to unify editor markup across all renderers<details>dropdownsformdhtmltextarea.jsXoopsDhtmlToolbarTest.phpto verify renderer output parity and event contractsThis will update automatically on new commits.
Summary by CodeRabbit
New Features
Bug Fixes