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

Video sitemap description: change the XSL Stylesheet to display the HTML #13261

Merged
merged 2 commits into from
Aug 27, 2019

Conversation

htdat
Copy link
Member

@htdat htdat commented Aug 19, 2019

Fixes #9546

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This is just to enhance the way we display video sitemap descriptions to humans.

Testing instructions:

  • Upload a new video to Media Libary
  • Add the description of the video, which includes HTML tags like <strong>Description</strong> with <a href="https://github.com/Automattic/jetpack/issues/9546">HTML tags</a>.
  • Check https://domain.com/video-sitemap-1.xml
  • Make sure the description of this video is displaying correctly without HTML-escaped tags: Description with HTML tags.

Proposed changelog entry for your changes:

  • Sitemaps: Improve the display of video sitemap description with HTML tags.

@htdat htdat requested a review from a team August 19, 2019 15:18
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello htdat! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D31677-code before merging this PR. Thank you!

@htdat htdat added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Aug 19, 2019
@jetpackbot
Copy link

jetpackbot commented Aug 19, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against 8fa1d91

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Sitemaps labels Aug 21, 2019
@jeherve jeherve added this to the 7.7 milestone Aug 21, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 21, 2019
jeherve
jeherve previously approved these changes Aug 21, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work well in my tests. 👍

@mdawaffe
Copy link
Member

What happens when there's "weird" HTML in the description. Images, for example? Does the output look ugly to humans?

Should we run the description through kses?

@htdat
Copy link
Member Author

htdat commented Aug 22, 2019

What happens when there's "weird" HTML in the description. Images, for example? Does the output look ugly to humans?

Yeah, interesting case, which I have not thought of yet! I've tried all possible tags when editing a media file.
It's not as nice as without images tags. However, I think the result is acceptable and not too ugly for humans. See below.

Should we run the description through kses?

We can do that but we may lose the visibility of other HTML tags to search engines.

I would vote not doing this :D

My detailed test

Description HTML code

<strong>Content</strong> with <a href="https://github.com/Automattic/jetpack/issues/9546">HTML tags</a>.

<img src="https://videos.files.wordpress.com/K3mDX6U2/testing_reuter_hd.original.jpg" alt="img descriptions" />

<code>php-code</code>

<blockquote>quote line 01
quote line 02</blockquote>

<ul><li>Bullet 01</li><li>Bullet 02</li><li>Bullet 03<br><br></li></ul>

This is the result in 100% browser size:

Screen Shot on 2019-08-22 at 16:27:23

This is the result in 75% browser size:

Screen Shot on 2019-08-22 at 16:28:06

@matticbot
Copy link
Contributor

htdat, Your synced wpcom patch D31677-code has been updated.

@mdawaffe
Copy link
Member

I tweaked the stylesheet in 8fa1d91 to limit the size of images.

Screen Shot 2019-08-26 at 3 34 47 PM

I'm fine merging this. Let's be clear with the user reporting the issue, though, that the HTML tags (including the wrapping <p> tags) are normal and expected: search engines will do the right things with them, and the sitemap XML files are not meant to be human readable (though we try to make them look nice all the same).

Firefox, for example, ignores the disable-output-escaping attribute in the XSLT, so this PR does not change the human view of the sitemap for humans using Firefox.

@jeherve jeherve merged commit 15aaa42 into master Aug 27, 2019
@jeherve jeherve deleted the enhance/9546 branch August 27, 2019 05:53
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 27, 2019
jeherve added a commit that referenced this pull request Aug 27, 2019
jeherve added a commit that referenced this pull request Aug 27, 2019
* 7.7 changelog: initial set of changes

* Changelog: add #13154

* Changelog: add #13134

* Changelog: add #12699 and many others

* Changelog: add #13127

* Changelog: add #13167

* Changelog: add #13225

* Changelog: add #13179

* Changelog: add #13173

* Changelog: add #13232

* Changelog: add #13137

* Changelog: add #13172

* Changelog: add #13182

* Changelog: add #13200

* Changelog: add #13244

* Changelog: add #13267

* Changelog: add #13204

* changelog: add #13205

* Changelog: add #13183

* Changelog: add #13278

* Changelog: add #13162

* Changelog: add #13268

* Changelog: add #13286

* Changelog: add #13273

* Changelog: add #12474

* Changelog: add #13085

* Changelog: add #13266

* Changelog: add #13306

* Changelog: add #13311

* Changelog: add #13302

* Changelog: add #13196

* Changelog: add #12733

* Changelog: add #13261

* Changelog: add #13322

* Changelog: add #13333

* Changelog: add #13335
@kraftbj
Copy link
Contributor

kraftbj commented Apr 29, 2020

r206613-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Sitemaps Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Video Sitemap: description should filter out HTML tags
6 participants