Fixed an issue where Breadcrumbs had invalid metadata for disabled pages #4671

Andy9999 · 2021-05-20T22:59:20Z

Invalid Breadcrumb Metadata for Disabled Pages Fixes #4667

Google Search Console complains about invalid metadata for disabled pages (Missing field "item"). The metadata for disabled pages should not be rendered as an "itemListElement". "itemListElement" lists need to have at least one "item", but there is no item getting rendered in case the page is disabled.

Issue #4667

Invalid Breadcrumb Metadata for Disabled Pages Fixes dnnsoftware#4667 Google Search Console complains about invalid metadata for disabled pages (Missing field "item"). The metadata for disabled pages should not be rendered as an "itemListElement". "itemListElement" lists need to have at least one "item", but there is no item getting rendered in case the page is disabled. Issue dnnsoftware#4667

dnfadmin · 2021-05-20T22:59:33Z

All CLA requirements met.

sleupold

Line 176 and 186 look the same to me - both seem to append itemprop twice for enabled pages - is this intended?

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

Removed duplicate line

Andy9999 · 2021-05-21T13:30:21Z

One can test the rendered breadcrumb with the rich results test:
https://search.google.com/test/rich-results

Testing it with Google search console may take a while.

bdukes · 2021-05-21T13:32:53Z

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

+                // Is this tab disabled? If so, only render a span
+                if (tab.DisableLink)
+                {
+                    this._breadcrumb.Append("<span class=\"" + this._cssClass + "\">" + tabName + "</span>");


I like this change; it makes sense and makes things better. One potential issue I see is that we're removing a span that wrapped the disabled page, which may have been used for styling. Should we continue to wrap this span in another span? We can't do anything if that span was targeted using the metadata attributes, but it seems likely to me that there are themes which expect that wrapping element.

Thoughts @dnnsoftware/approvers?

I noticed as well that a nested span without an id or class has been removed - IMO the effect on skins should be minimal and rendered HTML is even clearer without.

I agree with your comment @bdukes, some themes will likely need to be restyled (though minimal effort probably as @sleupold points out)-- I've seen several themes including our own that target with that wrapping <span>

Thank you for your feedback. I could try later today to wrap the span inside of an additional span. That would probably provide better compatibility unless CSS is selecting the itemprop="name".

I just tested to put another span around the span and it looks the same visually. Shall I change the code accordingly? It adds to the DOM model and is less clean on the other hand imho.

I think at this point it's probably more likely to cause a problem if it's missing than to cause a problem by being added. Unless someone has a different opinion, please go ahead and add it in. Thanks!

bdukes · 2021-05-21T13:35:25Z

Thanks @Andy9999! This looks good, we'll review and see if we have any feedback. We'll take care of the milestone and label.

A pull request on GitHub is between two branches, so while the PR is open, any new commit that your push to your develop branch will be added to this PR automatically. Since you used your develop branch, be careful that you don't accidentally push any other changes there. In the future, it will probably be easier for you to create a new branch on your repo for each PR. Let us know if you have any questions about the process.

Added an additional span to improve compatibility with existing skins expecting a wrapping span.

Andy9999 · 2021-05-22T14:16:58Z

OK, done. I have deployed it on our production site and tested it with the Google rich result test tool and it's still OK.

Just for future PRs, how could I implement some sort of switch to have two flavors the right way (with and without the wrapping span)? I mean in a way not to slow down things too much, getting cached and so on. Is there some API to have a switch in the host or portal settings SQL tables for example? Or maybe it could be an option for the skin object?

bdukes · 2021-05-22T17:07:30Z

For a skin/theme object, you can add a new property. For example, you can see that Login contains a LegacyMode property, which controls which markup you get.

bdukes

Thanks!

Added LegacyMode for the skin object. The default (LegacyMode="True") will render the span wrapped in a span. If setting it to "False" it will not wrap the span for disabled pages. This is made to optionally not render unnecessary HTML, while providing skin compatibility with the default setting.

Andy9999 · 2021-05-24T09:28:45Z

Thanks!

You are welcome! Thank you for the additional info about the LegacyMode. I made another change and it is now rendering the span in a span by default (LegacyMode="True"). If someone doesn't like the wrapped span the skin object can be optionally set to LegacyMode="False" to not wrap it. :)

In the meantime I got feedback from Search Console's verification as well for some of the pages that had the error and the issue it previously detected is marked as resolved.

bdukes

I made a couple of suggestions to switch around how the new property works.

Also, if we're adding a new property, I think we need to add it to BreadCrumb.xml also.

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

bdukes · 2021-05-24T13:31:39Z

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

@@ -31,7 +31,8 @@ public partial class BreadCrumb : SkinObjectBase

        public BreadCrumb()
        {
-            this._navigationManager = Globals.DependencyProvider.GetRequiredService<INavigationManager>();
+            this._navigationManager = Globals.DependencyProvider.GetRequiredService<INavigationManager>();
+            this.LegacyMode = true;


Suggested change

this.LegacyMode = true;

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

Added new LegacyMode setting.

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

Renamed LegacyMode to CleanerMarkup and improved help text for new setting.

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

bdukes

👍🏻 Thanks!

Set new "CleanerMarkup" setting to false by default. ;)

…to develop

valadas

Looks good to me

Andy9999 · 2021-05-26T00:45:27Z

👍🏻 thanks yall for helping me with my very first PR.

david-poindexter · 2021-05-26T00:47:01Z

Thank YOU @Andy9999 🎉

sleupold reviewed May 20, 2021

View reviewed changes

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs Outdated Show resolved Hide resolved

Update BreadCrumb.ascx.cs

8a26bf0

Removed duplicate line

bdukes added this to the 9.9.2 milestone May 21, 2021

bdukes added Type: Enhancement Type: Bug and removed Type: Enhancement labels May 21, 2021

bdukes reviewed May 21, 2021

View reviewed changes

Update BreadCrumb.ascx.cs

c007ea2

Added an additional span to improve compatibility with existing skins expecting a wrapping span.

bdukes approved these changes May 22, 2021

View reviewed changes

bdukes reviewed May 24, 2021

View reviewed changes

Andy9999 and others added 4 commits May 24, 2021 17:36

Update BreadCrumb.xml

901d850

Added new LegacyMode setting.

Update DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

7cb974f

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

Update DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs

d2e40d0

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>

Renamed LegacyMode to CleanerMarkup and Improved Help Text

e1af637

Renamed LegacyMode to CleanerMarkup and improved help text for new setting.

bdukes reviewed May 24, 2021

View reviewed changes

DNN Platform/Website/admin/Skins/BreadCrumb.ascx.cs Outdated Show resolved Hide resolved

Fix CleanerMarkup default

d0fc63b

bdukes approved these changes May 24, 2021

View reviewed changes

bdukes requested a review from valadas May 24, 2021 15:58

Update BreadCrumb.ascx.cs

afa1de8

Set new "CleanerMarkup" setting to false by default. ;)

bdukes requested a review from david-poindexter May 24, 2021 15:58

Merge branch 'develop' of https://github.com/Andy9999/Dnn.Platform in…

5997cdc

…to develop

valadas approved these changes May 25, 2021

View reviewed changes

mitchelsellers approved these changes May 25, 2021

View reviewed changes

mitchelsellers merged commit cca8ddc into dnnsoftware:develop May 25, 2021

sleupold mentioned this pull request Jun 6, 2021

Breadcrumb Fails HTML Validation Because of Invalid itemprop for Root Element #4705

Closed

valadas modified the milestones: 9.9.2, 9.10.0 Jul 7, 2021

valadas changed the title ~~Update BreadCrumb.ascx.cs~~ Fixed an issue where Breadcrumbs had invalid metadata for disabled pages Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed an issue where Breadcrumbs had invalid metadata for disabled pages #4671

Fixed an issue where Breadcrumbs had invalid metadata for disabled pages #4671

Andy9999 commented May 20, 2021

dnfadmin commented May 20, 2021 •

edited

Loading

sleupold left a comment •

edited

Loading

Andy9999 commented May 21, 2021 •

edited

Loading

bdukes May 21, 2021

sleupold May 21, 2021

aoverkamp May 21, 2021

Andy9999 May 21, 2021 •

edited

Loading

Andy9999 May 21, 2021 •

edited

Loading

bdukes May 22, 2021

bdukes commented May 21, 2021

Andy9999 commented May 22, 2021 •

edited

Loading

bdukes commented May 22, 2021

bdukes left a comment

Andy9999 commented May 24, 2021 •

edited

Loading

bdukes left a comment

bdukes May 24, 2021

bdukes left a comment

valadas left a comment

Andy9999 commented May 26, 2021

david-poindexter commented May 26, 2021

Fixed an issue where Breadcrumbs had invalid metadata for disabled pages #4671

Fixed an issue where Breadcrumbs had invalid metadata for disabled pages #4671

Conversation

Andy9999 commented May 20, 2021

dnfadmin commented May 20, 2021 • edited Loading

sleupold left a comment • edited Loading

Choose a reason for hiding this comment

Andy9999 commented May 21, 2021 • edited Loading

bdukes May 21, 2021

Choose a reason for hiding this comment

sleupold May 21, 2021

Choose a reason for hiding this comment

aoverkamp May 21, 2021

Choose a reason for hiding this comment

Andy9999 May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Andy9999 May 21, 2021 • edited Loading

Choose a reason for hiding this comment

bdukes May 22, 2021

Choose a reason for hiding this comment

bdukes commented May 21, 2021

Andy9999 commented May 22, 2021 • edited Loading

bdukes commented May 22, 2021

bdukes left a comment

Choose a reason for hiding this comment

Andy9999 commented May 24, 2021 • edited Loading

bdukes left a comment

Choose a reason for hiding this comment

bdukes May 24, 2021

Choose a reason for hiding this comment

bdukes left a comment

Choose a reason for hiding this comment

valadas left a comment

Choose a reason for hiding this comment

Andy9999 commented May 26, 2021

david-poindexter commented May 26, 2021

dnfadmin commented May 20, 2021 •

edited

Loading

sleupold left a comment •

edited

Loading

Andy9999 commented May 21, 2021 •

edited

Loading

Andy9999 May 21, 2021 •

edited

Loading

Andy9999 May 21, 2021 •

edited

Loading

Andy9999 commented May 22, 2021 •

edited

Loading

Andy9999 commented May 24, 2021 •

edited

Loading