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 ID attributes on any elements #8124

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Conversation

pento
Copy link
Member

@pento pento commented Jul 23, 2018

Description

The ID attribute is frequently used to create intra-page links, or to link to specific parts of a page.

Removing them when we convert existing posts to blocks causes those links to suddenly start failing.

It's worth noting that this PR intentionally does not create UI for editing the id attribute. The workflow for adding an id attribute in Classic involves switching to Text view, the workflow for Gutenberg currently remains the same. This PR is solely to address the data loss that can occur when a post is converted.

How has this been tested?

Create a post in the Classic Editor with this content:

<h2 id="foo">Bar</h2>

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

Check the Code view, to see that the id attribute is still set correctly.

Preview the post, and add the #foo anchor to the URL, and check that the page jumps to the heading.

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 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:28
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This change looks good and I agree with the reasoning behind it.

I would like to make sure @aduth or @iseulde are okey with this change.

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.

We should definitely avoid dropping the id for blocks supporting anchor like the heading but it's pointless to keep it for the other blocks because It will be dropped anyway by the serialization. An alternative would be to support anchor for blocks by default (like we do for custom classNames) but I think we decided against when we originally introduced the anchor. The argument is, the anchor doesn't make much sense for most blocks and is more important for headings (false by default).

@aduth
Copy link
Member

aduth commented Jul 23, 2018

it's pointless to keep it for the other blocks because It will be dropped anyway by the serialization.

This is an important note, and maybe also concerning that our tests don't verify that serialization won't cause the attributes to be dropped anyways.

@pento
Copy link
Member Author

pento commented Jul 24, 2018

Thanks for the feedback! I changed it only allow the id attribute on blocks that support anchor.

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 👍

const input = '<p id="foo">test</p>';
const output = '<p>test</p>';
expect( removeInvalidHTML( input, schema ) ).toBe( output );
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a unit tests for headings (where we keep the attribute?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not without moving things around, or copy/pasting code. Testing that functionality requires access to getRawTransformations(), which currently isn't exported.

@youknowriad
Copy link
Contributor

One question that crosses my mind here is that the anchor support is added by a hook, so I was wondering if it's possible to extend the raw handler by a plugin as well to add this logic of keeping the id? Or should we add a filter/hook to the raw handler to allow this kind of tweaks?

@pento
Copy link
Member Author

pento commented Jul 25, 2018

If a plugin's block declares that it supports anchor, then the HTML that its raw transform generates should get to keep the id attribute.

There's probably going to be a need to make this pluggable, particularly for sites that generate weird HTML that they want to keep.

@noisysocks
Copy link
Member

Fixes #7335.

@pento pento merged commit 7e223fc into master Jul 25, 2018
@pento pento deleted the fix/7335-allow-id-attributes branch July 25, 2018 04:53
@pento pento modified the milestones: Try Callout, 3.4 Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants