Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix microdata for the last breadcrumb item #26268

Open
wants to merge 6 commits into
base: staging
from

Conversation

@zero-24
Copy link
Contributor

commented Sep 11, 2019

Pull Request for Issue https://forum.joomla.de/thread/10185-strukturierte-daten-im-breadcrumbs-modul/ (German)

Summary of Changes

Add missing itemprop item to the last breadcrumb item

https://developers.google.com/search/docs/data-types/breadcrumb

Testing Instructions

Expected result

The google tool is happy

Actual result

The google tool compains that the item is missing.

Documentation Changes Required

none

zero-24 added 3 commits Sep 11, 2019
@ChristineWk

This comment has been minimized.

Copy link

commented Sep 11, 2019

I have tested this item successfully on 035c38b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26268.

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I have tested this item 🔴 unsuccessfully on 035c38b

facepalm


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26268.

@zero-24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

facepalm

hmm is something not working than expected?

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

This fails for multiple reasons.

  1. If show last is enabled and this pr is applied then you have changed the last part from text to a link. It is deliberately not a link because it is then a link to itself - easy to see this with a single article menu item

  2. This will change the visual display of any website with show last enabled - we really shouldnt do that.

  3. The code actually says
    // Make a link if not the last item in the breadcrumbs
    Which you haven't removed even though its not true any more

  4. Even if you check the template override in beez you will see that the last element is not a link

So what does google say about breadcrumbs?

Breadcrumb trails on a page indicate the page's position in the site hierarchy. A user can navigate all the way up in the site hierarchy, one level at a time, by starting from the last breadcrumb in the breadcrumb trail.

Which is why most people dont have show last enabled and if you do then you dont expect it to be a link to itself

Never rely on automated tools to test something :)

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

This is definitely a case where the user should create an override.

And even if it is decided to accept this it needs to be applied to beex as well

@zero-24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Which is why most people dont have show last enabled [...]

Well it is enabled by default. So i think most sites have this option enabled.

[...]and if you do then you dont expect it to be a link to itself

Ok got the point so the last is not part of the breadcrumb right? So this would be the correct fix == don't just add them partly to the breadcrumb microdata (5b10fe0)

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@zero-24 I am confused now what the intention is here now. Can you please update the title and description please as now this isnt adding anything ;)

@zero-24 zero-24 changed the title Add missing itemprop item to the last breadcrumb item Fix microdata for the the last breadcrumb item Sep 12, 2019

@zero-24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

Done thanks.

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Add missing itemprop item to the last breadcrumb item

can you correct that please

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Before applying this PR the tool shows two errors
This PR fixes one of those errors

The tool still shows me the following error on a single article menu item

image

@ReLater

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

  <li itemprop="itemListElement" itemscope itemtype="https://schema.org/ListItem">
    <link
       itemprop="item" href="https://example.com/books/sciencefiction/ancillaryjustice">
      <span itemprop="name">Ancillary Justice</span></link>
    <meta itemprop="position" content="3" />
  </li>

That's valid for Google.

...but looks wrong somehow from my point of view. Couldn't find any reliable source if LINK in BODY is allowed that way.

The main problem is that G changes its criterias and things that were valid yesterday aren't valid today...

Don't forget to push #25117 ;-) and remove MicroData from Joomla.

@zero-24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

The tool still shows me the following error on a single article menu item

hmm confirmed but it seams the tool is broken there? When I just enter the breadcrumb part of the page it works good but when i paste the complete page it also shows this error here.

@franz-wohlkoenig franz-wohlkoenig changed the title Fix microdata for the the last breadcrumb item Fix microdata for the last breadcrumb item Sep 13, 2019

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

I stick by my original comment that this is something a determined user can fix for themselves in a template override.

@StefanSTS

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

The solution is to not say that the last item is a part of that breadcrumb list which it isn't, because it is not part of the way back.

Removing all the itemprop, itemscope and itemtype attributes from the $show_last part would do such a thing and it would be semantically appropriate.

At the moment the last item is an itemListElement of https://schema.org/BreadcrumbList, which is wrong, since it is not part of the way back.

So the following code would suffice:

<? php elseif ($show_last) :
	// Render last item if reqd. ?>
	<li class="active">
		<span>
			<?php echo $item->name; ?>
		</span>
	</li>
<? php endif;
@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

I just tested @StefanSTS suggestion and at least it makes google happy

@hans2103

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

according to https://developers.google.com/search/docs/data-types/breadcrumb

A breadcrumb trail on a page indicates the page's position in the site hierarchy. A user can navigate all the way up in the site hierarchy, one level at a time, by starting from the last breadcrumb in the breadcrumb trail.

Interesting is the last line stating: by starting from the last breadcrumb

If I understand the PR correctly the last part of the breadcrumb trail is excluded from the list.
Why is this? Why don't we show the last item as a link too? Even if it just is a link to the current page.

@vahidheydarpour

This comment has been minimized.

Copy link

commented Sep 21, 2019

add the top code after 53 & 68 of modules/mod_breadcrumbs/tmpl/default.php


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26268.
@vahidheydarpour

This comment has been minimized.

Copy link

commented Sep 21, 2019

@hans2103

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

just stumbled upon an edge case we have to solve as well.

https://www.oeteldonk.org/veur-carnaval/proklemaosie-van-de-peer (Dutch website)
"veur-carnaval" is a menu item with type = heading.
Menu items with this type have no URL, therefor the Google Testing Tool will fail.
See screenshot below.

Schermafdruk 2019-09-21 12 57 24

@studio42

This comment has been minimized.

Copy link

commented Sep 21, 2019

