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 disabling loose lists #73

Closed
4 tasks done
stevemk14ebr opened this issue Mar 22, 2024 · 9 comments
Closed
4 tasks done

Allow disabling loose lists #73

stevemk14ebr opened this issue Mar 22, 2024 · 9 comments
Labels
🙋 no/question This does not need any changes 👎 phase/no Post cannot or will not be acted on

Comments

@stevemk14ebr
Copy link

Initial checklist

Problem

With nested lists, the loose flag in listItem causes newlines at the start of line items that breaks proper list rendering. See

<ul>
<li><div style="border: 1px solid rgba(255, 255, 255, 0.15); display: inline-block; border-radius: 4px; width: fit-content;"><span class="markdown" style="list-style-position: outside; display: inline; font-family: monospace; text-decoration: none; white-space: pre-wrap;"><code>hxxps[:]//evil[.]com/</code></span></div>
<div style="border: 1px solid rgba(255, 255, 255, 0.15); display: inline-block; border-radius: 4px; width: fit-content;"><span class="markdown" style="list-style-position: outside; display: inline; font-family: monospace; text-decoration: none; white-space: pre-wrap;"><ul>
<li><code>uri</code>: <code>/update.bin</code>
<ul>
<li><code>domainname</code>: <code>evil[.]com</code></li>
<li><code>scheme</code>: <code>hxxps</code></li>
</ul>
</li>
</ul></span></div></li>
</ul>

vs loose list:

<ul>
<li>
<p><div style="border: 1px solid rgba(255, 255, 255, 0.15); display: inline-block; border-radius: 4px; width: fit-content;"><span class="markdown" style="list-style-position: outside; display: inline; font-family: monospace; text-decoration: none; white-space: pre-wrap;"><code>hxxps[:]//evil[.]com/</code></span></div></p>
<p><div style="border: 1px solid rgba(255, 255, 255, 0.15); display: inline-block; border-radius: 4px; width: fit-content;"><span class="markdown" style="list-style-position: outside; display: inline; font-family: monospace; text-decoration: none; white-space: pre-wrap;"><ul>
<li><code>uri</code>: <code>/update.bin</code>
<ul>
<li><code>domainname</code>: <code>evil[.]com</code></li>
<li><code>scheme</code>: <code>hxxps</code></li>
</ul>
</li>
</ul></span></div></p>
</li>
</ul>

The issue is specifically:

<ul>
<li>
<p>

The start of the

block is put on a newline after the li and this newline causes the bullet to be on the wrong line. In the first case it goes:

<ul>
<li><div...

Where the div is immediately after with no newline and this renders correctly.

image

The markdown here is:

## Connections

*   `hxxps[:]//evil[.]com/`

    *   `uri`: `/update.bin`
        *   `domainname`: `evil[.]com`
        *   `scheme`: `hxxps`

Compare this to a markdown without a space between the first bullet and the next ones. (Not a loose list).
image
With the markdown being:

## Connections

*   `hxxps[:]//evil[.]com/`\
    *   `uri`: `/update.bin`
        *   `domainname`: `evil[.]com`
        *   `scheme`: `hxxps`

So you can see how this is undesireable, the bullet should stay aligned to the correct row but it's not when loose lists are detected. Please allow an option to disable loose lists, I cannot have this newline character present at the start of the listItem.

Solution

Add an optional flag to disable loose lists (

// Add eols before nodes, except if this is a loose, first paragraph.
if (
loose ||
index !== 0 ||
child.type !== 'element' ||
child.tagName !== 'p'
) {
children.push({type: 'text', value: '\n'})
}
)

Alternatives

Change listItem to not place a newline before the listItem.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Mar 22, 2024
@stevemk14ebr
Copy link
Author

import { visit } from 'unist-util-visit';
const splice = [].splice

export default function rehypeFixList(options) {
  return transform;

  function transform(tree) {
    visit(tree, 'element', onelement)
  }

  function onelement(node, index, parent) {
    if(node.tagName == 'li' && node.children.length > 1 && node.children[0].type == 'text' && node.children[0].value == '\n') {
        node.children = node.children.slice(1); //remove first newline after an li
    }
    return undefined;
  }
}

This is my custom plugin I put in the pipeline to fix this for now

@wooorm
Copy link
Member

wooorm commented Mar 22, 2024

What do you want to happen if there are two paragraphs next to each other?

Why is whitespace problematic? You can use CSS to change it?

