Use p tag instead of div for regular block content

[Truffula]

Currently regular blocks are a <div> with newlines added as a <br> (unless you convert a line to another type and back). It would allow more flexible styling (and arguably be more semantic) to default to a <p> tag with enter adding a new tag, and shift+enter adding a <br>. This could be added as an opt-in for backwards compatibility.

[ktkaushik]

see https://github.com/basecamp/trix/issues/117

[javan]

I don't think <p> will ever be the default block element in Trix. It's a popular request though and I would like to allow them via configuration more easily. Currently, you can set Trix.config.blockAttributes.default.tagName = "p" in your app, which works fine, but doesn't create a new <p> when you press return. https://github.com/basecamp/trix/pull/187 addresses that as does this (now stale) branch I started.

One potential issue with using <p> is Trix's image attachments, which are rendered in a <figure> element. <p> only permits phrasing content so placing a <figure> in one would be invalid.

For what it's worth, every word processing program (by default, at least) I've tried has the same newline behavior as Trix. Press return, cursor moves to the next line.

[ktkaushik]

It would actually be quite handy when one has to sit down and play around with Nodes in the editor upon manipulation.

Even if one is not doing so, i would still like to keep the elements in a p tag.

[Truffula]

Thanks for the quick response! Do images need to be allowed inline, or could they be added in their own block?

As far as word processors go, I think if you turn on hidden characters, when you press return you get a paragraph mark (equivalent of </p><p>) — if your formatting has space after/before paragraphs you'll get a larger break than the normal line spacing — whereas if you press shift+return you get a newline (equivalent of <br />).

Is #187 likely to be merged soon?

In any case, keep up the great work, and thanks for an awesome library! :)

[athelas64]

I have to agree with @Truffula that semantically, Enter is a new Paragraph, while Shift+Enter is a Linebreak - and that is what br tag stands for. Copy pasting text from Basecamp to Word clearly shows the mess. I don't think Images are allowed inline presently anyway, in Basecamp they certainly do not behave like that. So it should not be difficult to simply close the <p> tag and open <figure> whenever user inserts an image. This change would make Trix also more accessible for screenreaders (a blind person could navigate - jump through paragraphs, which is currently impossible).

As a teacher and using Basecamp for classroom, I need to be considerate of visually impaired students, and while presently I do not have any, I am part of the research team trying to bridge the gap in educational tools and accessibility of information for impaired students.

[gregblass]

@javan Except WordPress :(

[alexeckermann]

I'm trailing trix to use in our application for authors to write books. We'd ideally like to use paragraph elements as the primary wrapper for text content. There may be another way than changing the default block element — we'd like to keep div's as they are as a kind of section break.

Ideally, a structure like this:

div
-- p
-- figure
-- p
div
-- p

What I'm thinking, as a rough idea, is that PieceView#createStringNodes appends <p> nodes instead of just text nodes. This should not have the same problem as setting the default block element to <p> and conflict with <figure> elements as its handled separately by PieceView.

I'm going to dive deeper on this to assess what all the moving parts are before starting a fork with that approach. Is there any initial advice for those of you with deeper understanding — known potential conflicts or issues?

[alexeckermann]

@javan since this will be a requirement for my particular implementation of trix, and I suspect may be beneficial for others, is there any guidance on how we could best go about this?

My initial approach wouldn't have been all that successful. After more thought it comes down to how Trix.BlockView#createNodes renders out to a Trix.TextView that then collates nodes for pieces. For paragraphs to work, and to keep pieces as they are, an intermediary step to split pieces by \n\n to form paragraphs (retaining the pieces attributes) and \n for <br /> is roughly the new approach I am considering. However, I am unsure how well this will go down with the HTMLParser.

Something roughly flowing like: BlockView#createNodes -> ParagraphGroupView? -> TextView -> PieceView

But in this approach I would like to respect that the option to use paragraphs is optional. So thats potentially another added detail.

I'll give it a go in a fork branch. One way of learning more about trix's ins, outs and what-have-yous :)

[javan]

Here's a small potential change that would allow configuring the default block to use <p> more naturally:

diff --git a/src/trix/models/block.coffee b/src/trix/models/block.coffee
index f426bc3..8fb3f9d 100644
--- a/src/trix/models/block.coffee
+++ b/src/trix/models/block.coffee
@@ -96,7 +96,7 @@ class Trix.Block extends Trix.Object
     getBlockConfig(@getLastAttribute())?.terminal

   breaksOnReturn: ->
-    getBlockConfig(@getLastAttribute())?.breakOnReturn
+    getBlockConfig(@getLastAttribute() ? "default").breakOnReturn

   findLineBreakInDirectionFromPosition: (direction, position) ->
     string = @toString()
diff --git a/src/trix/models/line_break_insertion.coffee b/src/trix/models/line_break_insertion.coffee
index 27b0657..93e45eb 100644
--- a/src/trix/models/line_break_insertion.coffee
+++ b/src/trix/models/line_break_insertion.coffee
@@ -15,7 +15,7 @@ class Trix.LineBreakInsertion
     if @block.hasAttributes() and @block.isListItem() and not @block.isEmpty()
       @startLocation.offset isnt 0
     else
-      @breaksOnReturn and @nextCharacter isnt "\n"
+      @breaksOnReturn unless @shouldBreakFormattedBlock()

   shouldBreakFormattedBlock: ->
     @block.hasAttributes() and not @block.isListItem() and

And then in your app:

Trix.config.blockAttributes.default.tagName = "p"
Trix.config.blockAttributes.default.breakOnReturn = true

Let me know what you think.

[joeldrapper]

@javan this seams to work perfectly and it doesn’t break any tests. Is there any reason why this can’t be merged as a configureable option? I’d happily work on fixing any issues, but your solution seams perfect. 👌

[joeldrapper]

@javan I’ve tested your suggested changes quite throughly and everything appears to work very well. I’m running it live in production with paragraph tags and I’ve had no problems so far.

If I write some tests and submit a PR, would you consider merging this in? None of the original tests break if you don’t configure Trix to use paragraphs.

[javan]

@joeldrapper, that'd be great! Especially adding a test or two.

[svennerberg]

I would love to see this implemented as well.

[joeldrapper]

@svennerberg sorry for the delay on this! For now, you can use @javan’s implementation by changing a couple of lines.

[svennerberg]

@joeldrapper It works like a charm! Thanks!

[svennerberg]

@joeldrapper @javan After some further testing of this approach I experienced some unwanted side effects. When adding an image it's duplicated each time I save the document and then read it back from the database. So at first, there's 1 image then 2 then 4 and so on.

I don't understand exactly why this is happening but the problem disappears when I don't use:

Trix.config.blockAttributes.default.tagName = "p"
Trix.config.blockAttributes.default.breakOnReturn = true

[joeldrapper]

I’ve been using it in production with attachments disabled, so thanks for letting me know. I’ll look into it.

[danielpuglisi]

@joeldrapper any update on this?

[iamdriz]

I'm struggling to follow this one. Is this merged into the code then? As when I do:

Trix.config.blockAttributes.default.tagName = "p"
Trix.config.blockAttributes.default.breakOnReturn = true

I'm getting <br> tags inside the <p> tags. Is that correct behaviour? e.g.

<p><!--block-->Paragraph1<br><br>Paragraph2</p>

Where I was expecting more.

<p><!--block-->Paragraph1</p><p><!--block-->Paragraph2</p>

[stale[bot]]

This issue has been automatically marked as stale after 90 days of inactivity. It will be closed if no further activity occurs.

[ghost]

Is there any reason that the change @javan wasn't merged. I would say it's minimal requirement of a WYSIWYG editor to support paragraphs from an accessibility standpoint. Is there any way to hijack the events in the meantime and insert a custom block element?

[athelas64]

They never cared about accessibility all that much. Unfortunately.

[gregblass]

@athelas64 I don't think that's the right attitude here. Basecamp open-sourced a fantastic WYSIWYG editor. They didn't have to do that. They may have no need for this feature in their company.

I don't see any pull requests here. I bet if someone made a pull request with that implementation, they would merge it.

You always have the option to download the gem, modify it with the changes above, and use it locally. This may be a pain the first time you do it / have some learning curve, but it's a valuable thing to know how to do moving forward in your dev career.

[athelas64]

@gregblass I must say that it is the very right attitude regarding the issue itself. However, I kindly apologize for the fact that it is not the right attitude here. Thanks for the constructive reminder.

[ghost]

@gregblass This pull request wasn't merged and seems to address the issue: https://github.com/basecamp/trix/pull/187.

I'm well aware of how open source works and I have personally contributed to Rails with quite significantly significant changes in the past — I'm the author or the 'Sexy Validations' syntax in rails, along with easing the use of custom validators.

As Trix is now used in ActionText (a Rails 6 feature), I would argue that this functionality is pretty essential for anyone concerned about accessibility.

[gregblass]

@thelucid Nice! I did not know that it was being merged into Rails. That is awesome, and changes the situation here.

I just thought the "basecamp never cared about accessibility all that much" comment wasn't necessary - that's all.

I didn't look at open pull requests so that's great that there is one. Looks like no one needed my two cents here so I'll step away and hope this gets merged!

[javan]

Is there any reason that the change wasn't merged.

As I said before:

One potential issue with using <p> is Trix's image attachments, which are rendered in a <figure> element. <p> only permits phrasing content so placing a <figure> in one would be invalid.

If we "just" switched from <div> to <p>, inserting an attachment would result in invalid HTML like the following:

<p>the <figure>…</figure> quick</p>
<p>brown fox</p>