You can solve it with adding <meta itemprop="item" content="CURRENT URL"> to last item
Here a sample code

		<?php elseif ($show_last) :
			$uri = JFactory::getURI();
			$absolute_url = $uri->toString();
			// Render last item if reqd. ?>
			<li itemprop="itemListElement" itemscope itemtype="https://schema.org/ListItem" class="active">
				<span itemprop="name">
					<?php echo $item->name; ?>
				</span>
				<meta itemprop="position" content="<?php echo $key + 1; ?>">
				<meta itemprop="item" content="<?php echo $absolute_url; ?>">
			</li>
		<?php endif;

But you have another error when $item->link is empt y(in case you use separator for eg.)
I mean that in this case, the itemprop="itemListElement" should not be set, but this break the way breadcrumb is displayed by Google

@impressionestudio

This comment has been minimized.

Copy link

commented Sep 21, 2019

None of both codes about the last item worked 100% for me. Is this issue still open?

@JacquesR

This comment has been minimized.

Copy link

commented Sep 21, 2019

I also encountered this issue. While it does seem silly to show the last (the current page) item linked, that is also how the Microdata examples have done it for a very long time. It was also mentioned elsewhere that in menus we do link to the current page (just styling it differently), so in that sense not too different.

In an earlier PR for Joomla 4.0 @zwiastunsw made a reasonable argument why having the last item in the breadcrumb as a link, also make sense for accessibility.
#24113

For my own quick-fix of the Missing field "item" error, I only changed the "Render last item if reqd." section of the breadcrumb template in an override (but we should not expect most users to know how to do this.)

So that part I changed to:

// Render last item if reqd. ?> <li itemprop="itemListElement" itemscope itemtype="https://schema.org/ListItem" class="breadcrumb-item active"> <a itemtype="https://schema.org/Thing" itemprop="item" href="<?php echo JURI::getInstance()->toString(); ?>"> <span itemprop="name"><?php echo $item->name; ?></span></a> <meta itemprop="position" content="<?php echo $key + 1; ?>"> </li>

I don't know if echo JURI::getInstance()->toString(); is the best way to get the correct link, but it worked for a few different scenarios that I tested it with.

One could add an option to the breadcrumb module, to display the last item as a link or not. That would avoid changing old layouts unless they want to change.

@hans2103 I don't know how we would deal with your situation. Someone with good accessibility knowledge should perhaps comment if the practice (that I also use) of having some menu items that are not actual links (at all or just # ) will create other issues.

@n9iels

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

The information page about breadcrumbs written by Google tells us that the implementation is depending upon the search context. There is a example given where a specific book title is search for and the breadcrumbs include this page. But, they also specify that the last item should be omitted if "the page could be found in a larger set of search result". To me it is unclear what the best solution is, but it also indicates that both could be right and valid.

The current implementation is semantically wrong. I am (and probably nobody is) not quite sure about how Google processes semantically incorrect microdata, but it should be fixed for sure. Since the last item never has been a true HTML link <a> but was a sort off link with the microdata is seems logic to keep it that way. A possible semantically correct way to do this without changing the page visually could be this:

<li itemprop="itemListElement" itemscope itemtype="http://schema.org/ListItem">
    <span itemscope itemtype="http://schema.org/Thing" itemprop="item" itemid="/ul/to/page">
      <span itemprop="name">This page</span>
    </span>
    <meta itemprop="position" content="3" />
</li>

Note the itemscope on the first <span> element. This is mandatory, without the validation fails. This will also add a additional <span> element to the last page. And as last note, this PR should be ported to J4 whatever the outcome may be. The current implementation on that branch also won't pass the validation.

@zero-24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

I have just implemented the suggested changes by @n9iels thanks.

@sozzled

This comment has been minimized.

Copy link

commented Sep 22, 2019

Haven't tried the latest changes and, therefore, my comments may be inappropriate. See my comments posted at the J! forum a couple of days ago: https://forum.joomla.org/viewtopic.php?f=712&t=974526#p3581156

@sozzled

This comment has been minimized.

Copy link

commented Sep 22, 2019

Tested the last-mentioned update to the changed file. While it render's valid microdata (and passes GSC's requirements) it still does not meet my needs as I described at https://forum.joomla.org/viewtopic.php?f=712&t=974526#p3581156

Looks like GSC is being unnecessarily finnicky (?)

@n9iels

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

Tested the last-mentioned update to the changed file. While it render's valid microdata (and passes GSC's requirements) it still does not meet my needs as I described at https://forum.joomla.org/viewtopic.php?f=712&t=974526#p3581156

Looks like GSC is being unnecessarily finnicky (?)

I've tested it and it looks like that the $item->link is empty for the last item (the current active item) when opening a article that is linked from a category page has no direct menu link. On normale menu pages it all seems to work fine.

This behaviour is caused by this line, that is writen 9 years ago as far as I can tell 😅 No idea what the intention was.

$path = array(array('title' => $this->item->title, 'link' => ''));

@JacquesR

This comment has been minimized.

Copy link

commented Sep 22, 2019

$item->link did not work for me for the last item on pages that don't have a menu link. (like pages in a news category)

I don't know if echo JURI::getInstance()->toString(); is the best way to get the correct link, but it worked for a few different scenarios that I tested it with.

@n9iels Where does itemid come from? Can it be used with BreadcrumbList? Is Joomla following the Microdata breadcrumb specification from schema.org or somewhere else?

@sozzled the "issue" that you describe in the forum does not seem to be an issue? Searching for a popular term, all results are more or less the same irrespective of Joomla or another CMS or even if microdata was used or just a div called breadcrumbs. (Google shows the base URL at the start, which makes sense, since the user may want to see where they are going before clicking on the link)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.