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

Add support for embedding PDFs #6083

Closed
westonruter opened this issue Apr 21, 2021 · 3 comments · Fixed by #6112
Closed

Add support for embedding PDFs #6083

westonruter opened this issue Apr 21, 2021 · 3 comments · Fixed by #6112
Assignees
Labels
Enhancement New feature or improvement of an existing one Integration: Gutenberg P2 Low priority Sanitizers
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Apr 21, 2021

Feature description

Gutenberg 10.5 adds support for embedding PDFs in the File block: WordPress/gutenberg#30857.

It does so by adding an object element to the page:

<object
	className="wp-block-file__embed"
	data={ href }
	type="application/pdf"
	style={ {
		width: '100%',
		height: `${ previewHeight }px`,
	} }
	aria-label={ pdfEmbedLabel }
/>

We need to add an AMP_Object_Sanitizer which automatically converts object[type=application/pdf] into amp-google-document-embed.

We also need to dequeue the new wp-block-library-file script which is getting enqueued in the render_callback, so dequeuing will need to be done at the wp_print_footer_scripts action. Or otherwise, we can just unset the render_callback entirely so that the $content is just passed through as-is.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Enhancement New feature or improvement of an existing one Sanitizers Integration: Gutenberg labels Apr 21, 2021
@westonruter westonruter added this to the v2.1 milestone Apr 21, 2021
@pierlon pierlon self-assigned this Apr 21, 2021
@westonruter
Copy link
Member Author

Another implementation option, aside from a new sanitizer, would be to extend AMP_Core_Block_Handler. It could add a new entry to \AMP_Core_Block_Handler::$block_ampify_methods to override the output, and then also to eliminate the render_callback on that block to prevent the script from being enqueued.

On the other hand, adding a sanitizer would allow for any object[type=application/pdf] to be made AMP-compatible, not just those appearing in a File block.

@pierlon pierlon added the P2 Low priority label Apr 22, 2021
@pierlon
Copy link
Contributor

pierlon commented Apr 23, 2021

I've ran into a small issue with this. The amp-google-document-embed component is using the Google Docs Viewer to display the PDF. The problem is that this won't work if the PDF URL is not publicly accessible.

@westonruter
Copy link
Member Author

I think that is fine, if the problem is just that it's not working for you because the file is only on your local machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one Integration: Gutenberg P2 Low priority Sanitizers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants