Conversation
skjnldsv
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2324 +/- ##
===========================================
+ Coverage 43.79% 59.16% +15.36%
===========================================
Files 15 15
Lines 427 453 +26
Branches 81 95 +14
===========================================
+ Hits 187 268 +81
+ Misses 211 164 -47
+ Partials 29 21 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
skjnldsv
changed the title
Done @susnux 🚀 |
6ca5db6 to
e245c3e
Compare
|
Oh wow I did not expect this to happen 😆 |
Good call, I did not test it 🤔 |
I can only imagine integrations like e.g. text in files or similar. |
Yeah, that seems like a very specific race condition and context to have. |
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
I think one question we should solve would be if we want to keep HTML support, otherwise we could simplify this a lot.
We could still allow custom toast content by exposing the component from nextcloud-vue.
So in that case the component would have a onBeforeCreate that checks if the container is available or creates it if necessary.
And the NcToast component would roughly look like:
<template>
<Teleport :to="toastContainerSelector">
<div><!--- .... --->
<slot><!-- ... --></slot>
</div>
</Teleport>
</template>This would allow us to drop all of the custom "mounting" and announcement logic.
As then the announcement would also just teleport to the corresponding live region¹.
¹: This is currently a problem if you get 2 notifications quickly then the content is announced but directly cleared again and new notification is announced.
So teleporting into it would solve it because then its a list of active announcements.
Interesting topic! So, regarding the announcements, announcements != toasts. I'll think about it tomorrow. I'll check if I can come up with something clever. |
|
Hey @skjnldsv, thanks for working on this. Is this still a PR you want to get in or should I make a new one, thanks! |
|
@GVodyanov it should still go in at some point. But it might require a revive :) |
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Addresses susnux's blocking review comment: toast/container styles were global unscoped BEM classes, risking collisions when multiple library bundle versions are loaded on the same page. Both components now use `<style module>`, matching the ConflictPicker convention already in the repo. Also: - close prop renamed to noClose (defaults false), per susnux - toast container + live regions extracted into ToastContainer.vue, shared via a window-global singleton so multiple bundle versions render into one stack instead of stacking separately - polite announcements now queue as separate <li> items instead of clobbering each other on rapid succession - toast stack repositions next to NcAppNavigation via the real navigation-toggled event - dropped stray @eslint/js devDependency bump that had crept in, unrelated to this change Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Codecov flagged ~88% patch coverage on toast.ts with the onDismiss cleanup path and showUndo's invalid-callback guard uncovered. Adds: - hideToast() / close button removal + onRemove callback - auto-dismiss timeout and permanent (loading) toast - showUndo: invalid callback throws, undo button calls onUndo and dismisses - selector option mounts into a custom host element - toast container gets the nav-open modifier on navigation-toggled toast.ts coverage: 80.76% -> 93.58% lines. Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
|
Revived and addressed your comments Ferdinand, please feel free to have another look @susnux @GVodyanov |
showMessage(html, { isHTML: true }) announced the raw markup string to
the live region instead of its visible text — getVisibleText() (which
strips aria-hidden subtrees) was only applied to Node messages, never
to HTML strings, so screen readers would read out literal tag syntax.
Introduces getAnnouncementText() so HTML strings go through the same
parse-and-strip path as Node messages.
Also covers accessibility-relevant paths that had no tests:
- isHTML announcements are stripped to plain text
- isHTML and Node messages both skip aria-hidden subtrees in
announcements (getVisibleText was previously untested)
- undo toasts expose two distinguishable interactive elements
(Undo action + Close button) rather than one ambiguous one
- toasts stack in the order they were shown
Assisted-by: ClaudeCode:claude-sonnet-5
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ment CodeQL flagged the innerHTML assignment used to strip/extract text from HTML message strings (js/xss-through-dom, "DOM text reinterpreted as HTML") on lib/toast.ts:49. Not exploitable as written — the element was never attached to the document, and innerHTML-inserted <script> tags never execute regardless of attachment — but the pattern is still assigning untrusted input to .innerHTML of a document.createElement'd node, which CodeQL's dataflow can't distinguish from a live-DOM sink. Switch both spots (the isHTML-announcement path added in the previous commit, and the pre-existing plain-text stripping path) to `new DOMParser().parseFromString(data, 'text/html')`. The parsed result is a standalone Document, never part of the page's DOM at all, which removes the ambiguity outright rather than relying on "detached == safe" reasoning. Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
| /** | ||
| * Extract visible text from a node, skipping subtrees marked aria-hidden. | ||
| * This prevents decorative elements (e.g. spinner SVGs) from leaking into | ||
| * the live-region announcement. | ||
| * | ||
| * @param node The DOM node to extract text from | ||
| * @return The concatenated visible text content of the node and its children | ||
| */ | ||
| function getVisibleText(node: Node): string { | ||
| // Skip any nodes that are hidden from assistive technologies | ||
| if (node instanceof Element && node.getAttribute('aria-hidden') === 'true') { | ||
| return '' | ||
| } | ||
|
|
||
| // For text nodes, return the text content directly | ||
| if (node.nodeType === Node.TEXT_NODE) { | ||
| return node.textContent ?? '' | ||
| } | ||
|
|
||
| // For element nodes, recursively extract text from child nodes | ||
| return Array.from(node.childNodes).map(getVisibleText).join('') | ||
| } | ||
|
|
||
| /** | ||
| * Derive the plain text to announce in the live region for a given message. | ||
| * HTML strings are parsed so tags don't leak into the announcement and | ||
| * aria-hidden subtrees are skipped, matching the Node message behaviour. | ||
| * | ||
| * @param data The message passed to showMessage: plain text, HTML string, or a DOM Node | ||
| * @param isHTML Whether `data` should be parsed as HTML | ||
| * @return The visible plain-text announcement | ||
| */ | ||
| function getAnnouncementText(data: string | Node, isHTML: boolean): string { | ||
| if (typeof data === 'string') { | ||
| if (!isHTML) { | ||
| return data | ||
| } | ||
| // Parse in a standalone document (never attached to the page) so the | ||
| // markup is never live DOM, not just visually inert. | ||
| return getVisibleText(new DOMParser().parseFromString(data, 'text/html').body) | ||
| } | ||
| return getVisibleText(data) | ||
| } |
There was a problem hiding this comment.
This should be placed below the exported consts and interfaces.
(you can also place them below exported functions if you like as they are functions they get hoisted on execution anyways and are defined when needed by other function)
Per susnux's review comment: getVisibleText/getAnnouncementText were declared before the public ToastType/ToastAriaLive/ToastOptions/ ToastHandle exports. Function declarations are hoisted, so this was never a runtime issue, just readability — move them next to their only caller (showMessage) so the exported API surface reads first. Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
5 participants
Fix #2254
Summary
Drop
toastify-jsRemoves
toastify-js(last third-party UI dep for toasts) and replaces it with a self-contained ToastNotification.vue. The public API is unchanged.NcButton, spinner usesNcLoadingIcon--clickable-area-large, positioned below the headerAccessibility
aria-liveregions (polite + assertive) are created once and reused, injecting a live region that already carriesaria-liveis unreliable in NVDA/JAWS, so they are pre-created before any toast is shown{message}placeholder ("Error: {message}") so the colour cue is also conveyed to screen readers (WCAG 1.4.1)role="alert"/role="status"set on each toast based on the resolvedariaLivelevelNcLoadingIconis wrapped inaria-hidden="true"so the spinner doesn't pollute announcementsDemo
Peek.18-03-2026.15-40.mp4