basecamp / trix

A rich text editor for everyday writing
https://trix-editor.org/
MIT License
18.97k stars 1.11k forks source link

Cannot set HTML class attribute on block element #1027

Open spyderman4g63 opened 1 year ago

spyderman4g63 commented 1 year ago

It looks like support for html class attributes should work on block elements here by passing a className option.

  if (options.className) {
    options.className.split(" ").forEach((className) => {
      element.classList.add(className)
    })
  }

However I cannot get the editor to add a class to the element:

Trix.config.blockAttributes.h2 = {
    tagName: 'h2',
    className: 'test1'
}

addEventListener("trix-initialize", function(event) {  
  var buttonHTML = '<button type="button" data-trix-attribute="h2">H2</button>'
  event.target.toolbarElement
      .querySelector(".button_group.text_tools")
      .insertAdjacentHTML("beforeend", buttonHTML)
})
Steps to Reproduce
  1. Go to JS fiddle: https://jsfiddle.net/mzsjpn8e/8/
  2. enter text into the editor
  3. highlight the text
  4. click the h2 button
  5. inspect the source code to see that the h2 element was create but that there is no class attribute
    <trix-editor input="x" contenteditable="" trix-id="1" toolbar="trix-toolbar-1">
    <h2><!--block-->test</h2>
    </trix-editor>

    I am using actionText in rails. I see that the class attribute should already be allowed:

    3.1.3 :001 > ActionText::ContentHelper.allowed_attributes
    => 
    #<Set:                                                    
    {"href",
    "src",
    "width",
    "height",
    "alt",
    "cite",
    "datetime",
    "title",
    "class",
    "name",
    "xml:lang",
    "abbr",
    "sgid",
    "content-type",
    "url",
    "filename",
    "filesize",
    "previewable",
    "presentation",
    "caption"}> 
brendon commented 1 year ago

Is there a chance you're still on Trix 1.3?

EDIT: Cancel that, I see it doesn't work on 2.x too. Just doing some digging as to why that is.

brendon commented 1 year ago

@afcapel, the lack of consideration in the code for block level elements to have classes or styles set makes a lot of seemingly simple things complicated or impossible to implement simply.

A good example is centering text. The simplest approach would be to set a class or style attribute on the block level element and have the parser recognise this as having 'centering' enabled on that block level element.

I'd be keen to look at putting together a PR to enable this but I wanted to check with you as to whether you think this will turn out to be a nightmare? Given it's been a fundamental assumption in the codebase from the start, it just might not be easily accomplished without breaking a lot of things or other assumptions. Given you're now probably intimately aquatinted with the codebase (I assume it was you that rewrote the coffee script?) I thought you might be the best person to ask :)

afcapel commented 1 year ago

You can make the block view use a custom className attribute with something like 2cb3d42e44ef7f00ac18770e1a93c8b41730bd3c.

I'm curious, though, why would you want to add a centered class to the heading block, instead of styling h1 inside your rich text container with a css selector? For example

.rich-text h1 { text-align: center }
brendon commented 1 year ago

Hi @afcapel, in my case I'm using Trix inside of a Newsletter Editor system (for schools). They value being able to choose to sometimes centre align things.

If I could allow adding an alignment class to a heading or paragraph that would be most ideal. The toolbar would be less janky too since we'd not be adding another block element around the header. I use Bulma in this particular project so I can just use its helper classes: https://bulma.io/documentation/helpers/typography-helpers/#alignment

Currently I'm wrapping items in a custom element for centre and right alignment options. Feels awful but it works.

brendon commented 1 year ago

Thinking just slightly harder about this, I can see that settings styles would be the more idea way to go as you could imagine the kind of configuration that'd be required to define exclusive things like text justification (i.e. which classes aren't allowed to co-exist on an element). Styles give a direct route to the intended outcome and in the case to textAlign can only have one of many values.

Trix doesn't have the concept of a toolbar button action that doesn't subsequently wrap the selection in another element (as far as I'm aware?) so perhaps this is still a bit of work to achieve.

brendon commented 6 months ago

I've been reminded of this thread again :) Is this something that would potentially be on the roadmap for Trix? We've had a customer wanting to align images (as in float them), which brings me back to being able to set classes or styles on elements.

afcapel commented 6 months ago

@brendon nothing in the works, that I'm aware of, but happy to accept a PR implementing the feature.

brendon commented 6 months ago

No worries @afcapel, from memory, when I looked at the codebase I got the feeling that there'd need to be quite a restructure to support classes and styles on block elements. I'll take another look in the near future :)

brendon commented 4 months ago

Just checking, did this PR fix the inability to set a class attribute on a block? https://github.com/basecamp/trix/pull/1138

From my reading it does. I'm looking forward to having a play with it :)

afcapel commented 4 months ago

@brendon yes, it should make it possible to add custom HTML attributes to blocks now. I'd be careful allowing the class attribute, because you'll have to sanitize when you paste content. But I haven't tried that myself, YMMV.

brendon commented 4 months ago

Thanks for the heads up. Does trix already do any sanitising on paste? I assume it must, so it shouldn't be too hard to hook into that?