Use the sourcepos attribute for finding tasks

app/models/concerns/cache_markdown_field.rb

@@ -15,7 +15,7 @@ module CacheMarkdownField
   # Increment this number every time the renderer changes its output
   CACHE_REDCARPET_VERSION         = 3
   CACHE_COMMONMARK_VERSION_START  = 10
-  CACHE_COMMONMARK_VERSION        = 13
+  CACHE_COMMONMARK_VERSION        = 14
   # changes to these attributes cause the cache to be invalidates
   INVALIDATED_BY = %w[author project].freeze
@@ -130,13 +130,17 @@ module CacheMarkdownField
   def latest_cached_markdown_version
     return CacheMarkdownField::CACHE_COMMONMARK_VERSION unless cached_markdown_version
-    if cached_markdown_version < CacheMarkdownField::CACHE_COMMONMARK_VERSION_START
+    if legacy_markdown?
       CacheMarkdownField::CACHE_REDCARPET_VERSION
     else
       CacheMarkdownField::CACHE_COMMONMARK_VERSION
     end
   end
+  def legacy_markdown?
+    cached_markdown_version && cached_markdown_version.between?(1, CacheMarkdownField::CACHE_COMMONMARK_VERSION_START - 1)
+  end
+
   included do
     cattr_reader :cached_markdown_fields do
       FieldData.new

app/services/issuable_base_service.rb

@@ -262,16 +262,16 @@ class IssuableBaseService < BaseService
   # Handle the `update_task` event sent from UI.  Attempts to update a specific
   # line in the markdown and cached html, bypassing any unnecessary updates or checks.
-  def update_task_event(issue)
+  def update_task_event(issuable)
     update_task_params = params.delete(:update_task)
     return unless update_task_params
-    toggler = TaskListToggleService.new(issue.description, issue.description_html,
+    toggler = TaskListToggleService.new(issuable.description, issuable.description_html,
                                         line_source: update_task_params[:line_source],
                                         line_number: update_task_params[:line_number],
                                         currently_checked: !update_task_params[:checked],
                                         index: update_task_params[:index],
-                                        sourcepos: false)
+                                        sourcepos: !issuable.legacy_markdown?)
     if toggler.execute
       # by updating the description_html field at the same time,
@@ -282,9 +282,9 @@ class IssuableBaseService < BaseService
       # since we're updating a very specific line, we don't care whether
       # the `lock_version` sent from the FE is the same or not.  Just
       # make sure the data hasn't changed since we queried it
-      params[:lock_version]     = issue.lock_version
-      update_task(issue)
+      params[:lock_version]     = issuable.lock_version
+      update_task(issuable)
     else
       # if we make it here, the data is much newer than we thought it was - fail fast
       raise ActiveRecord::StaleObjectError

app/services/task_list_toggle_service.rb

@@ -11,7 +11,7 @@
 class TaskListToggleService
   attr_reader :updated_markdown, :updated_markdown_html
-  def initialize(markdown, markdown_html, line_source:, line_number:, currently_checked:, index:, sourcepos: false)
+  def initialize(markdown, markdown_html, line_source:, line_number:, currently_checked:, index:, sourcepos: true)
     @markdown, @markdown_html  = markdown, markdown_html
     @line_source, @line_number = line_source, line_number
     @currently_checked         = currently_checked
@@ -62,6 +62,13 @@ class TaskListToggleService
     @updated_markdown_html = html.to_html
   end
+  # When using CommonMark, we should be able to use the embedded `sourcepos` attribute to
+  # target the exact line in the DOM.  For RedCarpet, we need to use the index of the checkbox
+  # that was checked and match it with what we think is the same checkbox.
+  # The reason `sourcepos` is slightly more reliable is the case where a line of text is
+  # changed from a regular line into a checkbox (or vice versa).  Then the checked index
+  # in the UI will be off from the list of checkboxes we've calculated locally.
+  # It's a rare circumstance, but since we can account for it, we do.
   def get_html_checkbox(html)
     if use_sourcepos
       html.css(".task-list-item[data-sourcepos^='#{line_number}:'] > input.task-list-item-checkbox").first

spec/models/concerns/cache_markdown_field_spec.rb

@@ -251,6 +251,30 @@ describe CacheMarkdownField do
     end
   end
+  describe '#legacy_markdown?' do
+    subject { thing.legacy_markdown? }
+
+    it 'returns true for redcarpet versions' do
+      thing.cached_markdown_version = CacheMarkdownField::CACHE_COMMONMARK_VERSION_START - 1
+      is_expected.to be_truthy
+    end
+
+    it 'returns false for commonmark versions' do
+      thing.cached_markdown_version = CacheMarkdownField::CACHE_COMMONMARK_VERSION_START
+      is_expected.to be_falsey
+    end
+
+    it 'returns false if nil' do
+      thing.cached_markdown_version = nil
+      is_expected.to be_falsey
+    end
+
+    it 'returns false if 0' do
+      thing.cached_markdown_version = 0
+      is_expected.to be_falsey
+    end
+  end
+
   describe '#refresh_markdown_cache' do
     before do
       thing.foo = updated_markdown