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

Posts: Fix detectImage post normalizer when there's an iframe with an empty src #11422

Merged
merged 1 commit into from Feb 17, 2017

Conversation

youknowriad
Copy link
Contributor

closes #11411

The rule was failing silently on Chrome and was stoping the posts loading on Safari.

Testing instructions

  • Create a post containing a getty embed ( [getty src="539147864" width="594" height="441"] )
  • Navigate to /posts using Safari
  • The posts list should load properly

… empty src

The rule was failing silently on Chrome and was stoping the posts loading on Safari
@youknowriad youknowriad added Posts [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Bug labels Feb 16, 2017
@youknowriad youknowriad self-assigned this Feb 16, 2017
@matticbot
Copy link
Contributor

Copy link
Contributor

@kriskarkoski kriskarkoski left a comment

Choose a reason for hiding this comment

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

I've confirmed this allows the posts list to load when it includes a Getty embed.

Is it expected that the embed does not render as the expected image or show the shortcode?

@youknowriad
Copy link
Contributor Author

I can confirm that it's not the only embed not rendered on the post list (Getty is not even among the whitelisted iframe lists). Maybe more background from @timmyc?

@timmyc
Copy link
Contributor

timmyc commented Feb 17, 2017

I don't really know the background on these normalizer rules, /cc @samouri who was the last one to work on this logic.

The changes here seem reasonable to me though.

@samouri
Copy link
Contributor

samouri commented Feb 17, 2017

Interesting. This wasn't broken for me on Chrome but I was able to repro in Safari. The fix worked and the code LGTM. 🚢

I do wonder why this is happening though. On Safari I was also seeing a second error that seemed related to Getty javascript within the iframe: . The error disappeared with this fix so I'm thinking they could be related. My gut says it has something to do with the iframe sandboxing

@samouri samouri added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 17, 2017
@youknowriad youknowriad merged commit 60c56e1 into master Feb 17, 2017
@youknowriad youknowriad deleted the fix/getty-embed branch February 17, 2017 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Posts: Getty embed prevents posts list from loading in Safari
5 participants