-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Tasklists don't work if checkbox is a direct child of a list item #80
Comments
Quick update: you can also test this with actual HTML using import {inspect} from 'node:util';
import {fromHtml} from 'hast-util-from-html';
import {toMdast} from 'hast-util-to-mdast';
const hast1 = fromHtml(`<ul>
<li>
<input type="checkbox" checked> Checked
</li>
<li>
<input type="checkbox"> Unchecked
</li>
</ul>`, {fragment: true, verbose: false});
const mdast1 = toMdast(hast1);
console.log('Bare inputs:' + inspect(mdast1, false, 100, true));
console.log('\n-----------------\n');
const hast2 = fromHtml(`<ul>
<li>
<p><input type="checkbox" checked> Checked</p>
</li>
<li>
<p><input type="checkbox"> Unchecked</p>
</li>
</ul>`, {fragment: true, verbose: false});
const mdast2 = toMdast(hast2);
console.log('Paragraph inputs:' + inspect(mdast2, false, 100, true)); |
Sure, good to have, looks like a bugfix indeed. Something like: let head = node.children[0]
if (head && head.type === 'element' && head.tagName === 'p') {
head = head.children[0]
}
if (head && head.type === 'element' && head.tagName === 'input' …) I think? Are you interested in doing the work? |
Sure, happy to take this. 👍 Related question: should this try and handle cases where the first node is a comment? (I was also going to ask about whitespace, but it looks like that's already taken care of by the time we get to this step.) For example: <ul>
<li>
<!-- Still mark the list item as a tasklist item even though this comment is here? -->
<input type="checkbox" checked> Checked
</li>
</ul> |
Also, I guess, any other containing element, too, not just a <ul>
<li>
<span><input type="checkbox" checked> Checked</span>
</li>
<li>
<label><input type="checkbox" checked> Checked</label>
</li>
<li>
<div><input type="checkbox" checked> Checked</div>
</li>
</ul> There are probably a handful it doesn't make sense to look inside ( And nesting: <ul>
<li>
<p><span><input type="checkbox" checked> Checked</span></p>
</li>
</ul> Useful or too complex? Just trying to think of what's worth covering if I’m making this part more flexible. |
Compatibility with markdown is important but I don‘t think we should look into arbitrary elements, perhaps it’s a form, or something else, that could lead to bugs. The comment… Maybe. If you feel like it feel free to do it. |
Makes sense, I’ll try and do this Sunday if I've got time. Otherwise sometime next week.
It seems like whitespace nodes at the start of hast-util-to-mdast/lib/index.js Lines 28 to 41 in 7deea1b
|
Well, I wrote a fairly complex version of this that skipped over leading comments, but it didn’t work because…
Possible changes to address this seem fairly complex and out of scope here, so I’m going to unwind this code and drop the idea of supporting comments. (This also suggests handling other kinds of container elements for the checkbox besides Unfortunately trying to figure out what was going on there used all my remaining time for this today, so I’ll probably get back to it sometime later. |
Ah perfect, I didn’t think about that!
In mdast, there must be a paragraph node in
we could try to detect comments (starts with
Sure, thanks. No rush from me! Or, if you can drop the code here, I or someone else can see if it can be made into a PR! |
Previously, list items with leading checkboxes would only be treated as task list items if the checkbox was wrapped in a `<p>` element, like: <li><p><input type="checkbox"> Other content</p></li> This adds support for detecting task lists even when the checkbox is not nested in a `<p>`: <li><input type="checkbox"> Other content</li> This does *not* add support for some other complex scenarios, such as the checkbox being inside other phrasing content, being more deeply nested, or having comments in front of it: <li><strong><input type="checkbox"> Other content</strong></li> <li><p><strong><input type="checkbox"> Other content</strong></p></li> <li><!-- Comment --><input type="checkbox"> Other content</li> Fixes syntax-tree#80.
Just got back to this today and will post a PR momentarily. Sorry for the delay and the lack of replies; I had a last minute family issue come up.
Ah, that makes sense. I wasn't quite familiar enough with the ins and outs of the Mdast model to catch that the
Yeah, that makes sense. It's way out of scope for this change, but I wonder if adding a |
Comments in MDX are expressions, just like |
Right, I guess I was thinking more about converting HTML or MD (or something else) → MDX, so you’d need to know that (I think I had this on my mind because someone recently filed an issue on https://github.com/Mr0grog/google-docs-to-markdown that turned out to be that they were trying to use the output in MDX, which didn’t always work because the output is actually Markdown, not MDX.) |
md -> mdx might be interesting for several folks! But that’s a different discussion! |
Initial checklist
Affected packages and versions
has-util-to-mdast@10.0.0
Link to runnable example
No response
Steps to reproduce
Tasklists (lists where each item starts with a checkbox) are parsed correctly if the leading checkbox
<input>
element in an<li>
node is nested in a<p>
node, but not if the<input>
is a direct child of the<li>
. I’m not sure why this is a requirement (it’s certainly not in the HTML spec: https://html.spec.whatwg.org/multipage/grouping-content.html#the-li-element), so I assume it’s a bug. (But I appreciate also handling this common case where things are in a paragraph in the list item, which is best practice HTML.)You can see where this explicit check for a
<p>
is implemented inlib/handlers/li.js
:hast-util-to-mdast/lib/handlers/li.js
Lines 19 to 46 in 7deea1b
I used this script to test:
Expected behavior
I expected that this script:
…to output:
…which corresponds to this Markdown:
Actual behavior
Instead, the above script outputs:
…which corresponds to this Markdown:
Instead, to get the expected output, you. need to do:
Affected runtime and version
node>=16
Affected package manager and version
npm@9.5.1
Affected OS and version
macOS 13.5
Build and bundle tools
No response
The text was updated successfully, but these errors were encountered: