Fix pager HTML validation error: remove invalid <a> when disabled
When rendering the Pager shape, the “Previous” link is shown as disabled on the first page.
However, it was still outputting an element with a rel attribute but no href, which causes an HTML validation error.
Example before fix:
<li class="pager__item page-item disabled">
<a rel="prev"><</a>
</li>
This PR updates the ActionLink shape to remove the entirely when disabled, while keeping the <li class="disabled"> placeholder for consistent layout.
Example after fix:
<li class="pager__item page-item disabled"></li>
Fixes #17907
Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request.
If you like Orchard Core, please star our repo and join our community channels.
@dotnet-policy-service agree
Applied your suggestion, thanks @hishamco
I removed shape.Attributes.Remove("href") because when a pager item is disabled,
we now skip rendering the <a> tag entirely. Since there’s no <a> in that case,
there’s no need to remove href anymore.
Hi @hishamco, just wanted to follow up on your last comment. Since the tag is no longer rendered for disabled items, I removed the shape.Attributes.Remove("href") call as it’s not needed anymore. Could you please confirm if this resolves your concern?
Could you please take a GIF screenshot to see what it looks like now
As shown in the GIF below, the pager now behaves correctly:
- On the first page, the “Previous” and “First” links are disabled without rendering invalid
<a>tags.
@Piedone, do you have anything to add here, or shall we merge
I didn't check the code (on mobile) but from the above comments the results look good.
@wAsnk, do you want to check this PR?
Hi @hishamco, @Piedone — just wanted to follow up to see if there’s anything else needed from my side to move this PR forward. Thanks again for the review and guidance!
Nothing else from my side, up to Hisham.
@wAsnk, do you want to check this PR?
Looks good to me.
@hishamco can't this be merged?
Hi @hishamco,
It’s been a while since the last update, and I noticed the PR has already passed all checks and received approvals. Could you please let me know if there’s any reason it hasn’t been merged yet?
I’ve been waiting for some feedback or final confirmation, and it’s a bit discouraging not to hear back after addressing all comments and providing the requested changes. I’d really appreciate an update on this.
Thanks, Ziad
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.