Add task-list-item class to task list items

[nicoburns]

I am trying to render tasklists similar to how Github renders them. Unfortunately it is currently very difficult to differentiate between a tasklist and a regular list generated by comrak using CSS, which is leading to my renderer rendering both the checkboxes and the regular list bullets for tasklists.

Github works around this issue by adding a special class to the <ul> and the <li> elements, which allows list-style-type: none to be specified for tasklist list items (causing the regular bullets to be hidden). I have implemented this for the <li> element of task lists in this PR. Doing it for the <ul> as well is more difficult as it doesn't have a special AST node. And it is also not required to set the list-style-type style for the list items.

Illustration of the problem:

Testing mixed lists:

• Hello
• World
• Item
• Item 2

1. Numbered

• Item

1. Numbered

2. Item

3. Numbered

• Bullet

• Bullet

1. Numbered

[digitalmoksha]

Nice addition @nicoburns. I like the idea of having that class added, have thought about adding it myself - would save me from having to do it in post-processing. Though it might need to be behind a render option since it would change what the default GFM output is.

Having a class on the <ul> is useful. You could add a new property to NodeList that gets set when a task list item is added to the AST - its parent will be the <ul>. It should only be set on the first parent of a task list item, not any additional ancestors. It would be set as long as at least one item is a task item, even if it's a mixture of task list items and normal list items. This is also how GH behaves, you can inspect the example list below.

• one
• two
  • three

I like the class task-list (which is what we use), while GH uses contains-task-list

[digitalmoksha]

One suggestion - it looks like you're making a specific choice between ordered, unordered, and task list. However, a task list can be either ordered or unordered. GH makes the distinction but doesn't display the numbers, while GitLab makes the distinction and does display the numbers.

So I would keep making the list either ordered or unordered, and then they either contain tasks or not.

[nicoburns]

Yep! I thought I was being clever combining it into the type enum. Then realised the tests weren't passing 🤦 . Will rework this to use a separate property.

[nicoburns]

Ok, done! And I've squashed it all down into one commit because the history was getting messy :)

[digitalmoksha]

script/cibuild is failing because it's not passing the standard GFM spec. You're going to have to add a render option. But my naming_fu is failing me right now...

[nicoburns]

script/cibuild is failing because it's not passing the standard GFM spec. You're going to have to add a render option. But my naming_fu is failing me right now...

@digitalmoksha Right, an option it is! You reckon it ought to be added to this RenderOptions struct?

Given that we're doing a configuration option anyway, do you think it might make sense to make the actual class names configurable? So then the API would be something like:

struct RenderOptions {
      tasklist_classnames: TaskListClassNames, // Could also be Option<TaskListClassNames> ?
      ...
}

struct TaskListClassNames {
     list: Option<String>,
     list_item: Option<String>,
     checkbox: Option<String>,
}

Then we don't have to agree on the best class names to use!

[digitalmoksha]

@nicoburns yes, I think it should go in RenderOptions

Concerning the class names. I'll withdraw my naming suggestion. I think allowing the class names to be specified would add unnecessary complexity. So I would leave it as you have it, and if it ever became a real requirement, could be considered then.

But I'm all for simplicity.

[nicoburns]

@digitalmoksha I've updated to gate outputting of classes behind an render.tasklist_classes option! Tests passing locally, but I don't think it runs the spec tests.

[digitalmoksha]

@nicoburns looks really good!

wdyt about adding the option in main.rs? I think it's good to have these options available from the command line as well.

I approved to let the all the tests run. Looks like there is a small clippy error, but otherwise everything passes.

I would say if we can fix that, add the option for the command line, then we can pass this to @kivikakk for final approval.

[nicoburns]

@digitalmoksha Formatting error fixed. And option added to the CLI in main.rs :)

[digitalmoksha]

@nicoburns sorry, I missed the fact that we need to add the option to the fuzz_targets/all_options.rs file. Can you pop that in real quick?

diff --git a/fuzz/fuzz_targets/all_options.rs b/fuzz/fuzz_targets/all_options.rs
index 131c932..2af39c8 100644
--- a/fuzz/fuzz_targets/all_options.rs
+++ b/fuzz/fuzz_targets/all_options.rs
@@ -57,6 +57,7 @@ fuzz_target!(|s: &str| {
     render.ignore_empty_links = true;
     render.gfm_quirks = true;
     render.prefer_fenced = true;
+    render.tasklist_classes = true;
 
     markdown_to_html(
         s,

I'm running fuzzing now, cargo +nightly fuzz run gfm, and not seeing any problems.

@kivikakk mind giving this a final review when you have time?

[kivikakk]

This looks excellent. Thanks so much for your work, @nicoburns, and for reviewing and providing feedback, @digitalmoksha!