Skip to content

1047330: Fixed chart accessibility, b-unit, leak issues. - #44

Open
Yokesh-SF4393 wants to merge 4 commits into
mainfrom
1047330-charts
Open

1047330: Fixed chart accessibility, b-unit, leak issues.#44
Yokesh-SF4393 wants to merge 4 commits into
mainfrom
1047330-charts

Conversation

@Yokesh-SF4393

@Yokesh-SF4393 Yokesh-SF4393 commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Bug description

Need to fix the accessibility issue - aria role issues, tooltip narrator issue, rectangle/path focus issues, release b-unit test cases failures, memory leak issues.

Root cause

  1. Sample author typo. [AccessibilityRole="count"] was set on in [Annotation.razor] thinking [count] was a WAI-ARIA role for a numeric counter. [count] is not a valid WAI-ARIA 1.2 role. The component validator (added in the prior audit round) correctly rejected it at page render with ArgumentException.
  2. The SvgRect and SvgPath Razor templates hardcoded tabindex="@tabindex" and role="img" attributes unconditionally — even when the node was decorative (AriaHidden="true" or no AccessibilityText). Default TabIndex = "" still rendered as tabindex="", which most browsers treat as focusable. Combined with role="img" and no aria-label, screen readers announced an "unlabeled image".
  3. The chart's tooltip element is created by svgbase.Tooltip.appendTo() in chart.js. That library creates a plain
    with no ARIA attributes. WCAG 2.1 SC 4.1.3 (Status Messages) and the WAI-ARIA Authoring Practices both require that dynamic tooltip content have role="status" (or role="alert") + aria-live so screen readers announce updates.

Solution description

  1. Fix invalid ARIA role on Annotation page; page now renders
  2. Fix invalid ARIA role on ChartBasics page; page now renders
  3. Decorative rectangles/paths no longer appear as unlabeled focusable images - SvgRect.razor + SvgPath.razor: omit tabindex / role="img" on decorative SVG nodes
  4. chart.js: helper sets role="status" + aria-live="polite" + aria-atomic="true" on tooltip elements (init-time + on-hover + MutationObserver)

Review changes:

  1. Removed unnecessary imports in ChartHelper.cs
  2. Removed obsolete properties and methods and replaced with proper properties.
  3. Simplified GetCharSize in ChartHelper.cs now calculates the same fallback dimensions directly: Known characters: FontWidthLookup width multiplied by 6.2, Unknown characters: width 50, Height remains 130
  4. Removed RTL lookup from the non-chart MeasureText overload - The old overload could retrieve RTL measurements from the removed static cache. It now uses the same character-by-character fallback path as other text.
  5. Updated documentation - References to the deleted static cache were removed from the instance-aware measurement documentation.
  6. Moved font-key tracking to the chart instance - ChartHelper.cs now receives an SfChart and uses: chart._fontSizeCache, chart._requestedFontKeys. This preserves duplicate-request prevention without process-wide state.
  7. Updated SfChart forwarding method - SfChart.razor.cs: changed GetDistinctCharacter from static to instance-based and passes this to ChartHelper.
  8. Changed font key tracking to use thread-safe ConcurrentDictionary with TryAdd, preserving existing behavior and payload.
  9. Removed the tooltip accessibility race.

Code Studio usage(Mandatory)

  • Code Studio used in this PR/MR?

    • Yes
    • No
  • If Yes: Primary use (choose one)

    • Generate new code
    • Refactor/improve existing code
    • Tests
    • Bug fix / debugging help
    • Docs / comments
    • Review assistance (explanations/summaries)
    • Other:
  • Outcome

    • Saved time
    • Neutral
    • Cost time
  • If “Cost time” explain in short (1 or 2 lines):

Impact assessment

  • Low - Affects a single feature with minimal user impact
  • Medium - Affects multiple features or has moderate user impact
  • High - Critical functionality or significant user impact

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

  • Yes (Tag breaking-issue)
  • No

If yes, provide breaking commit details link and migration guidance.

Regression testing

  • Verified fix doesn't reintroduce previous bugs
  • Checked edge cases and error scenarios

Action taken to prevent recurrence

  • Added/updated unit tests
  • Other (specify): _________________
  • NA

Automation status

  • BUnit (provide PR link: _________________)
  • Playwight (provide PR link: _________________)
  • NA

Cross-platform verification

  • Blazor Server
  • Blazor WASM
  • NA

Related issues

Is this issue present in EJ2 or other components?

  • Resolved in EJ2 (PR link: _________________)
  • Created task for EJ2 (Task link: _________________)
  • Needs attention in other components (tag needs-attention-coreteam)
  • NA

Output screenshots

Post the output screenshots if a UI is affected or added due to this bug.

API changes

  • New API added (API Review task link: _________________)
  • Existing API renamed/modified (API Review task link: _________________)
  • No API changes

Performance verification

  • Verified no memory leaks introduced
  • Verified no performance degradation
  • Not applicable

Reviewer Checklist

  • Reviewed the provided Code Studio usages related information.
  • Code changes follow component guidelines
  • All provided information reviewed and verified
  • Solution addresses the root cause effectively

PrinceOliver
PrinceOliver previously approved these changes Aug 25, 2026

@Sittiq3586 Sittiq3586 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.")]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove the obsolete properties, if it doesn't used.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new methods you were implemented was also presented in the class, you can revamp the methods to reduce multiple methods.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Revamped the methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants