Parse task checkboxes in Markdown, part Ⅰ: The Refactoringing
What does this MR do and why?
First part to set the stage to fix Parse checkboxes in Markdown tables (#21506). (The whole feature was put together in the 2k line monstrosity that is Draft: Parse checkboxes in Markdown tables (!215887 - closed). Here we dial it back into stacked MRs.)
Part 2 is at Draft: Parse task checkboxes in Markdown, part ... (!219037).
An overview of all the changes required is included, followed by what's included in this MR.
Overview
Parsing task checkboxes in Markdown tables, like most Markdown changes, needs to touch a lot of places:
- The Markdown parser needs to know how to parse them (https://github.com/kivikakk/comrak/pull/622, https://github.com/kivikakk/comrak/pull/705)
- The GLFM renderer needs to know how to render them (gitlab-org/ruby/gems/gitlab-glfm-markdown!123 (merged), gitlab-org/ruby/gems/gitlab-glfm-markdown!128 (merged), gitlab-org/ruby/gems/gitlab-glfm-markdown!131 (merged))
- The sanitizer needs to know to keep the newly rendered elements
- The task list Banzai filter need to know what to do with these elements
- The frontend needs CSS to display them appropriately
- Multiple backend components need to be able to parse task list items out of source Markdown via the
Taskableconcern (for "1 of 2 checklist items completed" and system notes) - Multiple frontend components need to know how to translate checking or unchecking a rendered checkbox into a call to the backend to update the item
- Every frontend except work items actually has the backend do the source-level change to minimise accidental reverts/overwrites, meaning the backend needs to know how to update Markdown and HTML given the source position of a changed checkbox
- Work items does the source-level change on the frontend, meaning the frontend also needs to know how to update Markdown given the source position of a changed checkbox
- The rich text editor needs to be able to parse the rendered items, needs its schema to support the new changes, needs to be able to render an editable node view for them, and needs to know how to serialise the new schema out into Markdown (and HTML, when needed)
Every place, backend or fronetnd, that makes Markdown or HTML updates based on source position previously did it via line number alone, because you never had more than one checkbox on a given line. Task checkboxes in tables make multiple a possibility, and so we've taken the opportunity to make the checkbox locating precise via added information from the Markdown parser (right at the top of that list). Old "imprecise" sourcepos matching is still supported because it'll be in every single cached Markdown to date and still needs to work. Newly rendered content will also contain the checkbox's precise sourcepos, which will be used when present.
The frontend task list abstraction was half-built on a long-archived library which our backend task list abstraction also used to be half-built on. This library also encoded the "one item per line" requirement into its design, and given its lack of maintenance and our need for greater control, moving off it (and dropping the very old-timey jQuery events thrown around) was considered the best idea.
What's included in this MR?
This MR contains no user-facing changes. We do the following:
-
backend + frontend
- Update checkbox togglers to use precise checkbox sourcepos in preference to imprecise, when provided
- Minor: Use consistent sourcepos terminology throughout the monolith, matching the wider ecosystem and our upstream libraries: we refer to sourcepos
lineandcolumn
-
backend
- Update the version of
glfm-gitlab-markdownto get the precise checkbox source position information - Allow the precise checkbox sourcepos through in the sanitizer
-
Subtle edge-case in backend toggle service, since it works on both source and HTML: correctly maintain precise checkbox sourcepos in the face of replacing a multi-byte whitespace character with a single byte- Moved to follow-up MR: it was only used by code from it, and so we had an undercoverage problem!
- Update the version of
-
frontend
- Move off
deckar01-task_listin the frontend as we've already done in the backend, rewriting the abstraction and making the control flow easier to follow
- Move off
| Non-spec | Spec | Deps | |
|---|---|---|---|
| backend | +158 -44 | +204 -137 | +31 -31 |
| frontend | +152 -66 | +71 -85 | +1 -12 |
How to set up and validate locally
- Check out the branch, ensure you
bundleandyarnto have your dependencies up-to-date, andgdk restart rails-webto be using the latest version - Make some task lists in issues and MRs, toggle them like you normally would, check that everything looks just like it did!
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.