OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.23k stars 2.34k forks source link

HTML validation and accessibility errors in some themes #11242

Open DemeSzabolcs opened 2 years ago

DemeSzabolcs commented 2 years ago

Describe the bug

There are accessibility problems in:

Additional Info: Links must have discernible text: > > > > > > > > >

            </a>
<a href="#">
                <img class="img-fluid img-brand d-block mx-auto" src="/media/logos/google.svg" alt="">
            </a>
<a href="#">
                <img class="img-fluid img-brand d-block mx-auto" src="/media/logos/facebook.svg" alt="">
            </a>
<a href="#">
                <img class="img-fluid img-brand d-block mx-auto" src="/media/logos/ibm.svg" alt="">
            </a>
<a class="btn btn-dark btn-social mx-2" href="#!"><i class="fab fa-twitter" aria-hidden="true"></i></a>
<a class="btn btn-dark btn-social mx-2" href="#!"><i class="fab fa-facebook-f" aria-hidden="true"></i></a>
<a class="btn btn-dark btn-social mx-2" href="#!"><i class="fab fa-linkedin-in" aria-hidden="true"></i></a>
Also there are elements with insufficient color contrasts, but I didn't include those errors. For those see: #11241

- [The Coming Soon Theme](https://github.com/OrchardCMS/OrchardCore/tree/main/src/OrchardCore.Themes/TheComingSoonTheme) on the home page.

Asserting the accessibility analysis result failed. Check the accessibility report failure dump for details. -------- Shouldly.ShouldAssertException : axeResult.Violations should be empty but had 1 item and was [Selenium.Axe.AxeResultItem (14851760)]

Additional Info: Links must have discernible text: > > >

There are HTML validation errors in:
- [The Coming Soon Theme](https://github.com/OrchardCMS/OrchardCore/tree/main/src/OrchardCore.Themes/TheComingSoonTheme) on the home page.

" Check the HTML validation report in the failure dump for details. -------- Shouldly.ShouldAssertException : validationResult.Output should be empty but was "12bbd89b-7645-413b-a1d3-d69909ac40b9.html 25:55 error The autoplay attribute is not allowed on

Ôťľ 2 problems (2 errors, 0 warnings)

More information: https://html-validate.org/rules/no-autoplay.html https://html-validate.org/rules/attribute-allowed-values.html "



### To Reproduce
Steps to reproduce the behavior:
1. Set up Orchard Core with the [agency recipe](https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheAgencyTheme/Recipes/agency.recipe.json)/[coming soon recipe](https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheComingSoonTheme/Recipes/comingsoon.recipe.json).
2. Login and go to the homepage.
3. See that there are accessibility errors and/or HTML validation errors on the given site.

### Expected behavior
After setting up Orchard Core with the [agency recipe](https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheAgencyTheme/Recipes/agency.recipe.json)/[coming soon recipe](https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore.Themes/TheComingSoonTheme/Recipes/comingsoon.recipe.json) on the home page, there should be no accessibility and/or HTML validation problems.
hishamco commented 2 years ago

As @Skrypt said earlier we are not the creators of the themes and we should use theme as it is

Piedone commented 2 years ago

While with https://github.com/OrchardCMS/OrchardCore/issues/11241 I kind of agree that it's better to fix in the theme (since the fix is not just a technical one but affects the design), here I don't see why we'd need to keep these errors and wait for an update in the original project: The Liquid code is a derivative work, not part of the original template but part of Orchard, and the fixes are solely technical. We can fix them in Orchard and at the same time let the original author now about them to fix them in the original code too.

Let me follow up under the PR.

Piedone commented 2 years ago

There's a new issue in the Blog theme, duplicated fas class:

image

Piedone commented 2 years ago

And there's a new accessibility issue with The Default Theme too. This is an HTML file, just rename to .html, but GitHub doesn't allow uploading them. AccessibilityReport.txt

Skrypt commented 2 years ago

https://github.com/OrchardCMS/OrchardCore/blob/62eed89890b6b9b6d71a02e1465269b9bb512207/src/OrchardCore.Themes/TheBlogTheme/Views/Content-Category.liquid#L10

Remove the first "fas" since this seems to be added from database with the IconPicker. But maybe it was added for backward compatibility when we switched to a new version of FontAwesome for those who we're using "fa" instead of "fas". So, if that's the case then the issue is that the IconPicker saves "fas" in the database.

sebastienros commented 2 years ago

Just fix what is specifically added by us, or file issues in the official repos with PR if possible. Then we'll update the templates.