Issues with task lists in blockquotes

[mtodd]

Originally reported in: github/markup#322

Reproduce with:

> - [ ] item 1
> - [ ] item 2

While this appears to render fine, updates aren't handled correctly with inconsistent results: primarily resulting in updates not going through.

I think the simplest solution for the time being is to disable updates for these items, though making them work would be ✨.

[mtodd]

@raganwald to make it work, we need to fix the itemPattern to handle optional blockquoting.

If we want to avoid rendering it entirely, we'd want to better scope the ListItemSelector XPath selector.

Though rendering it disabled would require deeper/more involved modifications to introduce disabled task lists that aren't auto-enabled on load. I think this gives us the least benefit and requires the most work.

[raganwald]

I'm in favour of handling the block-quoting, if we don't have any semantic objection.

[mtodd]

@raganwald I'm right on the fence either way in terms of semantics, so practicality beats purity here.

[raganwald]

One day, when we have blue skies on mars, we will look at a thing like this and say, "If you quote a list, the state of the items in the quoted list should be synchronized with the state of the items in the original."

The reason we will have that conversation one day, is because we will make the wise choice not to have that conversation today.

[mtodd]

@raganwald to be fair, I'd rather think quoting a list wouldn't sync it but copy it at the time of the quotation... it's a quote, and I think the historical value of quotes is more interesting than the current state of its origin!

[raganwald]

Interesting... 💭

That was not what I was thinking, but if we are both thinking the exact same thing, one of us is redundant.

[raganwald]

@mtodd:

I'm circling back to this today. To recapitulate, if you write something like this in a comment:

> - [ ] item 1
> - [ ] item 2

The items render as checkboxes, however checking them in the UX does not update the comment to check or uncheck the checkbox. The requirement is that checking and unchecking work whether the item is in a blockquote or not, and the first line of investigation is to update itemPattern.

Let me know if you have anything to add/change...

[mtodd]

That's spot on! Thanks for taking a look! There are "functional" tests that
you can use to recreate the case and build a solution against!

On Tuesday, September 2, 2014, Reg Braithwaite notifications@github.com
wrote:

@mtodd https://github.com/mtodd:

I'm circling back to this today. To recapitulate, if you write something
like this in a comment:

• item 1
• item 2

The items render as checkboxes, however checking them in the UX does not
update the comment to check or uncheck the checkbox. The requirement is
that checking and unchecking work whether the item is in a blockquote or
not, and the first line of investigation is to update itemPattern.

Let me know if you have anything to add/change...

—
Reply to this email directly or view it on GitHub
#31 (comment).

Matt Todd
GitHub, Inc
www.github.com
cell: 404-314-2612
blog: maraby.org

[raganwald]

Observation:

Given a quoted list of two items in a GFM comment like this one, we see that the check list within the blockquote is disabled. This is unlike an ordinary task list, where all items are enabled. Although the check list items are disabled, clicking on the check boxes still sends an AJAX request:

The data sent appears to be wrong: When clicking the first item, we should be sending:

> - [x] item 1
> - [ ] item 2

Also, we should not be sending anything at all when clicking on a disabled item. So...

1. We could stop sending any XHR when a user clicks on a task list within a blockquote, or;
2. We could enable the task list, and send the correct markdown to the server.

I am proceeding with #2, "enable the task list and send the correct markdown to the server" unless someone raises their ✋

[mtodd]

@raganwald it's important (nay, vital) to distinguish between defects with this library and defects with the app that integrates this library into itself whose responsibility is to enable/disable task lists where appropriate. XHR updates are an app-specific responsibility, made possible by providing after-update hooks (using the tasklist:changed event). So both items in your list do not relate to this library specifically, but in the app in which you are testing.

The best way to recreate the library-specific issue is to create a functional test (or a unit test), and work from there. That way you can control the environment and work against the issues related to the library and not with the integrating apps.

[bkeepers]

So is this still an issue?

[mtodd]

@bkeepers should be fixed! The release with #36 was cut and deployed last week.