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

Trix no longer adds dataset attributes on inserted attachments in 2.1.4 #1178

Open
J-Michalek opened this issue Aug 7, 2024 · 9 comments · May be fixed by #1183
Open

Trix no longer adds dataset attributes on inserted attachments in 2.1.4 #1178

J-Michalek opened this issue Aug 7, 2024 · 9 comments · May be fixed by #1183

Comments

@J-Michalek
Copy link

J-Michalek commented Aug 7, 2024

Describe the bug or issue here…

Steps to Reproduce
  1. Go to https://trix-editor.org/
  2. Delete all of the content
  3. Paste the following code into the console and run it
(() => {
    const trix = document.querySelector("trix-editor");
    const attachment = new Trix.Attachment({
    "attachment-type": "custom",
    "content": "<div data-item-id='123'>Content</div>",
    "contentType": "custom"
    });

    trix.editor.insertAttachment(attachment)
})()
  1. Inspect the attachment within Trix and see that the div element is missing the data-item-id attribute

This was working fine prior to version 2.1.4.

Details
  • Trix version: 2.1.4
  • Browser name and version: Chrome 127.0.6533.89 (Official Build) (arm64)
  • Operating system: MacOS
@J-Michalek J-Michalek changed the title Trix stopped adding dataset attributes on inserted attachments in 2.1.4 Trix no longer adds dataset attributes on inserted attachments in 2.1.4 Aug 7, 2024
@OtherCroissant
Copy link

OtherCroissant commented Aug 22, 2024

I have the same issue, Seems like it has to do with the recent changes of always applying the HTMLSanitizer, so we need to add the data-item-id to the allowedAttibutes of the HTMLSanitizer somehow. But it looks like the HTMLSanitizer.setHTML() constructor is used to sanitize things, which does not pass a allowedAttributes list around and therefore only the default list HTMLSanitizer.DEFAULT_ALLOWED_ATTRIBUTES is used. Which is a constant and we cannot override?

In my case, I need a data-element-id on a link attachment. I am trying this, but until now no luck:

const defaultHrefParser = Trix.config.textAttributes.href.parser;

Trix.config.textAttributes.href = {
  ...Trix.config.textAttributes.href,
  parser: (element) => {
    element.allowedAttributes = [...element.allowedAttributes, "data-element-id"];
    defaultHrefParser(element);
  }
};

The parser element looks like it has the correct list of allowedAttributes now? (data-element-id is in there). But it does not let it through...

@Betree
Copy link

Betree commented Sep 2, 2024

Same issue but with the nodes themselves: since Trix 2.1.4, embedding YouTube/Vimeo videos in the editor no longer works because they're using iframes, and as far as I can tell they're no way to customize the sanitizer's behavior.

@Betree Betree linked a pull request Sep 2, 2024 that will close this issue
@Betree
Copy link

Betree commented Sep 2, 2024

I forked the repo and proposed a fix that moves the sanitizer's options to config. With that PR, I'm able to allow video embedding by allowing them explicitly:

remove(this.Trix.config.parser.forbiddenElements, type => type === 'iframe'); // Allow iframes for video embeds
this.Trix.config.parser.allowedAttributes.push('frameborder', 'allowfullscreen');

@OtherCroissant
Copy link

Is there anyone from Basecamp willing to review this?

@gabrielmiller
Copy link

2.1.11 made dompurify configurable. That went through review here.

With those changes I have been able to add arbitrary data-trix- attributes, fulfilling a similar need to what's noted here. It looks like that also allows for adding your own allowed attributes, but I didn't test that part.

@Betree
Copy link

Betree commented Dec 20, 2024

I tried to move back to this latest release, but even after allowing iframe with:

this.Trix.config.dompurify.ADD_TAGS = ['iframe'];

iframes are still being removed from the content. I don't have more time to investigate this for now, so we'll stick with our fork. I'm interested in the result if anyone can make it work!

@jondkinney
Copy link

jondkinney commented Jan 14, 2025

I also have what I think to be a valid Trix.config setup for 2.1.12

document.addEventListener("trix-before-initialize", function () {
  Trix.config.dompurify.ADD_ATTR = ["language", "data-date-tag"];
  Trix.config.dompurify.ADD_TAGS = ["span"];
});

Interestingly if I don't include language in the array for ADD_ATTR, then whatever I put there seems to replace the original config that did have language specified, which doesn't seem ideal.

Here's what it looks like without any config:
CleanShot 2025-01-14 at 16 12 05@2x

And here's what it looks like with the config from above:
CleanShot 2025-01-14 at 16 20 35@2x

However, I'm unable to retain the data-date-tag in my attachment, it's being stripped out.

As a workaround, I'm then grabbing the figure element and using getAttribute to retrieve the attachment, parsing it as JSON and then grabbing its content as a string and adding it to a temp dom node so I can read the data-attrs out.

const dateAttachment = JSON.parse(
  dateTagBtn.closest("figure").getAttribute("data-trix-attachment"),
).content;

// Create a temporary div element
let tempDiv = document.createElement("div");

// Set the innerHTML to your string
tempDiv.innerHTML = dateAttachment;

// Get the span element
const spanElement = tempDiv.querySelector(".date-tag-button");

const taskName = spanElement.getAttribute("data-task-name");
const dateTag = spanElement.getAttribute("data-date-tag");

But this is definitely a hack when the sanitization is advertised as configurable...

Also, in order to get the data-task-name and data-date-tag attributes to save to the database to be read out, I had to add the following in config/application.rb inside the Application class.

ActionText::Attachment::ATTRIBUTES << "data-date-tag"
ActionText::Attachment::ATTRIBUTES << "data-task-name"

@OtherCroissant
Copy link

@jondkinney It looks like it now only allows allowlisted data attributes that are prefixed with trix. So in your case:

document.addEventListener("trix-before-initialize", function () {
  Trix.config.dompurify.ADD_ATTR = ["language", "data-trix-date-tag"];
  Trix.config.dompurify.ADD_TAGS = ["span"];
});

Then the attribute is not sanitized away. See:

const allowedAttributePattern = /^data-trix-/

I don't understand this requirement, but well...

@jondkinney
Copy link

@OtherCroissant thanks so much for the response! I definitely missed that change/requirement of trix bit in the data-trix- prefix. Looking back @gabrielmiller does mention it

With those changes I have been able to add arbitrary data-trix- attributes, fulfilling a similar need to what's noted here.

Glad I will be able to remove my hacky workaround of reading the content attachment data-attributes from the figure, itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants