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

Added target and rel to the inlineWhiteList of the "a" element. #4522

Closed
wants to merge 1 commit into from

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jan 16, 2018

Allows target and rel to attributes of anchors to be kept during copy & paste and convert to blocks operation.
Fixes: #4498

How Has This Been Tested?

  1. Create a post in the classic editor or Gutenberg code editor with rel and target attributes in an anchor e.g:<p>test <a href="http://example.com" target="_blank" rel="nofollow noopener">link</a> test</p> ( can be copy pasted to the code editor).
  2. Open the post in the Gutenberg visual editor and use the convert to blocks option, verify in the code editor or code view that both rel and target attributes of anchor were kept.
  3. Copy an paste an HTML anchor with rel and target attributes in a paragraph block e.g: <a href="http://example.com" target="_blank" rel="nofollow noopener">link</a> verify both target and rel attributes were kept.

Allows target and rel to attributes to be kept during copy & paste and convert to blocks operation.
@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Feature] Paste labels Jan 16, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Jan 16, 2018
@ellatrix
Copy link
Member

Hm, do we really want to preserve the rel attribute if there is no way to adjust or remove it visually? In my opinion it should not be whitelisted as it is not supported by the current UI. I also don't really feel like preserving target makes sense in certain contexts like pasting from the web, but I guess it makes sense when converting old content to blocks.

@ellatrix
Copy link
Member

ellatrix commented Jan 17, 2018

In other words, I'm fine with adding target, but don't really see a good reason for us to add rel.

Edit: target UI has now been removed.

@jorgefilipecosta
Copy link
Member Author

Hi @iseulde thank you for sharing your feedback. I think most users of rel attribute are tech-savvy users performing SEO (adding rel nofollow). In the classic, without plugins, the link changer also does not allow changing rel. So most of existing rel usages were added using code changes (or plugins).
Right now in Gutenberg, we can use code view to change a link in a paragraph block and add rel attribute. I think the users that are able to use code changes to add rel in the classic editor and use code changes to add rel in Gutemberg will not expect that rel is removed when they use "convert to blocks" operation on existing posts. So if we don't allow rel attribute we may be removing SEO optimizations performed by some users without them noticing.

@ellatrix
Copy link
Member

That is true, but in this case it will also affect posting the attribute from the web, in which case the user may not be aware that they are pasting it. So the presence of the attribute does not imply tech-savviness. I'm unsure either what to do here. I don't like to see users pasting stuff from the web having this attribute all over their post without them being aware. Maybe we should have a setting { internal: true } or something for old post conversions.

@ChrisEdwardsCE
Copy link

In the push to squash the fears of Gutenberg, the ability to tell everyone that they should be able to convert their existing content into blocks is key. Converting to blocks and having everything you already added to links being stripped out makes this much harder to sell. This goes beyond just target and rel, this can be any attributes added to links.

What if there was an option when converting that allows you to preserve all link attributes? That way if you want to clear them you can?

I agree on the copy & paste side but Gutenberg should not be making decisions on a sites existing content. There are reasons content writers have added these attributes, including rel where they had to either use a custom plugin to add it or manually edit the HTML. To tell them they must go back through years of content and redo all that work after converting to blocks will lead to many upset users.

Blocks should convert existing content over as is. A user can always manually remove it by switching to HTML view.

@ellatrix
Copy link
Member

We are not automatically converting old content to blocks, so existing content will always be preserved. I do agree that we should not dump all tags and attributes on internal (definitely not external paste) conversions, though we should be careful about what to exempt. Maybe this should be part of a broader discussion about what to preserve exactly on conversion. Users could previously also add classes to any tags etc. Not exactly sure how that should be dealt with.

@danielbachhuber
Copy link
Member

We shouldn't unexpectedly lose user data in the block conversion process. If Gutenberg wants to deliberately ignore some data, then it will need to make it obvious to the end user (or prevent the conversion from happening.

@ellatrix
Copy link
Member

ellatrix commented Apr 5, 2018

Yes, let's add target and rel (and potentially many other tag + attribute combinations) to the whitelist for converting blocks, but not for paste. They can have two different whitelists. I don't think we should keep this stuff on external paste.

@danielbachhuber
Copy link
Member

I don't think we should keep this stuff on external paste.

