Merge branch 'master' into fire-crema-dependency

Conflicts:
	lib/task_list/version.rb

Author: mtodd

app/assets/javascripts/task_lists.coffee

@@ -97,15 +97,29 @@
 incomplete = "[ ]"
 complete   = "[x]"
 
+# Escapes the String for regular expression matching.
+escapePattern = (str) ->
+  str.
+    replace(/([\[\]])/g, "\\$1"). # escape square brackets
+    replace(/\s/, "\\s").         # match all white space
+    replace("x", "[xX]")          # match all cases
+
+incompletePattern = ///
+  #{escapePattern(incomplete)}
+///
+completePattern = ///
+  #{escapePattern(complete)}
+///
+
 # Pattern used to identify all task list items.
 # Useful when you need iterate over all items.
 itemPattern = ///
   ^
   (?:\s*[-+*]|(?:\d+\.))? # optional list prefix
   \s*                     # optional whitespace prefix
   (                       # checkbox
-    #{complete.  replace(/([\[\]])/g, "\\$1")}|
-    #{incomplete.replace(/([\[\]])/g, "\\$1")}
+    #{escapePattern(complete)}|
+    #{escapePattern(incomplete)}
   )
   (?=\s)                  # followed by whitespace
 ///
@@ -127,8 +141,8 @@ codeFencesPattern = ///
 itemsInParasPattern = ///
   ^
   (
-    #{complete.  replace(/[\[\]]/g, "\\$1")}|
-    #{incomplete.replace(/[\[\]]/g, "\\$1")}
+    #{escapePattern(complete)}|
+    #{escapePattern(incomplete)}
   )
   .+
   $
@@ -148,9 +162,9 @@ updateTaskListItem = (source, itemIndex, checked) ->
       if index == itemIndex
         line =
           if checked
-            line.replace(incomplete, complete)
+            line.replace(incompletePattern, complete)
           else
-            line.replace(complete, incomplete)
+            line.replace(completePattern, incomplete)
     line
   result.join("\n")
 

lib/task_list.rb

@@ -24,9 +24,9 @@ def summary
   end
 
   class Item < Struct.new(:checkbox_text, :source)
-    Complete = "[x]".freeze # see TaskListFilter
+    Complete = /\[[xX]\]/.freeze # see TaskList::Filter
     def complete?
-      checkbox_text == Complete
+      checkbox_text =~ Complete
     end
   end
 end

lib/task_list/filter.rb

@@ -31,8 +31,11 @@ def self.filter(*args)
   #   :task_list_items - An array of TaskList::Item objects.
   class Filter < HTML::Pipeline::Filter
 
-    Incomplete  = "[ ]".freeze
-    Complete    = "[x]".freeze
+    Incomplete = "[ ]".freeze
+    Complete   = "[x]".freeze
+
+    IncompletePattern = /\[[[:space:]]\]/.freeze # matches all whitespace
+    CompletePattern   = /\[[xX]\]/.freeze        # matches any capitalization
 
     # Pattern used to identify all task list items.
     # Useful when you need iterate over all items.
@@ -41,26 +44,22 @@ class Filter < HTML::Pipeline::Filter
       (?:\s*[-+*]|(?:\d+\.))? # optional list prefix
       \s*                     # optional whitespace prefix
       (                       # checkbox
-        #{Regexp.escape(Complete)}|
-        #{Regexp.escape(Incomplete)}
+        #{CompletePattern}|
+        #{IncompletePattern}
       )
       (?=\s)                  # followed by whitespace
     /x
 
-    ListSelector = [
-      # select UL/OL
-      ".//li[starts-with(text(),'[ ]')]/..",
-      ".//li[starts-with(text(),'[x]')]/..",
-      # and those wrapped in Ps
-      ".//li/p[1][starts-with(text(),'[ ]')]/../..",
-      ".//li/p[1][starts-with(text(),'[x]')]/../.."
-    ].join(' | ').freeze
+    ListItemSelector = ".//li[task_list_item(.)]".freeze
 
-    # Selects all LIs from a TaskList UL/OL
-    ItemSelector = ".//li".freeze
+    class XPathSelectorFunction
+      def self.task_list_item(nodes)
+        nodes if nodes.text =~ ItemPattern
+      end
+    end
 
     # Selects first P tag of an LI, if present
-    ItemParaSelector = ".//p[1]".freeze
+    ItemParaSelector = "./p[1]".freeze
 
     # List of `TaskList::Item` objects that were recognized in the document.
     # This is available in the result hash as `:task_list_items`.
@@ -100,20 +99,22 @@ def render_task_list_item(item)
     #
     # Returns an Array of Nokogiri::XML::Element objects for ordered and
     # unordered lists.
-    def task_lists
-      doc.xpath(ListSelector)
+    def list_items
+      doc.xpath(ListItemSelector, XPathSelectorFunction)
     end
 
-    # Public: filters a Nokogiri::XML::Element ordered/unordered list, marking
-    # up the list items in order to add behavior and include metadata.
+    # Filters the source for task list items.
     #
-    # Modifies the provided node.
+    # Each item is wrapped in HTML to identify, style, and layer
+    # useful behavior on top of.
+    #
+    # Modifications apply to the parsed document directly.
     #
     # Returns nothing.
-    def filter_list(node)
-      add_css_class(node, 'task-list')
+    def filter!
+      list_items.reverse.each do |li|
+        add_css_class(li.parent, 'task-list')
 
-      node.xpath(ItemSelector).each do |li|
         outer, inner =
           if p = li.xpath(ItemParaSelector)[0]
             [p, p.inner_html]
@@ -122,28 +123,15 @@ def filter_list(node)
           end
         if match = (inner.chomp =~ ItemPattern && $1)
           item = TaskList::Item.new(match, inner)
-          task_list_items << item
+          # prepend because we're iterating in reverse
+          task_list_items.unshift item
 
           add_css_class(li, 'task-list-item')
           outer.inner_html = render_task_list_item(item)
         end
       end
     end
 
-    # Filters the source for task list items.
-    #
-    # Each item is wrapped in HTML to identify, style, and layer
-    # useful behavior on top of.
-    #
-    # Modifications apply to the parsed document directly.
-    #
-    # Returns nothing.
-    def filter!
-      task_lists.each do |node|
-        filter_list node
-      end
-    end
-
     def call
       filter!
       doc
@@ -153,6 +141,7 @@ def call
     # names.
     def add_css_class(node, *new_class_names)
       class_names = (node['class'] || '').split(' ')
+      return if new_class_names.all? { |klass| class_names.include?(klass) }
       class_names.concat(new_class_names)
       node['class'] = class_names.uniq.join(' ')
     end

test/functional/test_task_lists_behavior.html

@@ -74,10 +74,26 @@
           <input type="checkbox" class="task-list-item-checkbox" disabled />
           I'm a task list item
         </li>
+        <li class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled />
+          with non-breaking space
+        </li>
+        <li class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled checked />
+          completed, lower
+        </li>
+        <li class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled checked />
+          completed capitalized
+        </li>
       </ul>
     </div>
     <form action="/update" method="POST" data-remote data-type="json">
-      <textarea name="comment[body]" class="js-task-list-field">- [ ] I'm a task list item</textarea>
+      <textarea name="comment[body]" class="js-task-list-field" cols="40" rows="10">
+- [ ] I'm a task list item
+- [ ] with non-breaking space
+- [x] completed, lower
+- [X] completed capitalized</textarea>
     </form>
   </div>
 

test/task_list/filter_test.rb

@@ -78,6 +78,61 @@ def test_handles_encoding_correctly
     assert_equal unicode, item.text.strip
   end
 
+  def test_handles_nested_items
+    text = <<-md
+- [ ] one
+  - [ ] one.one
+    md
+    assert item = filter(text)[:output].css('.task-list-item .task-list-item').pop
+  end
+
+  def test_handles_complicated_nested_items
+    text = <<-md
+- [ ] one
+  - [ ] one.one
+  - [x] one.two
+    - [ ] one.two.one
+    - [ ] one.two.two
+  - [ ] one.three
+  - [ ] one.four
+- [ ] two
+  - [x] two.one
+  - [ ] two.two
+- [ ] three
+    md
+
+    assert_equal 6 + 2, filter(text)[:output].css('.task-list-item .task-list-item').size
+    assert_equal 2, filter(text)[:output].css('.task-list-item .task-list-item .task-list-item').size
+  end
+
+  # NOTE: This is an edge case experienced regularly by users using a Swiss
+  # German keyboard.
+  # See: https://github.com/github/github/pull/18362
+  def test_non_breaking_space_between_brackets
+    text = "- [\xC2\xA0] ok"
+    assert item = filter(text)[:output].css('.task-list-item').pop, "item expected"
+    assert_equal 'ok', item.text.strip
+  end
+
+  # See: https://github.com/github/github/pull/18362
+  def test_non_breaking_space_between_brackets_in_paras
+    text = <<-md
+- [\xC2\xA0] one
+- [\xC2\xA0] this one will be wrapped in a para
+
+- [\xC2\xA0] this one too, wtf
+    md
+    assert_equal 3, filter(text)[:output].css(@item_selector).size
+  end
+
+  def test_capital_X
+    text = <<-md
+- [x] lower case
+- [X] capital
+    md
+    assert_equal 2, filter(text)[:output].css("[checked]").size
+  end
+
   protected
 
   def filter(input, context = @context, result = nil)

test/unit/test_updates.coffee

@@ -20,19 +20,36 @@ module "TaskList updates",
       disabled: true
       checked: false
 
+    # non-breaking space. See: https://github.com/github/task-lists/pull/14
+    @nbsp = String.fromCharCode(160)
+    @incompleteNBSPItem = $ '<li>', class: 'task-list-item'
+    @incompleteNBSPCheckbox = $ '<input>',
+      type: 'checkbox'
+      class: 'task-list-item-checkbox'
+      disabled: true
+      checked: false
+
     @field = $ '<textarea>', class: 'js-task-list-field', text: """
       - [x] complete
       - [ ] incomplete
+      - [#{@nbsp}] incompleteNBSP
     """
 
     @changes =
       toComplete: """
       - [ ] complete
       - [ ] incomplete
+      - [#{@nbsp}] incompleteNBSP
       """
       toIncomplete: """
       - [x] complete
       - [x] incomplete
+      - [#{@nbsp}] incompleteNBSP
+      """
+      toIncompleteNBSP: """
+      - [x] complete
+      - [ ] incomplete
+      - [x] incompleteNBSP
       """
 
     @completeItem.append @completeCheckbox
@@ -43,6 +60,10 @@ module "TaskList updates",
     @list.append @incompleteItem
     @incompleteItem.expectedIndex = 2
 
+    @incompleteNBSPItem.append @incompleteNBSPCheckbox
+    @list.append @incompleteNBSPItem
+    @incompleteNBSPItem.expectedIndex = 3
+
     @container.append @list
     @container.append @field
 
@@ -79,3 +100,18 @@ asyncTest "updates the source, marking the complete item as incomplete", ->
   , 20
 
   @completeCheckbox.click()
+
+# See: https://github.com/github/task-lists/pull/14
+asyncTest "updates the source for items with non-breaking spaces", ->
+  expect 3
+
+  @field.on 'tasklist:changed', (event, index, checked) =>
+    ok checked
+    equal index, @incompleteNBSPItem.expectedIndex
+    equal @field.val(), @changes.toIncompleteNBSP
+
+  setTimeout ->
+    start()
+  , 20
+
+  @incompleteNBSPCheckbox.click()