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.

Sign up for GitHub

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

Jump to bottom

Social viewlet generates invalid HTML #1087

Closed
jladage opened this issue Sep 25, 2015 · 11 comments · Fixed by plone/plone.app.layout#78
Closed

Social viewlet generates invalid HTML #1087

jladage opened this issue Sep 25, 2015 · 11 comments · Fixed by plone/plone.app.layout#78

Comments

@jladage
Copy link

jladage commented Sep 25, 2015

plone.app.layout's social viewlet defines <meta itemprop="image" ...> in the head tag, but it lacks a <span itemscope itemtype="Article"> .. </span>

There are two ways to solve this:

  1. Add itemscope itemtype="Article" as attribute to the <html> tag
  2. Add a <span itemscope itemtype="Article"> .. </span> around the metatags with itemprop attributes

Those are equal so please choose the one that is easiest to implement ;)
@jladage
Copy link
Author

jladage commented Sep 25, 2015

ok, so html gets filtered here :)

The meta tags with itemprop attributes miss a surrounding span with itemscope attribute.

@gforcada
Copy link
Member

@jladage you need to wrap html-like tags with backticks

@mauritsvanrees
Copy link
Member

When I add a span in the head, the w3c validator starts giving this error: "Stray end tag head."

So the first way is probably best. Except that itemtype="Article" should be a url, and WebPage is probably better: itemtype="http://schema.org/WebPage".

This means that it should be changed in the main template, not in plone.app.layout.

mauritsvanrees added a commit that referenced this issue Feb 1, 2016
@mauritsvanrees
Fixed validation error for social media viewlet.
27bb1d9 
Added `itemscope itemtype="http://schema.org/WebPage"` to main template html tag.

Fixes #1087
@mauritsvanrees mauritsvanrees mentioned this issue Feb 1, 2016
Merged
@hvelarde
Copy link
Member

hvelarde commented Feb 1, 2016

-1 and that was one of the reasons I was against of adding this feature to Plone 5 on the first place as discussed in https://dev.plone.org/ticket/20256

first, I don't know why we are adding schema.org markup in the header, that's not documented anywhere; second, we must be careful with this markup as it has to be very easy to override it for add-ons.

take, for instance, collective.nitf (a content type that describes a news article); we spend a lot of time adding semantic markup to it and now I think we are going to invalidate it with this change.

so, IMO, the solution here is to remove those tags; neither Facebook's Open Graph, nor Twitter Cards, require that.

@hvelarde
Copy link
Member

hvelarde commented Feb 1, 2016

@jensens did you read above before merging this?

@jensens
Copy link
Member

jensens commented Feb 1, 2016

well, no. mea culpa. reverted. this one needs discussion. #1355

since i'am not an expert on this, what about a less immersive way than hard-coded in main template?

@jensens jensens reopened this Feb 1, 2016
@hvelarde
Copy link
Member

hvelarde commented Feb 1, 2016

thank you!

@mauritsvanrees
Copy link
Member

From what I see, the Facebook og: tags must be in the head. I guess the Twitter tags too. See examples on https://developers.facebook.com/docs/opengraph/getting-started

The itemScope and itemType belonging to schema.org with their tags can be anywhere on the page.

So it would seem the solution is:

  • Keep the Facebook and Twitter tags in the current viewlet in the head.
  • Move all schema stuff from the head to a second viewlet above the content.

Does that sound good?

@hvelarde Is that maybe what you meant with your first comment? At the time, it sounded to me like you wanted to rip all schema stuff out, including what the viewlet in current Plone 5.0 does.

@mauritsvanrees mauritsvanrees mentioned this issue Feb 15, 2016
Closed
@hvelarde
Copy link
Member

@mauritsvanrees as I mentioned, including something like Twitter Cards in Plone's core was a mistake as we don't even know if Twitter is going to collapse next month or when the current Tech bubble finally burst.

we have 2 different things here: social media markup and semantic markup; so, let's focus on the social media markup first, trying to support only open standards; semantic markup must be addressed on a different issue and then we can discuss where to implement it.

besides that, Plone must evolve to support Linked Open Data.

@jensens
Copy link
Member

