1047330: Fixed chart accessibility, b-unit, leak issues. - #44
1047330: Fixed chart accessibility, b-unit, leak issues.#44Yokesh-SF4393 wants to merge 5 commits into
Conversation
Sittiq3586
left a comment
Sittiq3586
left a comment
There was a problem hiding this comment.
Major concerns (5) — must address before merge:
- Memory fix is incomplete — deprecated static caches remain in fallback path for non-SfChart callers and sibling chart components must be checked separately
- Performance regression — per-instance caching re-measures per chart instead of once per process (consider bounded LRU)
- Thread-safety bug — _requestedFontKeys is List, not concurrent
- First-tooltip narration race — setTimeout(…, 0) defers ARIA attributes; first update may reach AT without them
- Deprecated member call sites — fallback paths still reference the obsolete static members; verify TreatWarningsAsErrors won't break the build
Minor concerns (5):
- 3 XML-doc syntax errors (will break doc-gen)
- Bundled ChartAxisRenderer rendering fix should be a separate PR
- Casings diverge between samples and tests
- _tooltipLiveObserver doc-comment clarify-singleton intent
- Validator XML doc should note case-insensitivity
| /// It will be removed in a future major version. | ||
| /// </para> | ||
| /// </remarks> | ||
| [System.Obsolete("Use SfChart._fontSizeCache and MeasureText(string, ChartFontOptions, object) overload instead. This static cache causes memory leaks on long-lived Blazor Server hosts.")] |
There was a problem hiding this comment.
Remove the obsolete properties, if it doesn't used.
There was a problem hiding this comment.
Removed obsolete properties
| /// <param name="character">The character to measure.</param> | ||
| /// <param name="font">The font settings used during measurement.</param> | ||
| /// <returns>The measured character size.</returns> | ||
| private static Size GetCharSize(object chart, char character, ChartFontOptions font) |
There was a problem hiding this comment.
The new methods you were implemented was also presented in the class, you can revamp the methods to reduce multiple methods.
There was a problem hiding this comment.
Revamped the methods.
6074d47
This reverts commit 6074d47.
Sittiq3586
left a comment
Sittiq3586
left a comment
There was a problem hiding this comment.
PR #44 Review — Chart accessibility, bUnit, and leak fixes
Repo: syncfusion/blazor-toolkit
Branch: 1047330-charts → main
Diff: +385 / −120 across 20 files
Author: @Yokesh-SF4393
Status: Open — 1 requested changes (@Sittiq3586), 2 older approvals dismissed
Summary
This PR addresses three categories of bugs in the Blazor Toolkit Chart component:
- Invalid ARIA roles propagated to the DOM (
"count","Count"— not valid WAI-ARIA roles) - Decorative SVG nodes leaking focus + unlabeled-image announcements to assistive technology
- Process-wide static font-measurement cache causing memory leaks on long-lived Interactive Server hosts
The fix introduces a central role validator, makes decorative SVG attributes conditional, and moves font caches to an instance-scoped lifetime on SfChart.
Verification Against Stated Root Causes
| Stated root cause | Fix landed? | Notes |
|---|---|---|
Sample count/Count (invalid ARIA role) |
✅ | Validated at setter on six types; samples and tests now use "status" / "heading" |
SvgRect/SvgPath unconditionally rendered tabindex="" and role="img" |
✅ | New EffectiveTabIndex / EffectiveRole suppress attributes when decorative |
| Tooltip created without ARIA live region | Description in the PR body mentions role="status" + aria-live + MutationObserver in chart.js, but no JS files are modified in this PR |
|
Static SizePerCharacter/ChartFontKeys leaks in Interactive Server |
✅ | Moved to per-instance ConcurrentDictionary on JsInteropState; cleared in DisposeAsyncCore |
DisposeAsyncCore overridden only to call base |
✅ | Override removed; cache clear added in lifecycle partial |
Code-Review Findings
1. ✅ ARIA-role validator (src/Components/Charts/Common/Utils/Helper.cs)
Centralized in DataVizCommonHelper.AriaRoleValidator with the WAI-ARIA 1.2 abstract role set and case-insensitive comparison. Applied via setter across:
ChartAnnotations.csChartSubTitleStyle.csChartTitleStyle.csLegendSettings.csChartSeries.csChartTrendline.csSfChart.razor.Members.cs
This is the right shape — fail-fast at component init with a clear ArgumentException and helpful URL.
Minor nit: Helper.cs ends without a trailing newline (\ No newline at end of file). Please add one and check other touched files for the same; some SDKs warn on it.
2. ⚠️ Decorative SVG attributes — close, but asymmetric
SvgRect.razor.cs suppresses role/tabindex when AriaHidden == "true".
SvgPath.razor.cs suppresses only when AccessibilityText is empty — not when AriaHidden == "true".
role="@(string.IsNullOrEmpty(EffectiveRole) ? null : (object)EffectiveRole)"This means a path with AriaHidden="true" and no AccessibilityText will still emit role="img" because EffectiveRole only checks AccessibilityText. Recommend:
private string EffectiveRole =>
(string.Equals(AriaHidden, "true", StringComparison.OrdinalIgnoreCase)
|| string.IsNullOrEmpty(AccessibilityText))
? string.Empty : "img";Symmetry between SvgRect and SvgPath (both gated by AriaHidden OR no accessible name) is desirable — the PR description lumps them together, but the implementation differs.
Also note: tabindex="0" (default empty TabIndex) was the prior bug. string.IsNullOrEmpty(EffectiveTabIndex) returning null correctly omits the attribute in Razor — but only because the cast is (object). Worth a unit test asserting the attribute is absent from the rendered HTML, not just empty-valued.
3. ⚠️ Font cache leak fix — incomplete per reviewer
The reviewer raised two valid concerns, partially addressed:
a. Non-SfChart callers still take a fallback path. In ChartHelper.cs:
var sfChart = chart as Charts.SfChart;
if (sfChart is null)
return GetCharSize(character, font);Non-SfChart call sites silently fall through to the no-cache, character-by-character approximation. If any sibling chart component (SfSparkline, SfRangeNavigator, SfStockChart, augmentation components, etc.) calls this overload, they re-measure indefinitely. Either:
- Audit and migrate sibling charts to forward
this, or - Document the limitation in XML doc and add a TODO with a tracking issue.
b. ConcurrentDictionary<string, byte> _requestedFontKeys is correct, but only used inside GetCharSizeListAsync and GetDistinctCharacter. The byte value carries no semantics; consider using a strongly-typed set marker or simply a HashSet<string> held behind a lock to make intent clearer. TryAdd here is also subtly different from Add — both Add and Contains+Add patterns were replaced; keep a unit test that double-enqueues the same key and expects a no-op.
c. DisposeAsyncCore() now clears caches, but JsInteropState._fontSizeCache and _requestedFontKeys are non-null ConcurrentDictionary fields initialized inline. The Clear() call uses ?. defensively. If JsInteropState is ever constructed via new JsInteropState() after partial init (e.g., serialization), _fontSizeCache could be null. Right now it's assigned with = new(), so this is fine — keep an eye out.
4. 🐛 Currently broken test — ChartSubTitleStyle Count → heading (bUnit)
In tests/Syncfusion.Blazor.Toolkit.BUnitTest/Charts/Chart/Axis/ChartBasic.razor:
- AccessibilityRole="Count"
+ AccessibilityRole="heading"The bUnit test was updated to "heading" while the Playwright sample was updated to "Status" ("status" after validator lower-casing). Casings now diverge between sample and test — flagged explicitly by the reviewer. Recommend a unified helper or shared constant.
Also: there is no negative-path test that asserts ArgumentException is thrown when an invalid role is set on any of the six types. E.g.:
Assert.Throws<ArgumentException>(
() => ctx.RenderComponent<SfChart>(p => p.Add(x => x.AccessibilityRole, "count")));Adding these is essential to prevent regressions of the original bug — the validator is the entire safety net for finding #1.
5. 🧹 ChartAxisRenderer.cs — bundling unrelated fix
The two-line change:
- option.StrokeWidth = Axis?.Renderer?.MajorGridLinesWidth ?? 0;
+ option.StrokeWidth = Axis?.MajorGridLines.Width ?? 0;is the right shape (fixes null/incorrect grid width), but it's unrelated to accessibility/leak fixes. Reviewer flagged "Bundled ChartAxisRenderer rendering fix should be a separate PR". Agree — keep PRs atomic; it complicates bisect and cherry-pick into patch releases.
6. 📝 XML doc / SDK impact
- Three
<remarks>blocks now exceed style guides in some files (long inline comments). Verifydotnet build /p:TreatWarningsAsErrors=trueis green if the SDK is configured that way. - The XML cref list in
Helper.csreferences types in other files (SfChart, etc.). Cross-fileinternalcref resolution typically requiresInternalsVisibleToon the docs assembly or that the cref be reachable. Confirm CI builds docs cleanly. - Removing
protected override ValueTask DisposeAsyncCore()fromSfChart.razor.cswas correct; ensure nothing relied on dispose order (none seen in diff). The lifecycle now lives entirely in the partialSfChart.razor.LifeCycle.cs.
7. 🟡 Missing from the diff (vs. description)
chart.jstooltiprole="status"/aria-live/ MutationObserver — described in the PR body as one of the five solution items but no JavaScript file is modified. The "First-to-tooltip narration race" reviewer concern is therefore unresolved in this PR. Either land the JS change in this PR or re-scope the description.- No release notes added in
RELEASE.md/ changelog (typical for Syncfusion). The PR says "No breaking changes", but switchingAccessibilityRolefrom a free-string auto-prop to a validating one is a behavior change: existing apps using"count"will now throwArgumentExceptionat render. Confirm with PM whether this is acceptable pre-release vs. needs a softer landing (warning first, throw in the next major).
8. 🔍 Casing / RTL / fallback consistency
The MeasureText(string, ChartFontOptions, object chart) overload:
var sfChart = chart as Charts.SfChart;
if (sfChart is not null && sfChart._fontSizeCache.TryGetValue(key, out Size? value))
{
charSize = value;
return new Size(charSize.Width * (fontSize / 100), charSize.Height * (fontSize / 100));
}When chart is not an SfChart, the function falls through to the non-RTL loop without caching. This means RTL measurements on non-SfChart callers will be re-measured (or worse, computed via the no-cache GetCharSize(character, font) path with a default fallback width of 50 px). If any non-SfChart consumer (datasets, legend rendering, annotation rendering pipeline, etc.) calls this code path with RTL text, results may be inconsistent with the SfChart path. Worth a follow-up audit or at minimum an explicit comment.
Strengths
- ✅ Solid accessibility posture: fail-fast validation at the parameter setter is the correct pattern. Decorative-node suppression is the right primitive.
- ✅ Instance scoping of caches is the textbook fix for the Interactive Server leak.
- ✅ Casing-correct role set uses
StringComparer.OrdinalIgnoreCaseconsistently. - ✅ Removal of redundant override + clean docs help the next maintainer.
- ✅ Samples and most tests updated (
Annotation.razor,ChartBasics.razor,Annotation.razortest).
Verdict
Approve after these are addressed (or disagreements noted):
1. Required before merge
- Land the
chart.jstooltip ARIA-live fix in this PR, or scope the description to exclude it and open a follow-up (currently the PR claims a fix that isn't in the diff). - Add negative-path
ArgumentExceptionunit tests for invalid ARIA roles on all six types. - Align test vs. sample role casing (
headingvs.status) or share via a constant.
2. Strongly recommended
- Make
SvgPath.EffectiveRolegate onAriaHidden == "true" || AccessibilityText empty, matchingSvgRect. - Audit all non-
SfChartcallers ofMeasureText/GetDistinctCharacter, or document the limitation with a tracking issue. - Move
ChartAxisRenderer.cschange to its own PR.
3. Nice to have
- Confirm
dotnet build /p:TreatWarningsAsErrors=trueis clean. - Add trailing newline to
Helper.cs(and verify other edited files). - Promote the role validator to a public/internal type that other toolkit components can reuse.
4. Behavior-change callout
The validator changes the public-facing surface contract. Coordinate with docs/release notes and confirm with PM before tagging a release. Consider landing as a warning for one release cycle before promoting to ArgumentException.
Note: This file was generated as a code-review artifact. The MCP
my-mcp-serveronly points athttps://gitea.syncfusion.com, which doesn't host the GitHub repo, so the comment couldn't be auto-posted. Copy the relevant sections into the GitHub PR thread manually.
Review changes:
|
5 participants
Bug description
Need to fix the accessibility issue - aria role issues, rectangle/path focus issues, release b-unit test cases failures, memory leak issues.
Root cause
Solution description
Review changes:
Code Studio usage(Mandatory)
Code Studio used in this PR/MR?
If
Yes: Primary use (choose one)Outcome
If “Cost time” explain in short (1 or 2 lines):
Impact assessment
Reason for not identifying earlier
This was recently identified by testing the with MS audit and AI agents. Now identified and fixed.
Areas tested against this fix
Breaking changes
breaking-issue)If yes, provide breaking commit details link and migration guidance.
Regression testing
Action taken to prevent recurrence
Automation status
Cross-platform verification
Related issues
Is this issue present in EJ2 or other components?
needs-attention-coreteam)Output screenshots
Post the output screenshots if a UI is affected or added due to this bug.
API changes
Performance verification
Reviewer Checklist