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

Remove trailing slash from closing tag #480

Closed
wants to merge 1 commit into from

Conversation

potaito
Copy link

@potaito potaito commented Dec 29, 2022

According to https://validator.w3.org trailing slashes like these should not be used:

"Trailing slash on void elements has no effect and interacts badly with unquoted attribute values"

It's just an INFO message, but doesn't hurt to adhere to the recommendation I think. Below is an example Screenshot of the results I get when validating a page generated with jekyll and jekyll-seo-tag:

screenshot

Test

I tested the changes locally and re-uploaded the generated HTML file to the validator. The w3 validator was happy :)

According to https://validator.w3.org trailing slashes
like these should not be used:

"Trailing slash on void elements has no effect 
and interacts badly with unquoted attribute values"

It's just an INFO message, but doesn't hurt to adhere
to the recommendation I think.
@sasadangelo
Copy link

Hi potaito,
I need that this feature is included into the Jekyll Tags code. I have my website on GitHUB, what I can do to have this fix in the meanwhile it enter in the official code?
I think this fix is not disruptive and easy to include. Why hasn't it been included yet?

@potaito
Copy link
Author

potaito commented Jan 26, 2023

We can kindly ask @ashmaroli and @mattr- to take a look. They probably just didn't get around to it yet. I'm not a maintainer, so I can't do more than open a PR.

@sasadangelo
Copy link

Ok thank you!

@ashmaroli
Copy link
Member

Hello everyone,
Unfortunately, the proposed change(s) are not as benign as it seems.
HTML is lax in terms of an element's closing tag. But XHTML isn't. To my knowledge, XHTML requires an element to be explicitly closed, including void elements.

Since, removing the slash from the closing tags will be a breaking change for users with XHTML templates, I am not willing to merge this right away. I am however, open to accept this when there is significant evidence to favor the move.

@sasadangelo
Copy link

sasadangelo commented Jan 26, 2023

Hi ashmaroli,

Thanks you for the reply. You're right, this change isn't XHTML compatible. However, can you tell me an example of "breaking change for users with XHTML templates"? My understanding is that this plugin is for Jekyll that is built to create static website. Static websites are visible thorugh browsers. All browsers support HTML 5 and previous versions that, according to the W3C standard uses void elements like meta, link, img that don't expect content inside so they don't need closing tag. As far as I know latest XHTML version was 2.0 which was under development for several years but was never completed or released. This version of XHTML was eventually abandoned in favor of a new approach called HTML5.

I don't want convince anyone, I just want to understand what's wrong with my reasoning.

@ashmaroli
Copy link
Member

ashmaroli commented Jan 26, 2023

can you tell me an example of "breaking change for users with XHTML templates"? My understanding is that this plugin is for Jekyll that is built to create static website..

Say, I have a site configured with permalink: "/:categories/:title.xhtml" with the base layout with:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
  {% seo %}
</head>
<body>
{{ content }}
</body>
</html>

It wouldn't be wise for {% seo %} to render non-XHTML-compliant markup in this case, would it?

@sasadangelo
Copy link

sasadangelo commented Jan 26, 2023

Just a question: are there people out there wrinting web pages with these specification?

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">

Why not HTML 5?
However, I got your point. To be honest I don't agree too much but it is respectable. In this case, to be honest, add SEO tags like the one your plugn include it's not hard to do, people like me are forced to move to its own impementation.
Thank you for your reply.

@sasadangelo
Copy link

sasadangelo commented Jan 26, 2023

At this point, why not an option to remove the trailing slash on void elements (meta, link, img, etc.) for HTML standard?

@ashmaroli
Copy link
Member

are there people out there wrinting web pages with these specification?

I personally doubt there are any, in this day and age.
But as a maintainer, I cannot make such assumptions. What I can do is to take a stand to explicitly drop support for XHTML compatibility and release with a major version bump. However, GitHub Pages will not update their stack to preserve backwards-compatibility at their end.

@ashmaroli
Copy link
Member

why not an option to remove the trailing slash on void elements (meta, link, img, etc.) for HTML standard?

Because it is sub-optimal. The options are to either maintain two separate templates or introduce numerous {% if seo_tag.xhtml_mode %}..., both of which are not worth the effort.

@potaito
Copy link
Author

potaito commented Jan 26, 2023

Thanks for the explanation @ashmaroli . I have not considered XHTML when I proposed this.

@sasadangelo
Copy link

sasadangelo commented Jan 26, 2023

Ok thank you @ashmaroli for explanation. Your tool is generic and make sense support both the standards.

@sasadangelo
Copy link

Hi @ashmaroli,

You already explained that this fix cannot be done on your side. I agree with you. I am not a Jekyll expert, especially in Plugin write. Is there any code I can write on my side just to override your implementation for meta and links tags?

@ashmaroli
Copy link
Member

ashmaroli commented Feb 7, 2023

@sasadangelo Out-of-the-box, there is no support for a custom template.
However, there is a convoluted workaround that involves using GitHub Actions to build and deploy your Jekyll site(s):

  1. Set up GitHub Actions to build and deploy site.
  2. Replace gem github-pages with individual constituent dependencies in the Gemfile. (Create a new Gemfile via bundle init, if it doesn't exist.)
  3. Modify gem "jekyll-seo-tag" listing in Gemfile to point to this pull request (unaffected by branch-deletion):
    - gem 'jekyll-seo-tag', '...'
    + gem 'jekyll-seo-tag', github: 'jekyll/jekyll-seo-tag', ref: 'refs/pull/480/head'

Eventually, if you feel the need to make more changes to the plugin, you can fork this repository, commit all desired changes to a branch and edit the Gemfile to point to that branch instead.

@sasadangelo
Copy link

Hi ashmaroli,

Thank you for your reply. Yes this is a thing I thought. My question is if it is possible add a piece of ruby code on plugins folder just to override meta and link tags.

@ashmaroli
Copy link
Member

Unfortunately @sasadangelo, that is not an option since the change you require is in a Liquid template.

@Mohamed3nan
Copy link

Mohamed3nan commented Jul 18, 2023

@sasadangelo Out-of-the-box, there is no support for a custom template. However, there is a convoluted workaround that involves using GitHub Actions to build and deploy your Jekyll site(s):

  1. Set up GitHub Actions to build and deploy site.
  2. Replace gem github-pages with individual constituent dependencies in the Gemfile. (Create a new Gemfile via bundle init, if it doesn't exist.)
  3. Modify gem "jekyll-seo-tag" listing in Gemfile to point to this pull request (unaffected by branch-deletion):
    - gem 'jekyll-seo-tag', '...'
    + gem 'jekyll-seo-tag', github: 'jekyll/jekyll-seo-tag', ref: 'refs/pull/480/head'

Eventually, if you feel the need to make more changes to the plugin, you can fork this repository, commit all desired changes to a branch and edit the Gemfile to point to that branch instead.

This worked in my case without any conflicts
Thanks
image

@potaito potaito closed this by deleting the head repository May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants