fix(link): ensure disabled links are actually disabled when tab-navigated#287
fix(link): ensure disabled links are actually disabled when tab-navigated#287kevinvenclovas wants to merge 1 commit intoLumexUI:mainfrom
Conversation
📝 WalkthroughWalkthroughModified the LumexLink component to properly disable links by conditionally setting HTML attributes. When disabled, the href attribute is removed and accessibility attributes (tabindex="-1", aria-disabled="true") are applied, preventing navigation and tab-access. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LumexUI/Components/Link/LumexLink.razor.cs (1)
92-97:⚠️ Potential issue | 🟠 MajorPrevent
AdditionalAttributesfrom re-enabling disabled links.At Line 92 onward, consumer-supplied attributes are merged last, so
href,tabindex,target, orrelcan override disabled safeguards. That can reintroduce the exact behavior this PR is fixing.Suggested hardening diff
get { var attributes = new Dictionary<string, object>(); - // Only add href if not disabled - if( !Disabled ) - { - attributes["href"] = Href; - } - else - { - // For disabled links, add accessibility attributes - attributes["tabindex"] = "-1"; - attributes["aria-disabled"] = "true"; - } - - if( External && !Disabled ) - { - attributes["target"] = "_blank"; - attributes["rel"] = "noopener noreferrer"; - } - if( AdditionalAttributes is not null ) { foreach( var attribute in AdditionalAttributes ) { attributes[attribute.Key] = attribute.Value; } } + + if( !Disabled ) + { + attributes["href"] = Href; + if( External ) + { + attributes["target"] = "_blank"; + attributes["rel"] = "noopener noreferrer"; + } + } + else + { + attributes.Remove("href"); + attributes.Remove("target"); + attributes.Remove("rel"); + attributes["tabindex"] = "-1"; + attributes["aria-disabled"] = "true"; + } return attributes; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LumexUI/Components/Link/LumexLink.razor.cs` around lines 92 - 97, The merge of AdditionalAttributes in LumexLink.razor.cs currently allows consumer attributes to override safeguards; update the foreach that copies AdditionalAttributes into attributes to skip protected keys when Disabled is true (at least "href", "tabindex", "target", "rel") so consumers cannot re-enable the link; perform case-insensitive key checks (e.g., use a HashSet<string>(StringComparer.OrdinalIgnoreCase)) and only assign attributes[attribute.Key] = attribute.Value when either Disabled is false or the attribute key is not in the protected set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/LumexUI/Components/Link/LumexLink.razor.cs`:
- Around line 92-97: The merge of AdditionalAttributes in LumexLink.razor.cs
currently allows consumer attributes to override safeguards; update the foreach
that copies AdditionalAttributes into attributes to skip protected keys when
Disabled is true (at least "href", "tabindex", "target", "rel") so consumers
cannot re-enable the link; perform case-insensitive key checks (e.g., use a
HashSet<string>(StringComparer.OrdinalIgnoreCase)) and only assign
attributes[attribute.Key] = attribute.Value when either Disabled is false or the
attribute key is not in the protected set.
desmondinho
changed the title
| var attributes = new Dictionary<string, object>(); | ||
|
|
||
| // Only add href if not disabled | ||
| if( !Disabled ) |
There was a problem hiding this comment.
I actually think we should keep the href for semantics. Since a link is not navigable, it won't harm anyway
| } | ||
|
|
||
| if( External ) | ||
| if( External && !Disabled ) |
There was a problem hiding this comment.
And I am not sure that Disabled should affect externality. Is there any specific reason to check if a link is disabled here?
2 participants
Closes #137
Summary by CodeRabbit