Post Title: Add support for text decoration #38986

scruffian · 2022-02-22T13:53:32Z

Description

We removed support for text decoration from the Post Title block in #34064, though I'm not quite sure why. This adds back that support, but it makes a few changes to ensure that it is applied to the link, not to the wrapper, if there is a link.

We might want to get #31768 shipped as part of this change too?

Testing Instructions

Open a page template that contains a Post Title
Edit the block and confirm that you can add a "strikethrough" text decoration
Also confirm that the setting can be "none" as per Automattic/themes#5573

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
I've updated related schemas if appropriate.

packages/block-library/src/post-title/index.php

getdave · 2022-02-22T17:09:12Z

packages/block-library/src/post-title/index.php

 	if ( isset( $attributes['isLink'] ) && $attributes['isLink'] ) {
-		$title = sprintf( '<a href="%1$s" target="%2$s" rel="%3$s">%4$s</a>', get_the_permalink( $post_ID ), esc_attr( $attributes['linkTarget'] ), esc_attr( $attributes['rel'] ), $title );
+		return sprintf(
+			'<%1$s><a href="%2$s" target="%3$s" rel="%4$s" %5$s>%6$s</a></%1$s>',


Where did $tag_name come from? Why do we now have a wrapping element?

Previously this was output to a $title variable. $title was then wrapped in a wrapping element after the if statement.

This change simply returns the whole markup rather than creating a variable, which I thought was easier to read than having multiple if statements.

I appreciate this might seem like a nit pick, but in order to keep this PR focused I really think we should go back to the previous setup whereby we had a single return statement.

Why?

Having two return statements means the HTML output is effectively generated in x2 places thus increasingly the complexity of reasoning around the code. It also doubles the surface area for the introduction of XSS bugs (as evidenced by the original omission of the esc_* functions in this PR).

getdave

Appreciate your work here and patience in responding to my queries.

Nonetheless, I'm still a bit confused here. Could you help me to understand a few things?

I just want to make sure I fully understand the change and how it's being applied and what implications there are in terms of backwards compatibility.

getdave · 2022-02-23T16:35:57Z

packages/block-library/src/post-title/index.php

 	if ( isset( $attributes['isLink'] ) && $attributes['isLink'] ) {
-		$title = sprintf( '<a href="%1$s" target="%2$s" rel="%3$s">%4$s</a>', get_the_permalink( $post_ID ), esc_attr( $attributes['linkTarget'] ), esc_attr( $attributes['rel'] ), $title );
+		return sprintf(
+			'<%1$s><a href="%2$s" target="%3$s" rel="%4$s" %5$s>%6$s</a></%1$s>',


I appreciate this might seem like a nit pick, but in order to keep this PR focused I really think we should go back to the previous setup whereby we had a single return statement.

Why?

Having two return statements means the HTML output is effectively generated in x2 places thus increasingly the complexity of reasoning around the code. It also doubles the surface area for the introduction of XSS bugs (as evidenced by the original omission of the esc_* functions in this PR).

getdave · 2022-02-23T16:37:50Z

packages/block-library/src/post-title/index.php

+			get_the_permalink( $post_ID ),
+			esc_attr( $attributes['linkTarget'] ),
+			esc_attr( $attributes['rel'] ),
+			$wrapper_attributes,


We're now applying the wrapper_attributes to the <a> as opposed to the wrapping tag. What are the impacts on this on Theme styling and potential regressions?

scruffian · 2022-02-23T17:11:52Z

The aim of the PR is to be able to control the text decoration of the post title block. The block markup is dependent on whether or not the post-title has the isLink attribute set. With isLink the title gets wrapped in an additional a tag before the wrapper tag. In this case the text-decoration of the block is being overridden by the a tag which has its own text-decoration (by default all a tags will have a text-decoration of underline). Therefore to override the default text-decoration of link we need to apply the text-decoration` to the link element itself, not just to the parent wrapper.

This isn't easy to do, since the block supports just return a long string of styles and classes, so you can't apply some of them to some elements and some to others, so instead I am just applying all of the rules to the a tag. It's a heavy handed approach but we don't have the ability to have more granular control. I'm not sure if this would cause other issues - I can't think of any but that doesn't mean there aren't any!

We could avoid the double return by assigning the link to a variable and and outputting the wrapper attributes based on the isLink condition, but I think this would probably be harder to understand. Alternatively we could apply the wrapper attributes to both the wrapper element and the link.

There are a few other options we could explore if we want to more granular control, but they would take significant work:

Block supports have the concept of an __experimentalSelector which is used to determine which elements the supports apply to. The block markup isn't aware of these yet, but we could use that to determine where the wrapper attributes were applied. One problem with this is that the block doesn't always contain a link. Another problem is that this approach still doesn't allow us to apply different styles to different elements.

Another option would be to create more granular hooks for every block support mechanism, which would allow us to control which tags they are applied to. This would require a significant rework of how block supports work, see #38167.

Another option would be to always output the a tag regardless of whether the post title is a link, and just not include the href. This would make the markup more consistent for all options but would still require some reworking of the markup.

paaljoachim · 2022-02-26T10:33:03Z

I just want to say it is just a matter of time before the Post Title block becomes the go to when a user wants to add a page/post title to their layout. As in this issue: #27093 It would be natural to have all the "regular" settings as one have in a Paragraph block also in the Post Title block.

getdave · 2022-03-01T10:54:20Z

@scruffian Thanks for the context. It helps a lot. I'm going to pull this down and take a more detailed look later today.

getdave

Just noticed during testing that the text decoration UI isn't showing up in the editor

Screen.Capture.on.2022-03-01.at.11-59-44.mov

scruffian · 2022-03-01T14:53:27Z

I think that's ok for now. The aim of this PR is to get text decoration working via theme.json. We can add the UI in a later PR.

getdave · 2022-03-01T16:28:23Z

Ok that's fine 👍

Would you mind re-visiting the testing instructions in order that we can validate the PR a little?

Thanks!

scruffian · 2022-03-01T17:19:09Z

Sorry I just checked again and the controls are there, but they are hidden under the three dots:

getdave

Some things I noticed on first checkout and test.

Editor does not reflect text decoration when it's a link

Screen.Capture.on.2022-03-02.at.14-19-17.mp4

Classname placement inconsistency on front of site

When switching between making the block a link or not, the markup varies. The main issue I see is that the .wp-block-post-title moves from the <h2> to the <a>.

I imagine this is likely to cause regressions and seems heavy handed considering what we actually want to achieve is just moving the style="text-decoration:line-through;" to the <a>.

I think we should investigate using __experimentalSelector to see if we can better target the <a> element.

t-hamano · 2023-12-18T03:43:39Z

I found that this support was already added by #42328. Can this PR be closed?

scruffian · 2024-01-11T13:08:39Z

Closed by #42328

Post Title: Add support for text decoration

56924a7

scruffian requested review from getdave and creativecoder February 22, 2022 13:53

scruffian requested a review from ajitbohra as a code owner February 22, 2022 13:53

scruffian self-assigned this Feb 22, 2022

scruffian mentioned this pull request Feb 22, 2022

Archeo: Remove text decoration from post titles Automattic/themes#5573

Merged

getdave reviewed Feb 22, 2022

View reviewed changes

packages/block-library/src/post-title/index.php Outdated Show resolved Hide resolved

restore output escaping

53ffbd0

getdave reviewed Feb 22, 2022

View reviewed changes

getdave reviewed Feb 23, 2022

View reviewed changes

getdave reviewed Mar 1, 2022

View reviewed changes

getdave reviewed Mar 2, 2022

View reviewed changes

scruffian mentioned this pull request May 17, 2022

Block Supports: Tracking issue #41113

Closed

23 tasks

skorasaurus added the [Block] Post Title Affects the Post Title Block label Aug 12, 2022

scruffian closed this Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post Title: Add support for text decoration #38986

Post Title: Add support for text decoration #38986

scruffian commented Feb 22, 2022

getdave Feb 22, 2022

scruffian Feb 22, 2022

getdave Feb 23, 2022

getdave left a comment

getdave Feb 23, 2022

getdave Feb 23, 2022

scruffian commented Feb 23, 2022

paaljoachim commented Feb 26, 2022

getdave commented Mar 1, 2022

getdave left a comment

scruffian commented Mar 1, 2022

getdave commented Mar 1, 2022

scruffian commented Mar 1, 2022

getdave left a comment

t-hamano commented Dec 18, 2023

scruffian commented Jan 11, 2024

Post Title: Add support for text decoration #38986

Post Title: Add support for text decoration #38986

Conversation

scruffian commented Feb 22, 2022

Description

Testing Instructions

Screenshots

Types of changes

Checklist:

getdave Feb 22, 2022

Choose a reason for hiding this comment

scruffian Feb 22, 2022

Choose a reason for hiding this comment

getdave Feb 23, 2022

Choose a reason for hiding this comment

getdave left a comment

Choose a reason for hiding this comment

getdave Feb 23, 2022

Choose a reason for hiding this comment

getdave Feb 23, 2022

Choose a reason for hiding this comment

scruffian commented Feb 23, 2022

paaljoachim commented Feb 26, 2022

getdave commented Mar 1, 2022

getdave left a comment

Choose a reason for hiding this comment

scruffian commented Mar 1, 2022

getdave commented Mar 1, 2022

scruffian commented Mar 1, 2022

getdave left a comment

Choose a reason for hiding this comment

Editor does not reflect text decoration when it's a link

Classname placement inconsistency on front of site

t-hamano commented Dec 18, 2023

scruffian commented Jan 11, 2024