jensens commented Feb 15, 2016

+1 for any PLIP (inc.implementation) that brings Linked Open Data to Plone :D

@hvelarde
Copy link
Member

I would love to do that, unfortunately is out of my scope and I have no time/resources to do so.

I'm just proposing something that, IMO, is the way to go.

mauritsvanrees added a commit to plone/plone.app.layout that referenced this issue Feb 29, 2016
@mauritsvanrees
Fixed invalid html of social viewlet.
3a4f532 
Moved the schema.org tags to the body in a new viewlet
plone.abovecontenttitle.socialtags and added itemScope and itemType
there.

Fixes plone/Products.CMFPlone#1087
@mauritsvanrees mauritsvanrees mentioned this issue Feb 29, 2016
Merged
mauritsvanrees added a commit to plone/plone.app.layout that referenced this issue Mar 1, 2016
@mauritsvanrees
Fixed invalid html of social viewlet.
57b8c1b 
Moved the schema.org tags to the body in a new viewlet
plone.abovecontenttitle.socialtags and added itemScope and itemType
there.

Fixes plone/Products.CMFPlone#1087
jensens pushed a commit to plone/plone.app.layout that referenced this issue Mar 11, 2016
@mauritsvanrees @jensens
Fixed invalid html of social viewlet.
e96a056 
Moved the schema.org tags to the body in a new viewlet
plone.abovecontenttitle.socialtags and added itemScope and itemType
there.

Fixes plone/Products.CMFPlone#1087
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Mar 11, 2016
@jensens
[fc] Repository: plone.app.layout
5ae4bc2 
Branch: refs/heads/master
Date: 2016-03-11T13:55:42+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.layout@e96a056

Fixed invalid html of social viewlet.

Moved the schema.org tags to the body in a new viewlet
plone.abovecontenttitle.socialtags and added itemScope and itemType
there.

Fixes plone/Products.CMFPlone#1087

Files changed:
A plone/app/layout/viewlets/social_tags_body.pt
M CHANGES.rst
M plone/app/layout/viewlets/configure.zcml
M plone/app/layout/viewlets/social.py
M plone/app/layout/viewlets/tests/test_social.py
Repository: plone.app.layout
Branch: refs/heads/master
Date: 2016-03-11T13:56:11+01:00
Author: Jens W. Klein (jensens) <jk@kleinundpartner.at>
Commit: plone/plone.app.layout@73d588e

Merge pull request #78 from plone/maurits-issue-1087

Fixed invalid html of social viewlet

Files changed:
A plone/app/layout/viewlets/social_tags_body.pt
M CHANGES.rst
M plone/app/layout/viewlets/configure.zcml
M plone/app/layout/viewlets/social.py
M plone/app/layout/viewlets/tests/test_social.py
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Mar 11, 2016
@jensens
[fc] Repository: plone.app.layout
da02ffe 
Branch: refs/heads/master
Date: 2016-03-11T13:55:42+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.layout@e96a056

Fixed invalid html of social viewlet.

Moved the schema.org tags to the body in a new viewlet
plone.abovecontenttitle.socialtags and added itemScope and itemType
there.

Fixes plone/Products.CMFPlone#1087

Files changed:
A plone/app/layout/viewlets/social_tags_body.pt
M CHANGES.rst
M plone/app/layout/viewlets/configure.zcml
M plone/app/layout/viewlets/social.py
M plone/app/layout/viewlets/tests/test_social.py
Repository: plone.app.layout
Branch: refs/heads/master
Date: 2016-03-11T13:56:11+01:00
Author: Jens W. Klein (jensens) <jk@kleinundpartner.at>
Commit: plone/plone.app.layout@73d588e

Merge pull request #78 from plone/maurits-issue-1087

Fixed invalid html of social viewlet

Files changed:
A plone/app/layout/viewlets/social_tags_body.pt
M CHANGES.rst
M plone/app/layout/viewlets/configure.zcml
M plone/app/layout/viewlets/social.py
M plone/app/layout/viewlets/tests/test_social.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixed invalid html of social viewlet plone/plone.app.layout
5 participants
@jensens @mauritsvanrees @hvelarde @jladage @gforcada