fix(ZMSKVR-1229): changed div to p, added aria-atomic#1893
fix(ZMSKVR-1229): changed div to p, added aria-atomic#1893ChristianVierthaler wants to merge 11 commits intonextfrom
Conversation
📝 WalkthroughWalkthroughReworked AppointmentPreview markup (removed MucCallout, added aria attributes and new inner layout with icon/blocks) and changed AppointmentSummary container element. sanitizeHtml gained block-level root detection and now wraps inline-only sanitized content in a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@zmscitizenview/src/components/Appointment/AppointmentSelection/AppointmentPreview.vue`:
- Around line 52-62: The block-level HTML produced by
sanitizeHtml(selectedProvider.scope.infoForAppointment) is currently placed
inside a <span> in AppointmentPreview.vue which violates HTML nesting rules;
replace the outer <span> with a block-level container (e.g., <p> or <div>) that
wraps the <strong>{{ t("hint") }}</strong>, <br />, and the element using
v-html="sanitizeHtml(selectedProvider.scope.infoForAppointment)" so the v-html
content can contain <p>, <div>, <ul>, etc.; mirror the approach used in
AppointmentSummary.vue to ensure correct semantics and accessibility.
🧹 Nitpick comments (1)
zmscitizenview/src/components/Appointment/AppointmentSelection/AppointmentPreview.vue (1)
17-51: Using<span>+<br />for layout instead of semantic block elements.The refactoring replaces block-level wrappers (
<div>,<p>) with<span>elements and<br />tags for vertical spacing. While this may fix a specific HTML validation issue (e.g.,<p>nesting violations inside the callout), relying on<br />for structural spacing is generally discouraged — it conflates content and presentation, and makes future styling adjustments harder.If the root cause was invalid nesting (e.g.,
<div>inside<p>fromMucCallout), consider using<div>wrappers with CSS margins instead, which preserves semantics and separation of concerns.
zmscitizenview/src/components/Appointment/AppointmentSelection/AppointmentPreview.vue
Outdated
Show resolved
Hide resolved
… rendering - Replace <p> and <span> wrappers with <div> in AppointmentSummary and AppointmentPreview - Enhance sanitizeHtml to auto-wrap plain text/inline content in <p> tags using DOM-based detection - Use DOMParser to accurately detect block-level elements instead of fragile regex - Add comprehensive tests for block-level element detection and auto-wrapping behavior - Ensures valid HTML nesting and handles database inconsistency where some entries have <p> tags and others don't
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmscitizenview/tests/unit/utils/sanitizeHtml.spec.ts (1)
3-4:⚠️ Potential issue | 🟡 MinorRemove unnecessary
@ts-expect-error—sanitizeHtml.tsis a plain TypeScript utility, not a Vue SFC.The
@/path alias is properly configured intsconfig.json, so this import should resolve without suppression. The comment message ("Vue SFC import") is also inaccurate for a plain.tsmodule. If a real type error exists here, it should be documented specifically rather than suppressed with a misleading comment.
🤖 Fix all issues with AI agents
In `@zmscitizenview/src/utils/sanitizeHtml.ts`:
- Around line 29-42: hasBlockLevelRoot currently only inspects firstElementChild
and thus ignores leading text nodes, causing mixed content like
"text<div>...</div>" to be treated as block-rooted; update hasBlockLevelRoot to
examine container.childNodes instead of firstElementChild and return false if
any leading child node before the first element is a text node containing
non-whitespace characters (use Node.TEXT_NODE checks and trim to detect
meaningful text), otherwise proceed to test the first element node as
block-level.
🧹 Nitpick comments (1)
zmscitizenview/src/utils/sanitizeHtml.ts (1)
46-86: HoistblockLevelTagsto module scope — it's a static constant recreated on every call.This
Setis invariant and doesn't need to be rebuilt each timehasBlockLevelRootis invoked. Move it to module level for a minor perf win and cleaner intent.♻️ Proposed refactor
+// Block-level HTML5 elements (static, used by hasBlockLevelRoot) +const BLOCK_LEVEL_TAGS = new Set([ + "address", "article", "aside", "blockquote", "details", "dialog", + "dd", "div", "dl", "dt", "fieldset", "figcaption", "figure", + "footer", "form", "h1", "h2", "h3", "h4", "h5", "h6", + "header", "hgroup", "hr", "li", "main", "nav", "ol", "p", + "pre", "section", "table", "tbody", "td", "tfoot", "th", + "thead", "tr", "ul", +]); + function hasBlockLevelRoot(html: string): boolean { const parser = new DOMParser(); const doc = parser.parseFromString(`<div>${html}</div>`, "text/html"); const container = doc.body.firstElementChild; if (!container || container.children.length === 0) { return false; } const firstElement = container.firstElementChild; if (!firstElement) return false; - const blockLevelTags = new Set([ - "address", "article", "aside", "blockquote", "details", "dialog", - ... - "ul", - ]); - - return blockLevelTags.has(firstElement.tagName.toLowerCase()); + return BLOCK_LEVEL_TAGS.has(firstElement.tagName.toLowerCase()); }
- Check childNodes instead of firstElementChild to detect leading text - Return false (needs wrapping) if meaningful text precedes any element - Ignore whitespace-only text nodes before elements - Add tests for mixed content edge cases (leading text before block/inline elements) - Ensures all text content is properly wrapped even when mixed with block elements
ThomasAFink
force-pushed
the
bugfix-zmskvr-1229-html-structure-in-callout
branch
from
a68bf0d to
bf81345
Compare
- Fix mismatched closing tags: replace </span> with </p> and </div> - Corrects HTML structure after previous refactoring to use <div> wrapper - Resolves linting error: Unexpected closing tag 'span'
…-callout wrapping in <p> - Wrap all content sections in a single container <div> within the content slot - Prevents muc-callout from wrapping multiple top-level elements in <p> tag - Ensures valid HTML structure when muc-callout processes the content slot - Fixes invalid HTML nesting: <p><div>...</div></p>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@zmscitizenview/src/components/Appointment/AppointmentSelection/AppointmentPreview.vue`:
- Around line 14-15: The aria-label on the info callout is hardcoded as
"Information" and should use the app's translation function instead; update the
element in AppointmentPreview.vue to call the i18n translator (use the same
t(...) helper used elsewhere in this template/component) for the aria-label so
it becomes aria-label="t('Information')" or the appropriate translation key,
ensuring you import/receive the same t reference used by other strings in this
component.
In `@zmscitizenview/src/utils/sanitizeHtml.ts`:
- Around line 121-127: The current wrap logic returns `<p>${trimmed}</p>` when
hasBlockLevelRoot(trimmed) is false, which produces invalid HTML for mixed
content like "Some text<div>x</div>"; instead, parse trimmed (e.g., with
DOMParser or a lightweight regex split) and iterate child nodes: accumulate
consecutive text/inline nodes into a new <p> string and emit block-level
elements (div/section/article/etc.) as-is, flushing any accumulated inline text
as a <p> before each block; update the code around hasBlockLevelRoot and the
final return to build and return the concatenated serialized string (use the
existing hasBlockLevelRoot(trimmed), trimmed variable, and the
sanitizeHtml/sanitize function scope to locate where to implement the
node-iteration/accumulation logic).
🧹 Nitpick comments (2)
zmscitizenview/src/utils/sanitizeHtml.ts (2)
62-102: MoveblockLevelTagsto module scope to avoid repeated allocations.This
Setis a static constant but gets re-created on every call tohasBlockLevelRoot. Hoist it to module level.♻️ Suggested refactor
+const BLOCK_LEVEL_TAGS = new Set([ + "address", "article", "aside", "blockquote", "details", "dialog", + "dd", "div", "dl", "dt", "fieldset", "figcaption", "figure", + "footer", "form", "h1", "h2", "h3", "h4", "h5", "h6", + "header", "hgroup", "hr", "li", "main", "nav", "ol", "p", + "pre", "section", "table", "tbody", "td", "tfoot", "th", + "thead", "tr", "ul", +]); + function hasBlockLevelRoot(html: string): boolean { // ... - const blockLevelTags = new Set([...]); - return blockLevelTags.has(element.tagName.toLowerCase()); + return BLOCK_LEVEL_TAGS.has(element.tagName.toLowerCase()); // ... }As per coding guidelines, "Keep configurable data at high levels."
30-34:DOMParserinstantiation on every call is heavier than needed.Consider caching the parser at module level since
DOMParserinstances are stateless and reusable:+const domParser = new DOMParser(); + function hasBlockLevelRoot(html: string): boolean { - const parser = new DOMParser(); - const doc = parser.parseFromString(`<div>${html}</div>`, "text/html"); + const doc = domParser.parseFromString(`<div>${html}</div>`, "text/html");
| class="m-callout m-callout--info" | ||
| aria-label="Information" |
There was a problem hiding this comment.
Hardcoded aria-label="Information" should use the translation function.
Every other user-facing string in this template uses t(...), but this aria-label is a raw English string. Screen reader users in other locales will hear "Information" in English regardless of their language setting.
- aria-label="Information"
+ :aria-label="t('information')"🤖 Prompt for AI Agents
In
`@zmscitizenview/src/components/Appointment/AppointmentSelection/AppointmentPreview.vue`
around lines 14 - 15, The aria-label on the info callout is hardcoded as
"Information" and should use the app's translation function instead; update the
element in AppointmentPreview.vue to call the i18n translator (use the same
t(...) helper used elsewhere in this template/component) for the aria-label so
it becomes aria-label="t('Information')" or the appropriate translation key,
ensuring you import/receive the same t reference used by other strings in this
component.
| // Check if content already has a block-level root element | ||
| if (hasBlockLevelRoot(trimmed)) { | ||
| return trimmed; | ||
| } | ||
|
|
||
| // Wrap plain text or inline-only content in <p> tag | ||
| return `<p>${trimmed}</p>`; |
There was a problem hiding this comment.
Wrapping mixed content (leading text + block element) in <p> produces invalid HTML.
When hasBlockLevelRoot returns false for input like "Some text<div>content</div>", the result is <p>Some text<div>content</div></p> — a <div> inside a <p> is invalid per the HTML spec (browsers will auto-close the <p> before the <div>, causing unpredictable DOM structure).
This edge case is tested and apparently intentional (spec line 164-171), but worth being aware that the browser will not render the markup as written. If this input pattern is realistic, consider splitting the text node and block element rather than wrapping everything in <p>.
🤖 Prompt for AI Agents
In `@zmscitizenview/src/utils/sanitizeHtml.ts` around lines 121 - 127, The current
wrap logic returns `<p>${trimmed}</p>` when hasBlockLevelRoot(trimmed) is false,
which produces invalid HTML for mixed content like "Some text<div>x</div>";
instead, parse trimmed (e.g., with DOMParser or a lightweight regex split) and
iterate child nodes: accumulate consecutive text/inline nodes into a new <p>
string and emit block-level elements (div/section/article/etc.) as-is, flushing
any accumulated inline text as a <p> before each block; update the code around
hasBlockLevelRoot and the final return to build and return the concatenated
serialized string (use the existing hasBlockLevelRoot(trimmed), trimmed
variable, and the sanitizeHtml/sanitize function scope to locate where to
implement the node-iteration/accumulation logic).
d1c0b3e to
4219a4c
Compare
2 participants
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
New Features
Bug Fixes
Tests