To see just how invalid that HTML is, try loading it in a browser:

So, that's the main reason support for <p> hasn't moved forward. It's not a simple change and it's not a high priority for us.

They never cared about accessibility all that much. Unfortunately.

I would argue that this functionality is pretty essential for anyone concerned about accessibility.

I can ensure you that we do care about accessibility (see https://m.signalvnoise.com/the-next-big-jump-in-basecamp-accessibility/ for one example). Can you help me understand how much accessibility would be improved by using <p>s? For example, how significant is the difference between these two HTML snippets when using a screen reader or other assistive device?

<div>
paragraph one
<br>
<br>
paragraph two
</div>

<p>paragraph one</p>
<p>paragraph two</p>

/cc @bergatron

[ghost]

@javan This paragraph sums it up nicely (pardon the pun): https://developer.mozilla.org/en-US/docs/Web/HTML/Element/br#Accessibility_concerns.

I would say that images probably should appear mid way through a paragraph anyway, so couldn't the figure just be placed in a div instead of p when it is added i.e. the div could just be a block type just like h1 is used for heading?

[danielpuglisi]

Hello everyone. Just wanted to drop in a quick solution on how we work around this issue until a solution is found.

We use the editor for different purposes (blog posts, cms pages, emails etc) and our main issue are the multiple <br> tags to differentiate between paragraphs. Reason being there is no way, or at least I'm not aware of one, to style the margin between two paragraphs with <br> tags. Although now that I'm rethinking it, one could maybe do something with a br + br selector to achieve that. But not sure if possible.

Anyway. We solved this by forking Trix and implementing the small change @javan proposed here https://github.com/basecamp/trix/issues/202#issuecomment-282306943 so that we get a new <div> tag every time a user hits enter and a <br> tag if they hit shift + enter. This is by the way how word processors work as well. Most of the time you just don't see it because they don't have the margin bottom pre-styled and this is something we have to do as well.

Because we also use Trix to compose emails, we not always want the editor to behave like a word processor. Mainly because paragraphs in emails are a nightmare and it's easier to just use multiple <br> tags. So we added a custom data-trix-options='break_on_return' attribute to the ones that should act like a word processor. Here is the necessary JS:

  if $("[data-trix-options~='break_on_return']").length > 0
    Trix.config.blockAttributes.default.breakOnReturn = true

... and the styles for the Trix editor:

[data-trix-options~='break_on_return']
  div, blockquote, pre, ul, ol, h2
    margin-bottom: 1rem
    + h2
      margin-top: 2rem
  pre
    font-size: 1.2rem
  h2
    font-size: rem-calc(24px)

Those are needed so the users actually see what they get. Feel free to adjust to your own needs.

The only thing left to do is to convert the <div> to <p> tags. We currently just use Nokogiri to switch them before rendering the content:

  def render_editor_html(html)
    doc = Nokogiri::HTML.fragment(html)

    div_nodes = doc.search("./div[not(.//figure)]")
    div_nodes.each do |node|
      if node.content.blank?
        node.remove
        next
      else
        node.name = 'p'
      end
    end

    doc.to_html.html_safe
  end

But I wonder if this is even necessary and has any affects on accessibility. Please enlighten me :)

Hope this is useful. Cheers.

[danielpuglisi]

@javan any chance we could get the breakOnReturn option from your comment officially implemented?

[inod]

Since Trix 1.1.0 the fix from https://github.com/basecamp/trix/issues/202#issuecomment-282306943 doesn't work in macOS Safari any more.

In Safari, shift+enter creates a new block. Firefox and Chrome work fine.

Any ideas?

[danielpuglisi]

@inod having the same issue? Have you found a solution? At least control+enter works in Safari.

[inod]

@inod having the same issue? Have you found a solution? At least control+enter works in Safari.

Shift+enter in safari, trix 1.2.0 also doesn't work (with above changes from @javan)

A hacky solution that works on top of the other changes is:

document.addEventListener('trix-before-initialize', function(ev) {
    ev.target.addEventListener('keydown', function(ev) {
        if (ev.shiftKey && ev.key == "Enter") {
            ev.target.editor.recordUndoEntry('Shift+Enter');
            ev.target.editor.insertHTML('<br><br>');
            ev.preventDefault();
        }
    });
});

I've used '<br><br>' because ev.target.editor.insertLineBreak(); inserted a new paragraph block and ev.target.editor.insertHTML('<br>'); did nothing

[lazaronixon]

I found a solution for style here.. available on this fork https://github.com/lazaronixon/trix/tree/contentify It will generate a output like:

<h1>title<h1>
<div class="p">paragraph1</div>
<div class="p">paragraph1</div>

You can configure paragraph class with:

Trix.config.css.paragraph = "trix-p"

package.json

"trix": "lazaronixon/trix#contentify"