Why should there be an option to do this instead of a plugin? Most things happen with plugins. Especially if not all users would want something, in a particular way.

@stevemk14ebr
Copy link
Author

stevemk14ebr commented Mar 22, 2024

There is no CSS layout I could find that would fix the newline before the li and the paragraph tag. If you set display: inline it will override correct bullet layouts in other situations.

As my image shows, this newline causes the bullet to render on the wrong line. The issue is not two paragraphs next to each other, the issue is that when this happens the first paragaph is put on a newline after the li opens:

this is wrong:

<ul>
<li>
<p>

it should be this

<ul>
<li><p>

the non-loose list generates this correct, but starts with the content directly and doesn't wrap it in a paragraph, it emits:

<ul>
<li><div>

New lines inbetween the paragraphs would be expected and correct. But there are newlines inserted before the first paragraph when loose list is detected.

@stevemk14ebr
Copy link
Author

stevemk14ebr commented Mar 22, 2024

Suggested fix might be something like this: https://github.com/syntax-tree/mdast-util-to-hast/blob/c7a658efaf31316fe01033bada3e5f396cbc6576/lib/handlers/list-item.js#L63C2-L82

 let firstChildAdded = false;
  while (++index < results.length) {
    const child = results[index]

    // Add eols before nodes, except if this is a loose, first paragraph.
    if (firstChildAdded && (
      loose ||
      index !== 0 ||
      child.type !== 'element' ||
      child.tagName !== 'p')
    ) {
      children.push({type: 'text', value: '\n'})
    }

    if (child.type === 'element' && child.tagName === 'p' && !loose) {
      children.push(...child.children)
    } else {
      children.push(child)
    }
    firstChildAdded = true;
  }

You could also change the order of the operations to add the newlines after the first child is added instead. But you'd have to detect the final child and not emit a newline in that case.

@ChristianMurphy
Copy link
Member

this is wrong:

<ul>
<li>
<p>

it should be this

<ul>
<li><p>

Looking at CommonMark https://spec.commonmark.org/dingus/?text=%23%23%20Connections%0A%0A*%20%20%20%60hxxps%5B%3A%5D%2F%2Fevil%5B.%5Dcom%2F%60%0A%0A%20%20%20%20*%20%20%20%60uri%60%3A%20%60%2Fupdate.bin%60%0A%20%20%20%20%20%20%20%20*%20%20%20%60domainname%60%3A%20%60evil%5B.%5Dcom%60%0A%20%20%20%20%20%20%20%20*%20%20%20%60scheme%60%3A%20%60hxxps%60
The reverse is true, according to the reference implementation. The first output is correct, it should not be the second.

As @wooorm mentions you are welcome to use CSS to adjust the styles, or create a plugin to adjust the content if you prefer that over CSS https://unifiedjs.com/learn/

@ChristianMurphy ChristianMurphy closed this as not planned Won't fix, can't repro, duplicate, stale Mar 22, 2024
@ChristianMurphy ChristianMurphy added the 🙋 no/question This does not need any changes label Mar 22, 2024

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Mar 22, 2024
@stevemk14ebr
Copy link
Author

Ah ok thanks, I see what's happening.

Copying the styles used in the reference page works to fix the loose list bullets. However, in my case those markdown directives you see are rendered as spans containing content and marked as display: inline-block which breaks the layout in normal lists.

So for me, this is a bad interaction between my custom code, the newline rehype adds, and typical css bullet layouts. I'll stick with my custom pass to remove the newline since this is unique to my implementation constraints

@wooorm
Copy link
Member

wooorm commented Mar 22, 2024

Yes, I was about to say but you seem to reach similar conclusions: this is regular markdown/html behavior. I haven't heard of your problem before. So there must be something specific to your case.
Directives, in this case an inline block one, seems to be the source. Not sure how to handle it, perhaps you can use a different directive kind, or a different display?

@stevemk14ebr
Copy link
Author

stevemk14ebr commented Mar 22, 2024

I have it working now, I need my directives to be rendered as display: inline because users may choose to write content with words on either side with the directive in the middle holding some static markdown content. But in other cases (like when they hold bullets) the directives should instead be display: block. A good solution would be me changing my code to change the display style depending on the content but in reality this issue only occurs with this one specific case of loose lists so I'm choosing to just edit the rehype AST myself and remove the troublesome newline. The newline interacts poorly with my display inline and whitespace css.

All is well, sorry for the noise, I can work around this easily with my custom plugin pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 no/question This does not need any changes 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

3 participants