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

Pages with meta tags, all of whose property attr are unrelated to OG properties ruins the preview #37

Closed
Clpsplug opened this issue Jan 23, 2023 · 2 comments · Fixed by #42
Closed

Pages with meta tags, all of whose property attr are unrelated to OG properties ruins the preview #37

Clpsplug opened this issue Jan 23, 2023 · 2 comments · Fixed by #42

Comments

@Clpsplug
Copy link
Contributor

The issue

I attempted to linkpreview the following URL:
https://docs.unity3d.com/Packages/com.unity.inputsystem@1.0/api/UnityEngine.InputSystem.InputSystem.html
It is a documentation page for Unity's package.

Expected behavior

This link does not appear to have any OGP properties, so I expected this plugin to produce a "no OGP image link preview snippet" with the title Class InputSystem | Input System | 1.0.2.

Actual behavior

This happened.

Image from Gyazo

  • Jekyll-linkpreview attempted to render the "link preview with OGP image snippet" (linkpreview.html) even though there are no OG properties on this page
  • The title, description, and image are missing (missing image is expected, but the other two are not.)

I looked into the cache and found that the corresponding file had this:

{"title":null,"url":null,"image":null,"description":null,"domain":"docs.unity3d.com"}

(filename: cec2702fa0dbc6125ac754817ddbb10b.json)

Suspected issues

This page has these <meta> tags in its HTML:

    <meta property="docfx:navrel" content="../toc.html">
    <meta property="docfx:tocrel" content="toc.html">
    <meta property="unity:packageTitle" content="Input System | 1.0.2">
    <meta property="docfx:rel" content="../">

So this page has <meta> tags with a property attribute, but its value is never og:.*. This edge case of an HTML page causes this method to miss this case and causes it to spawn OpenGraphPropertiesFactory when it should be spawning NonOpenGraphPropertiesFactory instead.

jekyll-linkpreview/lib/jekyll-linkpreview.rb

Lines 190 to 198 in 61c8b0b

private
def create_properties_from_page(page)
if page.meta_tags['property'].empty? then
factory = NonOpenGraphPropertiesFactory.new
else
factory = OpenGraphPropertiesFactory.new
end
factory.from_page(page)
end

(Any <meta> tag whose property attribute value matches og:.* on this page and the plugin would have avoided this issue.)

Potential fix

In addition to page.meta_tags['property'].empty?, check for the total absence of property attributes whose value matches regex og:.*. If any of those holds, we should use NonOpenGraphPropertiesFactory.
@Clpsplug
Copy link
Contributor Author

I also noticed that this specific page has quite a unique structure - the <title> tag is in the <body> tag. For this reason, the preview was missing a title even if I applied my proposed fix locally. This is because MetaInspector::Parsers::TextsParser.title only looks at the title tag within the head tag.

We cannot blame them, though; having a <title> tag outside of the <meta> tag technically violates the HTML spec.

But there is an alternative: the .best_title accessor, which also looks at the title tag within the body tag. We could also modify this part below to produce the best result possible.

jekyll-linkpreview/lib/jekyll-linkpreview.rb

Lines 117 to 121 in 61c8b0b

class NonOpenGraphPropertiesFactory
def from_page(page)
NonOpenGraphProperties.new(
page.title, page.url, get_description(page), page.host)
end

I'm going to prep a PR, but should the 'using .best_title accessor instead' fix be a separate PR from the 'non-OGP meta tags messing with the factory selection' fix?

@ysk24ok
Copy link
Owner

ysk24ok commented Jan 24, 2023

Thank you for reporting this.

In addition to page.meta_tags['property'].empty?, check for the total absence of property attributes whose value matches regex og:.*.

Right. We should check these four properties as https://ogp.me/#metadata says they are required for every page.

  • og:title
  • og:type
  • og:url
  • og:image

I'm going to prep a PR, but should the 'using .best_title accessor instead' fix be a separate PR from the 'non-OGP meta tags messing with the factory selection' fix?

I think so, and could you put #37 (comment) as a new issue so a PR can close the corresponding issue?

This was referenced Jan 24, 2023
Closed
Merged
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
2 participants
@ysk24ok @Clpsplug