-
-
Notifications
You must be signed in to change notification settings - Fork 770
BreadCrumb skin object to render semantic, accessible breadcrumb markup #6898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Replaced asp:label with nav and ol structure for better semantic markup.
mitchelsellers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note/request included on a change to localization.
Additionally however I do believe this would be a substantially breaking change to introduce, so the planning of how this would be released is something we have to take into consideration. @dnnsoftware/approvers anyone else with thoughts on this?
| @@ -1,2 +1,6 @@ | |||
| <%@ Control Language="C#" AutoEventWireup="false" Inherits="DotNetNuke.UI.Skins.Controls.BreadCrumb" Codebehind="BreadCrumb.ascx.cs" %> | |||
| <asp:label id="lblBreadCrumb" runat="server" EnableViewState="False" itemprop="breadcrumb" itemscope itemtype="https://schema.org/breadcrumb"/> | |||
| <nav class="dnnBreadcrumb" aria-label="Breadcrumb"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is a literal name used for human interaction (via screen readers) I believe this should be pulling from localization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call — agreed. I’ve switched the aria-label value to come from the control’s resx (e.g. BreadCrumbAriaLabel.Text) with a sensible fallback to "Breadcrumb" if the resource is missing. This keeps screen-reader text properly localized.
bdukes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like adding this markup as an option, however I don't think we can change the markup by default, I think it needs to be opt-in (e.g. add a new property for display mode or DisplayAsList or something).
| var changed = false; | ||
|
|
||
| if (url.StartsWith("/", StringComparison.Ordinal)) | ||
| if (url.StartsWith("/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to keep the StringComparison
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed — restored StartsWith(..., StringComparison.Ordinal) (and kept StringComparison usage throughout) to avoid culture-sensitive comparisons.
| this.breadcrumb.Append(this.separator); | ||
| } | ||
| breadcrumb.Append("<li itemprop=\"itemListElement\" itemscope itemtype=\"https://schema.org/ListItem\">"); | ||
| breadcrumb.Append("<a href=\"" + this.homeUrl + "\" class=\"" + this.cssClass + "\" itemprop=\"item\"><span itemprop=\"name\">" + this.homeTabName + "</span></a>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either use a formattable string or AppendFormat.
Also, the tab name will need to be HTML encoded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated — replaced string concatenations with AppendFormat/formattable strings and HTML-encoded tab names (HttpUtility.HtmlEncode) before rendering to prevent malformed markup and ensure safe output.
Removed nav and ol tags from breadcrumb control.
Updated the BreadCrumb class to enhance functionality and maintain compatibility with legacy systems. Introduced new properties for cleaner markup and list-based semantic rendering.
Totally agree on the breaking-change risk. I’ve updated the implementation so the new semantic/list markup is opt-in via a new property (UseListMarkup, default false). |
Agreed — I changed this to opt-in. Added UseListMarkup (default false) so legacy markup remains the default behavior. Setting |
|
@bdukes and @mitchelsellers Thanks for the feedback! I’ve updated the PR to minimize breaking changes and address review notes: Added UseListMarkup (default false) so legacy output remains the default, and semantic markup is opt-in. Kept legacy wrapper Switched the control output to Localized the aria-label via resx with fallback. Restored |
| nav.dnnBreadcrumb ol { | ||
| list-style: none; | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 0.5rem; | ||
| } | ||
|
|
||
| nav.dnnBreadcrumb ol li span { | ||
| margin-inline-start: 0.5rem; | ||
| display: inline-block; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mitchelsellers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry upon further review one small changes suggested.
| // This mimics the old asp:Label output wrapper and keeps schema breadcrumb attrs. | ||
| var breadcrumb = new StringBuilder( | ||
| "<span itemprop=\"breadcrumb\" itemscope itemtype=\"https://schema.org/breadcrumb\">" + | ||
| "<span itemscope itemtype=\"http://schema.org/BreadcrumbList\">"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be broken out into either one initialization & one Append call or just one big string to avoid the concatenated string being allocated.

Summary
This PR refactors the BreadCrumb skin object to render semantic, list-based markup using
<nav>,<ol>, and<li>elements instead of inline spans and separators.The updated output improves accessibility and aligns breadcrumb rendering with common WCAG and ARIA breadcrumb patterns, while preserving existing behavior and customization options.
Fixes #6609
Details
<nav aria-label="Breadcrumb">) and an ordered list (<ol itemscope itemtype="https://schema.org/BreadcrumbList">).Labelwith aLiteralto avoid injecting wrapper markup inside the list container (ensuring valid list structure).<li>per breadcrumb item with Schema.orgBreadcrumbList/ListItemmicrodata andpositionmeta.aria-current="page"to the<li>representing the current page (instead of the<a>), matching common ARIA breadcrumb patterns.<li>(only when another crumb follows) and marked them decorative viaaria-hidden="true", avoiding extra list items while preserving the existingSeparatorcustomization and path resolution.type.scssfornav.dnnBreadcrumb olto prevent list items stacking vertically after switching to list markup, and to ensure the ordered list does not display numeric markers while keeping the visual breadcrumb layout consistent:list-style: none;(prevents numbering/markers)display: flex; align-items: center; gap: 0.5rem;(keeps crumbs inline and spaced)nav.dnnBreadcrumb ol li span { margin-inline-start: 0.5rem; display: inline-block; }(consistent spacing for the separator/content)Impact
Testing
HideWithNoBreadCrumbbehavior.BreadcrumbList.