What's TinyMCE's current behavior? If this is a change, we should make sure this is documented in #4186

@ellatrix
Copy link
Member

ellatrix commented Apr 5, 2018

Paste behaviour is very different. In the old editor there is almost no cleaning up, it's only making the tree valid (has all valid HTML as a whitelist). Paste in Gutenberg only keeps a small subset of semantic tags, attributes and known classes that are usable in the editor.

@ChrisEdwardsCE
Copy link

To throw in my 2 cents, I do believe that when pasting, attributes & classes should not be brought over.

When converting to a page to a block, styles, link attributes, etc should stay. If a user took the time to go into the html (or use a plugin) to add in these extra attributes, removing them is us dictating to them what they are allowed to have on their site and what they are not.

@ellatrix
Copy link
Member

ellatrix commented Apr 6, 2018

Sure, I share your opinion. We will add this for old content conversion. I'm not so sure about styles and classes (could be previously bad paste too, because the old editor would preserve everything).

What I'm also thinking more and more is that we should visualise anything in rich text fields that is there but not otherwise visible such as classes, attributes, non breaking spaces, etc., even if the user adds them in the HTML editor. I think it would be good to communicate that something is different, and the way to "inspect" this or change it is via the HTML editor. Maybe if you click on it you'd see [ edit | remove ] options. Would be curious to know what @jasmussen thinks, but can make a quick branch later to explain.

@jasmussen
Copy link
Contributor

One of the things I like the most about Gutenberg, is that we actually do something with what is pasted;

  • if you paste from Google Docs, Word or Pages, we identify that and clean up
  • if you paste markdown, we detect that and convert
  • if you paste an image you have on your clipboard, we insert it and upload it

If you want a "pure" paste for whatever reason, you can do that in the code editor.

What I'm also thinking more and more is that we should visualise anything in rich text fields that is there but not otherwise visible such as classes, attributes, non breaking spaces, etc., even if the user adds them in the HTML editor. I think it would be good to communicate that something is different, and the way to "inspect" this or change it is via the HTML editor. Maybe if you click on it you'd see [ edit | remove ] options.

I like the sound of this. These could be sort of inline tokens, like for example a HTML anchor, or the nonbreaking space example you mentioned is also good. I believe Dreamweaver way back in the day did this as well, and just like how the inline boundaries are there to visualize to the user what is underneath, tokenizing these things makes a lot of sense to me. If it becomes controversial, we could take further inspiration from code editors, and make it an option: "show invisible tokens". Perhaps not the highest priority to solve this, but it seems like a good path to explore.

Worth noting in that vein, that Inline Images is being explored in that vein, with inline tokens as the way for these to be inserted. See the mockups in #2043, which include @mentions as another inline token visualized.

Here's an old screenshot of how Dreamweaver used to highlight "invisible elements" as tokens:

screen shot 2018-04-09 at 09 10 39

@jorgefilipecosta
Copy link
Member Author

Thank you all for the discussion! It looks like most people agree that when copy pasted this attributes should be removed, when using covert to blocks they should be kept.

@iseulde, for me the way to implement this seems to be adding information if the context is copy past or convert to blocks to rawHandler (context = 'CONVERT_TO_BLOCKS'). Pass this information down to functions used by rawHandler that that need the information. Create a new object phrasingContentSchemaConvertToBlocks that contains the trusted attributes for covert to blocks. Do you see an alternative to this approach?

@danielbachhuber
Copy link
Member

@jorgefilipecosta @WordPress/gutenberg-core Did we achieve some consensus on how to handle HTML attributes that are a part of the HTML spec but don't (currently, at least) have UI in Gutenberg?

@afercia brought up more examples in #4498 (comment)

@jorgefilipecosta
Copy link
Member Author

Hi @danielbachhuber, In my option as referred some of this attributes should be kept like aria-label attributes, anchors, and target even if we don't have a UI to change them. Previously for some of this attributes, no UI existed and these attributes were added anyway. But I'm not certain if there this opinion is consensual.
We have some PR's going in this direction #8125 and #8124, given that they are already approved I'm going to close this one.

@jorgefilipecosta jorgefilipecosta deleted the fix/accepted-attributes-anchor branch July 24, 2018 17:59
@danielbachhuber
Copy link
Member

👍 Thanks @jorgefilipecosta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Paste [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants