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

Allow the target attribute on <a> tags. #8125

Merged
merged 2 commits into from Jul 25, 2018
Merged

Conversation

pento
Copy link
Member

@pento pento commented Jul 23, 2018

Description

Sine we have UI for adding target="_blank" to <a> elements, we should allow it to stay when converting existing posts to blocks.

This PR partially address #4498.

How has this been tested?

Create a post in the Classic Editor with this content:

<a href="https://wordpress.org" target="_blank">Hi!</a>

Open the post in Gutenberg, and convert it to blocks.

Click on the link, check that the "Open in new window" setting is enabled.

Preview the post, check that clicking the link opens it in a new window.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@pento pento added the [Feature] Raw Handling Related to the ability to convert content to blocks, paste handling, etc label Jul 23, 2018
@pento pento added this to the Try Callout milestone Jul 23, 2018
@pento pento self-assigned this Jul 23, 2018
@pento pento requested a review from a team July 23, 2018 06:57
@gziolo gziolo requested review from ellatrix and mcsf July 23, 2018 07:19
@@ -18,7 +18,7 @@ const phrasingContentSchema = {
em: {},
del: {},
ins: {},
a: { attributes: [ 'href' ] },
a: { attributes: [ 'href', 'target' ] },
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have the UI to add other kinds of targets (aside _blank). We also add rel="noreferrer noopener" to these links. The question is: Should we allow only target="_blank" and normalize the "rel" attribute in that case? or it's fine to just pick the target and keep the link as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Those are good questions. Perhaps we should perform some normalization, as there is an expectation — given the precedents when pasting from word processors, etc. — that Gutenberg generally cleans up markup. I was looking at the raw-handling pipeline and am guessing that an acceptable place to normalize target and rel would be phrasing-content-reducer, next to where special rules are applied to b and i tags:

} else if ( node.nodeName === 'B' ) {
node = replaceTag( node, 'strong', doc );
} else if ( node.nodeName === 'I' ) {
node = replaceTag( node, 'em', doc );
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, @mcsf, I added normalisation here.

@afercia
Copy link
Contributor

afercia commented Jul 23, 2018

Well as an accessibility person I'd very happy to see these links have also a visually hidden text (opens in a new window) together with rel="noopener noreferrer". However, I'd suggest to consider this is about the users content and WordPress shouldn't modify users content without their consent.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pento pento merged commit 8d14697 into master Jul 25, 2018
@pento pento deleted the fix/4498-convert-anchor-target branch July 25, 2018 00:44
@pento pento modified the milestones: Try Callout, 3.4 Jul 25, 2018
@ellatrix
Copy link
Member

I would have liked to see this filter only on block conversion, not on paste. I'm not sure if the user intends these link to open in a new window if the source does.

@pento
Copy link
Member Author

pento commented Jul 25, 2018

It might be a good idea to add some styling to links that are set to open in a new window, so the author can easily see which links are set to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Raw Handling Related to the ability to convert content to blocks, paste handling, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants