Skip to content

File: Use HTML API to update the PDF preview label#60494

Merged
talldan merged 2 commits into
trunkfrom
update/file-replace-regex
Apr 5, 2024
Merged

File: Use HTML API to update the PDF preview label#60494
talldan merged 2 commits into
trunkfrom
update/file-replace-regex

Conversation

@Mamaduka
Copy link
Copy Markdown
Member

@Mamaduka Mamaduka commented Apr 5, 2024

What?

This is a follow-up to #43050 (comment).

PR updates the File block rendered to use HTML API instead of custom regex to update the PDF preview label.

Why?

HTML API is a recommended way to make similar modifications.

Testing Instructions

  1. Open a post.
  2. Insert a File block.
  3. Upload and select a PDF file.
  4. Enable "Show inline embed".
  5. Preview the post.
  6. Inspect the file block's HTML via DevTools.
  7. Confirm that the label is correct for an object tag.

Testing Instructions for Keyboard

Same.

Screenshots or screencast

CleanShot 2024-04-05 at 10 51 21

@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Block] File Affects the File Block labels Apr 5, 2024
@Mamaduka Mamaduka self-assigned this Apr 5, 2024
@Mamaduka Mamaduka requested a review from ajitbohra as a code owner April 5, 2024 06:55
@Mamaduka Mamaduka requested review from dmsnell and talldan and removed request for ajitbohra April 5, 2024 06:55
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 5, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Copy Markdown
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

This is working well for me, behaviour is the same as in trunk when I tested 👍

@talldan talldan merged commit 02071b7 into trunk Apr 5, 2024
@talldan talldan deleted the update/file-replace-regex branch April 5, 2024 09:15
@github-actions github-actions Bot added this to the Gutenberg 18.2 milestone Apr 5, 2024
@Mamaduka
Copy link
Copy Markdown
Member Author

Mamaduka commented Apr 5, 2024

Thanks for the review, Dan!

@@ -59,6 +38,19 @@ static function ( $matches ) {
$processor->next_tag( 'object' );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

generally it's nice to see official code check the response to next_tag() to ensure the tags are there. it could be the case that something changed it along the way. the writes will be benign, but the updates won't happen and we won't be the wiser.

this goes for the first tag above too.

$processor = new WP_HTML_Tag_Processor( $content );
if ( $processor->next_tag() ) {
	$processor->set_attribute( 'data-wp-interactive', 'core/file' );
}

// If OBJECT is the first element, this could pass immediately.
while ( 'OBJECT' !== $processor->get_tag() && $processor->next_tag( 'object' ) ) {
	continue;
}

$filename = null;
if ( 'OBJECT' === $processor->get_tag() ) {
	$filename = $processor->get_attribute( 'aria-label' );
}

$has_filename = is_string( $filename ) && ! empty( $filename ) && 'PDF embed' !== $filename;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @dmsnell!

Do we still want to do this when we know the $content markup?

I don't mind working on a follow-up PR; I'm unsure about current HTML API best practices.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we still want to do this when we know the $content markup?

the matter is that we don't know the $content markup. while the updates should be benign, if another plugin is installed and has changed the block output before this runs (could be on save, load, or render) then the code won't do what it thinks it's doing.

current HTML API best practices

what I hope we do in Core is set an example of checking for errors before assuming things will work. there are reasonable times to avoid checking errors, but unless we have static html, such as new WP_HTML_Tag_Processor( '<div><p>This exists</p></div>' );, then it will be helpful to check.

In any case this will also make debugging easier if we have clear exit paths for failure modes rather than continuing on as if nothing happened. the failure changes to "why was this HTML not what I expected" from "why isn't the HTML API setting these attribute?"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dmsnell, that makes sense. I will follow-up with the File block code updates.

Question: HTML API still doesn't allow text content update/replacement, correct? See: #60461 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is correct. It's actually quite easy to add in a subclass, but I would prefer we don't ship any such examples in Core.

One of the challenges in the Tag Processor is that it would be easy to assume that a #text node is a tag's inner text, when in fact it may only be one small piece of it.

If you want to explore that, let's chat in Slack on the #core-html-api channel. Again, I would urge against its use in Core before Core has its own method to replace that content.

cbravobernal pushed a commit to garridinsi/gutenberg that referenced this pull request Apr 9, 2024
* File: Use HTML API to update the PDF preview label

* Fix PHPCS errors

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] File Affects the File Block [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants