Fix errors when checking MR task lists

The HTML spec in https://www.w3.org/TR/html52/syntax.html#in-body
mentions: Newlines at the start of textarea elements are ignored as
an authoring convenience.

This causes issues with task list parsing because the lines are now off
by one.

We didn't have this problem with issues because the textarea value was
set in Javascript with Vue.

This also removes instances of js-task-list-field which are actually
unused.

app/assets/javascripts/task_list.js

@@ -31,6 +31,15 @@ export default class TaskList {
   init() {
     this.disable(); // Prevent duplicate event bindings
 
+    const taskListFields = document.querySelectorAll(
+      `${this.taskListContainerSelector} .js-task-list-field[data-value]`,
+    );
+
+    taskListFields.forEach(taskListField => {
+      // eslint-disable-next-line no-param-reassign
+      taskListField.value = taskListField.dataset.value;
+    });
+
     $(this.taskListContainerSelector).taskList('enable');
     $(document).on('tasklist:changed', this.taskListContainerSelector, this.updateHandler);
   }

app/views/projects/issues/show.html.haml

@@ -68,9 +68,8 @@
       .title-container
         %h2.title= markdown_field(@issue, :title)
       - if @issue.description.present?
-        .description{ class: can?(current_user, :update_issue, @issue) ? 'js-task-list-container' : '' }
+        .description
           .md= markdown_field(@issue, :description)
-          %textarea.hidden.js-task-list-field= @issue.description
 
       = edited_time_ago_with_tooltip(@issue, placement: 'bottom', html_class: 'issue-edited-ago js-issue-edited-ago')
 

app/views/projects/merge_requests/_description.html.haml

@@ -3,7 +3,6 @@
     .description.qa-description{ class: can?(current_user, :update_merge_request, @merge_request) ? 'js-task-list-container' : '' }
       .md
         = markdown_field(@merge_request, :description)
-      %textarea.hidden.js-task-list-field
-        = @merge_request.description
+      %textarea.hidden.js-task-list-field{ data: { value: @merge_request.description } }
 
   = edited_time_ago_with_tooltip(@merge_request, placement: 'bottom')

app/views/shared/notes/_edit.html.haml

-%textarea.hidden.js-task-list-field.original-task-list{ data: { update_url: note_url(note) } }= note.note
+%textarea.hidden.js-task-list-field.original-task-list{ data: { update_url: note_url(note), value: note.note } }

app/views/shared/snippets/_header.html.haml

@@ -21,8 +21,6 @@
     .description
       .md
         = markdown_field(@snippet, :description)
-      %textarea.hidden.js-task-list-field
-        = @snippet.description
 
   - if @snippet.updated_at != @snippet.created_at
     = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', exclude_author: true)

changelogs/unreleased/208260-fix-mr-task-lists-starting-with-new-line.yml

+---
+title: Fix checking of task lists when MR description starts with a blank line
+merge_request: 43125
+author:
+type: fixed

spec/features/merge_request/user_views_open_merge_request_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe 'User views an open merge request' do
       # returns the whole document, not the node's actual parent element
       expect(find(:xpath, "#{node.path}/..").text).to eq(merge_request.description[2..-1])
 
-      expect(page).to have_content(merge_request.title).and have_content(merge_request.description)
+      expect(page).to have_content(merge_request.title)
     end
   end
 

spec/features/projects/commit/user_comments_on_commit_spec.rb

@@ -6,19 +6,22 @@ RSpec.describe "User comments on commit", :js do
   include Spec::Support::Helpers::Features::NotesHelpers
   include RepoHelpers
 
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
   let(:comment_text) { "XML attached" }
 
-  before do
-    sign_in(user)
+  before_all do
     project.add_developer(user)
+  end
 
-    visit(project_commit_path(project, sample_commit.id))
+  before do
+    sign_in(user)
   end
 
   context "when adding new comment" do
     it "adds comment" do
+      visit(project_commit_path(project, sample_commit.id))
+
       emoji_code = ":+1:"
 
       page.within(".js-main-target-form") do
@@ -57,6 +60,8 @@ RSpec.describe "User comments on commit", :js do
 
   context "when editing comment" do
     before do
+      visit(project_commit_path(project, sample_commit.id))
+
       add_note(comment_text)
     end
 
@@ -87,6 +92,8 @@ RSpec.describe "User comments on commit", :js do
 
   context "when deleting comment" do
     before do
+      visit(project_commit_path(project, sample_commit.id))
+
       add_note(comment_text)
     end
 
@@ -108,4 +115,35 @@ RSpec.describe "User comments on commit", :js do
       expect(page).not_to have_css(".note")
     end
   end
+
+  context 'when checking task lists' do
+    let(:note_with_task) do
+      <<-EOT.strip_heredoc
+
+      - [ ] Task 1
+      EOT
+    end
+
+    before do
+      create(:note_on_commit, project: project, commit_id: sample_commit.id, note: note_with_task, author: user)
+      create(:note_on_commit, project: project, commit_id: sample_commit.id, note: note_with_task, author: user)
+
+      visit(project_commit_path(project, sample_commit.id))
+    end
+
+    it 'allows the tasks to be checked' do
+      expect(page).to have_selector('li.task-list-item', count: 2)
+      expect(page).to have_selector('li.task-list-item input[checked]', count: 0)
+
+      all('.task-list-item-checkbox').each do |checkbox|
+        checkbox.click
+      end
+      wait_for_requests
+
+      visit(project_commit_path(project, sample_commit.id))
+
+      expect(page).to have_selector('li.task-list-item', count: 2)
+      expect(page).to have_selector('li.task-list-item input[checked]', count: 2)
+    end
+  end
 end

spec/features/task_lists_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe 'Task Lists' do
+RSpec.describe 'Task Lists', :js do
   include Warden::Test::Helpers
 
   let_it_be(:project) { create(:project, :public, :repository) }
@@ -38,41 +38,7 @@ RSpec.describe 'Task Lists' do
     MARKDOWN
   end
 
-  let(:nested_tasks_markdown) do
-    <<-EOT.strip_heredoc
-    - [ ] Task a
-      - [x] Task a.1
-      - [ ] Task a.2
-    - [ ] Task b
-
-    1. [ ] Task 1
-       1. [ ] Task 1.1
-       1. [x] Task 1.2
-    EOT
-  end
-
-  let(:commented_tasks_markdown) do
-    <<-EOT.strip_heredoc
-    <!--
-    - [ ] a
-    -->
-
-    - [ ] b
-    EOT
-  end
-
-  let(:summary_no_blank_line_markdown) do
-    <<-EOT.strip_heredoc
-    <details>
-    <summary>No blank line after summary element breaks task list</summary>
-    1. [ ] People Ops: do such and such
-    </details>
-
-    * [ ] Task 1
-    EOT
-  end
-
-  before(:all) do
+  before_all do
     project.add_maintainer(user)
     project.add_guest(user2)
   end
@@ -86,7 +52,7 @@ RSpec.describe 'Task Lists' do
   end
 
   describe 'for Issues' do
-    describe 'multiple tasks', :js do
+    describe 'multiple tasks' do
       let!(:issue) { create(:issue, description: markdown, author: user, project: project) }
 
       it 'renders' do
@@ -127,7 +93,7 @@ RSpec.describe 'Task Lists' do
       end
     end
 
-    describe 'single incomplete task', :js do
+    describe 'single incomplete task' do
       let!(:issue) { create(:issue, description: singleIncompleteMarkdown, author: user, project: project) }
 
       it 'renders' do
@@ -146,7 +112,7 @@ RSpec.describe 'Task Lists' do
       end
     end
 
-    describe 'single complete task', :js do
+    describe 'single complete task' do
       let!(:issue) { create(:issue, description: singleCompleteMarkdown, author: user, project: project) }
 
       it 'renders' do
@@ -175,7 +141,7 @@ RSpec.describe 'Task Lists' do
                       project: project, author: user)
       end
 
-      it 'renders for note body', :js do
+      it 'renders for note body' do
         visit_issue(project, issue)
 
         expect(page).to have_selector('.note ul.task-list',      count: 1)
@@ -183,14 +149,14 @@ RSpec.describe 'Task Lists' do
         expect(page).to have_selector('.note ul input[checked]', count: 2)
       end
 
-      it 'contains the required selectors', :js do
+      it 'contains the required selectors' do
         visit_issue(project, issue)
 
         expect(page).to have_selector('.note .js-task-list-container')
         expect(page).to have_selector('.note .js-task-list-container .task-list .task-list-item .task-list-item-checkbox')
       end
 
-      it 'is only editable by author', :js do
+      it 'is only editable by author' do
         visit_issue(project, issue)
 
         expect(page).to have_selector('.js-task-list-container')
@@ -209,7 +175,7 @@ RSpec.describe 'Task Lists' do
                       project: project, author: user)
       end
 
-      it 'renders for note body', :js do
+      it 'renders for note body' do
         visit_issue(project, issue)
 
         expect(page).to have_selector('.note ul.task-list',      count: 1)
@@ -224,7 +190,7 @@ RSpec.describe 'Task Lists' do
                       project: project, author: user)
       end
 
-      it 'renders for note body', :js do
+      it 'renders for note body' do
         visit_issue(project, issue)
 
         expect(page).to have_selector('.note ul.task-list',      count: 1)
@@ -240,7 +206,7 @@ RSpec.describe 'Task Lists' do
     end
 
     shared_examples 'multiple tasks' do
-      it 'renders for description', :js do
+      it 'renders for description' do
         visit_merge_request(project, merge)
         wait_for_requests
 
@@ -249,7 +215,7 @@ RSpec.describe 'Task Lists' do
         expect(page).to have_selector('ul input[checked]', count: 2)
       end
 
-      it 'contains the required selectors', :js do
+      it 'contains the required selectors' do
         visit_merge_request(project, merge)
         wait_for_requests
 
@@ -261,7 +227,7 @@ RSpec.describe 'Task Lists' do
         expect(page).to have_selector('form.js-issuable-update')
       end
 
-      it 'is only editable by author', :js do
+      it 'is only editable by author' do
         visit_merge_request(project, merge)
         wait_for_requests
 
@@ -300,7 +266,7 @@ RSpec.describe 'Task Lists' do
     describe 'single incomplete task' do
       let!(:merge) { create(:merge_request, :simple, description: singleIncompleteMarkdown, author: user, source_project: project) }
 
-      it 'renders for description', :js do
+      it 'renders for description' do
         visit_merge_request(project, merge)
         wait_for_requests
 
@@ -319,7 +285,7 @@ RSpec.describe 'Task Lists' do
     describe 'single complete task' do
       let!(:merge) { create(:merge_request, :simple, description: singleCompleteMarkdown, author: user, source_project: project) }
 
-      it 'renders for description', :js do
+      it 'renders for description' do
         visit_merge_request(project, merge)
         wait_for_requests
 
@@ -337,7 +303,17 @@ RSpec.describe 'Task Lists' do
   end
 
   describe 'markdown task edge cases' do
-    describe 'commented tasks', :js do
+    describe 'commented tasks' do
+      let(:commented_tasks_markdown) do
+        <<-EOT.strip_heredoc
+        <!--
+        - [ ] a
+        -->
+
+        - [ ] b
+        EOT
+      end
+
       let!(:issue) { create(:issue, description: commented_tasks_markdown, author: user, project: project) }
 
       it 'renders' do
@@ -360,7 +336,18 @@ RSpec.describe 'Task Lists' do
       end
     end
 
-    describe 'summary with no blank line', :js do
+    describe 'summary with no blank line' do
+      let(:summary_no_blank_line_markdown) do
+        <<-EOT.strip_heredoc
+        <details>
+        <summary>No blank line after summary element breaks task list</summary>
+        1. [ ] People Ops: do such and such
+        </details>
+
+        * [ ] Task 1
+        EOT
+      end
+
       let!(:issue) { create(:issue, description: summary_no_blank_line_markdown, author: user, project: project) }
 
       it 'renders' do
@@ -382,5 +369,31 @@ RSpec.describe 'Task Lists' do
         expect(page).to have_selector('ul input[checked]', count: 1)
       end
     end
+
+    describe 'markdown starting with new line character' do
+      let(:markdown_starting_with_new_line) do
+        <<-EOT.strip_heredoc
+
+        - [ ] Task 1
+        EOT
+      end
+
+      let(:merge_request) { create(:merge_request, description: markdown_starting_with_new_line, author: user, source_project: project) }
+
+      it 'allows the task to be checked' do
+        visit project_merge_request_path(project, merge_request)
+        wait_for_requests
+
+        expect(page).to have_selector('ul input[checked]', count: 0)
+
+        find('.task-list-item-checkbox').click
+        wait_for_requests
+
+        visit project_merge_request_path(project, merge_request)
+        wait_for_requests
+
+        expect(page).to have_selector('ul input[checked]', count: 1)
+      end
+    end
   end
 end