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

Don't escape markup sensitive characters in <title> #5788

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

majakomel
Copy link
Contributor

@majakomel majakomel commented Jan 28, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Avoids broken titles like The DEV &lt;&gt; Stackbit integration is in the running for a Product Hunt "Golden Kitty Award" by replacing .innerHTML with .textContent which gets a raw copy of text node content.

❗️This bug only appears when articles are not accessed via internal navigation.

Related Tickets & Documents

Closes #5226

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

html_escape

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 28, 2020
@nickytonline
Copy link
Contributor

Ahh, just realized TITLE vs title yields different results. But regardless, why are there two <title /> tags for the document?

@@ -10,7 +10,7 @@
<% if @shell %>
<script defer>
if (!document.title) {
document.title = document.getElementsByTagName("TITLE")[1].innerHTML
document.title = document.getElementsByTagName("TITLE")[1].textContent
Copy link
Contributor

Choose a reason for hiding this comment

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

This would fix the issue, but I'm not sure why this piece of code exists. I did a document.getElementsByTagName('title') from the home page and an article page and both currently yield 5 <title /> tags.

The first one is the document's title and the other 4 are titles for <svg /> elements. e.g.

image

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little late to this but I don't think this is no longer an issue @nickytonline

@rhymes
Copy link
Contributor

rhymes commented Feb 22, 2020

@nickytonline is still an issue in your opinion? The article that prompted this issue related to this PR seems fine: https://dev.to/astrit/why-how-did-i-build-500-css-only-icons-library-a-life-story-34on

@Defman21
Copy link
Contributor

Defman21 commented Feb 22, 2020

@rhymes it is.

image

image

@rhymes
Copy link
Contributor

rhymes commented Feb 22, 2020

@Defman21 my mistake! For some reason I thought the issue was about the title we display, not the HTML <title> tag :) Yep, still an issue :)

@maestromac
Copy link
Contributor

Hmm, this could be a browser-specific issue. Of course we still need to address it

image

Copy link
Contributor

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Tested locally. LGTM!

Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@Zhao-Andy Zhao-Andy merged commit 0ee7b2f into forem:master Mar 11, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Mar 11, 2020
@majakomel majakomel deleted the title-escape branch May 3, 2020 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not escape characters for <title>
6 participants