OrchardCore icon indicating copy to clipboard operation
OrchardCore copied to clipboard

Fix pager HTML validation error: remove invalid <a> when disabled

Open ziadalazwak opened this issue 3 months ago • 15 comments

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

ziadalazwak avatar Sep 24 '25 19:09 ziadalazwak

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.

github-actions[bot] avatar Sep 24 '25 19:09 github-actions[bot]

@dotnet-policy-service agree

ziadalazwak avatar Sep 24 '25 19:09 ziadalazwak

Applied your suggestion, thanks @hishamco

ziadalazwak avatar Sep 24 '25 23:09 ziadalazwak

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.

ziadalazwak avatar Sep 25 '25 06:09 ziadalazwak

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?

ziadalazwak avatar Sep 26 '25 15:09 ziadalazwak

Could you please take a GIF screenshot to see what it looks like now

hishamco avatar Sep 26 '25 15:09 hishamco

As shown in the GIF below, the pager now behaves correctly:

Animation

  • On the first page, the “Previous” and “First” links are disabled without rendering invalid <a> tags.

ziadalazwak avatar Sep 26 '25 18:09 ziadalazwak

@Piedone, do you have anything to add here, or shall we merge

hishamco avatar Sep 26 '25 18:09 hishamco

I didn't check the code (on mobile) but from the above comments the results look good.

Piedone avatar Sep 26 '25 22:09 Piedone

@wAsnk, do you want to check this PR?

hishamco avatar Sep 27 '25 00:09 hishamco

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!

ziadalazwak avatar Sep 30 '25 23:09 ziadalazwak

Nothing else from my side, up to Hisham.

Piedone avatar Oct 01 '25 00:10 Piedone

@wAsnk, do you want to check this PR?

Looks good to me.

wAsnk avatar Oct 01 '25 06:10 wAsnk

@hishamco can't this be merged?

Piedone avatar Oct 07 '25 20:10 Piedone

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

ziadalazwak avatar Oct 13 '25 20:10 ziadalazwak

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.

github-actions[bot] avatar Dec 13 '25 12:12 github-actions[bot]