Merge branch 'bw-enable-commonmark' into 'master'

enable CommonMark as the default

See merge request gitlab-org/gitlab-ce!19331

app/assets/javascripts/behaviors/markdown/copy_as_gfm.js

@@ -119,7 +119,7 @@ const gfmRules = {
       return el.outerHTML;
     },
     'dl'(el, text) {
-      let lines = text.trim().split('\n');
+      let lines = text.replace(/\n\n/g, '\n').trim().split('\n');
       // Add two spaces to the front of subsequent list items lines,
       // or leave the line entirely blank.
       lines = lines.map((l) => {
@@ -129,9 +129,13 @@ const gfmRules = {
         return `  ${line}`;
       });
 
-      return `<dl>\n${lines.join('\n')}\n</dl>`;
+      return `<dl>\n${lines.join('\n')}\n</dl>\n`;
     },
-    'sub, dt, dd, kbd, q, samp, var, ruby, rt, rp, abbr, summary, details'(el, text) {
+    'dt, dd, summary, details'(el, text) {
+      const tag = el.nodeName.toLowerCase();
+      return `<${tag}>${text}</${tag}>\n`;
+    },
+    'sup, sub, kbd, q, samp, var, ruby, rt, rp, abbr'(el, text) {
       const tag = el.nodeName.toLowerCase();
       return `<${tag}>${text}</${tag}>`;
     },
@@ -215,22 +219,22 @@ const gfmRules = {
       return text.replace(/^- /mg, '1. ');
     },
     'h1'(el, text) {
-      return `# ${text.trim()}`;
+      return `# ${text.trim()}\n`;
     },
     'h2'(el, text) {
-      return `## ${text.trim()}`;
+      return `## ${text.trim()}\n`;
     },
     'h3'(el, text) {
-      return `### ${text.trim()}`;
+      return `### ${text.trim()}\n`;
     },
     'h4'(el, text) {
-      return `#### ${text.trim()}`;
+      return `#### ${text.trim()}\n`;
     },
     'h5'(el, text) {
-      return `##### ${text.trim()}`;
+      return `##### ${text.trim()}\n`;
     },
     'h6'(el, text) {
-      return `###### ${text.trim()}`;
+      return `###### ${text.trim()}\n`;
     },
     'strong'(el, text) {
       return `**${text}**`;
@@ -241,11 +245,13 @@ const gfmRules = {
     'del'(el, text) {
       return `~~${text}~~`;
     },
-    'sup'(el, text) {
-      return `^${text}`;
-    },
     'hr'(el) {
-      return '-----';
+      // extra leading \n is to ensure that there is a blank line between
+      // a list followed by an hr, otherwise this breaks old redcarpet rendering
+      return '\n-----\n';
+    },
+    'p'(el, text) {
+      return `${text.trim()}\n`;
     },
     'table'(el) {
       const theadEl = el.querySelector('thead');
@@ -263,7 +269,9 @@ const gfmRules = {
 
         let before = '';
         let after = '';
-        switch (cell.style.textAlign) {
+        const alignment = cell.align || cell.style.textAlign;
+
+        switch (alignment) {
           case 'center':
             before = ':';
             after = ':';

app/models/concerns/cache_markdown_field.rb

@@ -114,7 +114,7 @@ module CacheMarkdownField
   end
 
   def latest_cached_markdown_version
-    return CacheMarkdownField::CACHE_REDCARPET_VERSION unless cached_markdown_version
+    return CacheMarkdownField::CACHE_COMMONMARK_VERSION unless cached_markdown_version
 
     if cached_markdown_version < CacheMarkdownField::CACHE_COMMONMARK_VERSION_START
       CacheMarkdownField::CACHE_REDCARPET_VERSION

changelogs/unreleased/bw-enable-commonmark.yml

+---
+title: Use CommonMark syntax and rendering for new Markdown content
+merge_request: 19331
+author:
+type: added

doc/user/markdown.md

@@ -3,13 +3,15 @@
 ## GitLab Flavored Markdown (GFM)
 
 > **Note:**
-Not all of the GitLab-specific extensions to Markdown that are described in
-this document currently work on our documentation website.
+> Not all of the GitLab-specific extensions to Markdown that are described in
+> this document currently work on our documentation website.
 >
-For the best result, we encourage you to check this document out as rendered
+> For the best result, we encourage you to check this document out as rendered
 by GitLab: [markdown.md]
 
-_GitLab uses the [Redcarpet Ruby library][redcarpet] for Markdown processing._
+_GitLab uses (as of 11.1) the [CommonMark Ruby Library][commonmarker] for Markdown processing of all new issues, merge requests, comments, and other Markdown content in the GitLab system.  Previous content and Markdown files `.md` in the repositories are still processed using the [Redcarpet Ruby library][redcarpet]._
+
+_Where there are significant differences, we will try to call them out in this document._
 
 GitLab uses "GitLab Flavored Markdown" (GFM). It extends the standard Markdown in a few significant ways to add some useful functionality. It was inspired by [GitHub Flavored Markdown](https://help.github.com/articles/basic-writing-and-formatting-syntax/).
 
@@ -21,7 +23,7 @@ You can use GFM in the following areas:
 - milestones
 - snippets (the snippet must be named with a `.md` extension)
 - wiki pages
-- markdown documents inside the repository
+- markdown documents inside the repository (currently only rendered by Redcarpet)
 
 You can also use other rich text files in GitLab. You might have to install a
 dependency to do so. Please see the [github-markup gem readme](https://github.com/gitlabhq/markup#markups) for more information.
@@ -500,6 +502,7 @@ For example:
 # This header has Unicode in it: 한글
 ## This header has spaces in it
 ### This header has spaces in it
+## This header has 3.5 in it (and parentheses)
 ```
 
 Would generate the following link IDs:
@@ -509,6 +512,7 @@ Would generate the following link IDs:
 1. `this-header-has-unicode-in-it-한글`
 1. `this-header-has-spaces-in-it`
 1. `this-header-has-spaces-in-it-1`
+1. `this-header-has-3-5-in-it-and-parentheses`
 
 Note that the Emoji processing happens before the header IDs are generated, so the Emoji is converted to an image which then gets removed from the ID.
 
@@ -567,7 +571,7 @@ Become:
 + Or pluses
 
 If a list item contains multiple paragraphs,
-each subsequent paragraph should be indented with four spaces.
+each subsequent paragraph should be indented to the same level as the start of the list item text (_Redcarpet: paragraph should be indented with four spaces._)
 
 Example:
 
@@ -575,6 +579,7 @@ Example:
 1. First ordered list item
 
    Second paragraph of first item.
+
 2. Another item
 ```
 
@@ -582,18 +587,20 @@ Becomes:
 
 1.  First ordered list item
 
-    Second paragraph of first item.
+    Paragraph of first item.
+
 2.  Another item
 
-If the second paragraph isn't indented with four spaces,
-the second list item will be incorrectly labeled as `1`.
+If the paragraph of the first item is not indented with the proper number of spaces,
+the paragraph will appear outside the list, instead of properly indented under the list item.
 
 Example:
 
 ```no-highlight
 1. First ordered list item
 
-   Second paragraph of first item.
+  Paragraph of first item.
+
 2. Another item
 ```
 
@@ -601,7 +608,8 @@ Becomes:
 
 1. First ordered list item
 
-   Second paragraph of first item.
+  Paragraph of first item.
+
 2. Another item
 
 ### Links
@@ -719,20 +727,24 @@ Content can be collapsed using HTML's [`<details>`](https://developer.mozilla.or
 <p>
 <details>
 <summary>Click me to collapse/fold.</summary>
-These details will remain hidden until expanded.
+
+These details <em>will</em> remain <strong>hidden</strong> until expanded.
 
 <pre><code>PASTE LOGS HERE</code></pre>
+
 </details>
 </p>
 
-**Note:** Unfortunately Markdown is not supported inside these tags, as described by the [markdown specification](https://daringfireball.net/projects/markdown/syntax#html). You can work around this by using HTML, for example you can use `<pre><code>` tags instead of [code fences](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/user/markdown.md#code-and-syntax-highlighting).
+**Note:** Markdown inside these tags is supported, as long as you have a blank link after the `</summary>` tag and before the `</details>` tag, as shown in the example.  _Redcarpet does not support Markdown inside these tags.  You can work around this by using HTML, for example you can use `<pre><code>` tags instead of [code fences](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/user/markdown.md#code-and-syntax-highlighting)._
 
 ```html
 <details>
 <summary>Click me to collapse/fold.</summary>
-These details will remain hidden until expanded.
 
-<pre><code>PASTE LOGS HERE</code></pre>
+These details _will_ remain **hidden** until expanded.
+
+    PASTE LOGS HERE
+
 </details>
 ```
 
@@ -774,7 +786,7 @@ Underscores
 
 ### Line Breaks
 
-My basic recommendation for learning how line breaks work is to experiment and discover -- hit &lt;Enter&gt; once (i.e., insert one newline), then hit it twice (i.e., insert two newlines), see what happens. You'll soon learn to get what you want. "Markdown Toggle" is your friend.
+A good way to learn how line breaks work is to experiment and discover -- hit <kbd>Enter</kbd> once (i.e., insert one newline), then hit it twice (i.e., insert two newlines), see what happens. You'll soon learn to get what you want. The "Preview" tab is your friend.
 
 Here are some things to try out:
 
@@ -810,7 +822,7 @@ spaces.
 
 ### Tables
 
-Tables aren't part of the core Markdown spec, but they are part of GFM and Markdown Here supports them.
+Tables aren't part of the core Markdown spec, but they are part of GFM.
 
 Example:
 
@@ -828,9 +840,7 @@ Becomes:
 | cell 1   | cell 2   |
 | cell 3   | cell 4   |
 
-**Note**
-
-The row of dashes between the table header and body must have at least three dashes in each column.
+**Note:** The row of dashes between the table header and body must have at least three dashes in each column.
 
 By including colons in the header row, you can align the text within that column.
 
@@ -863,6 +873,18 @@ Becomes:
 
 You can add footnotes to your text as follows.[^2]
 
+### Superscripts / Subscripts
+
+CommonMark and GFM currently do not support the superscript syntax ( `x^2` ) that Redcarpet does.  You can use the standard HTML syntax for superscripts and subscripts.
+
+```
+The formula for water is H<sub>2</sub>O
+while the equation for the theory of relativity is E = mc<sup>2</sup>.
+```
+
+The formula for water is H<sub>2</sub>O while the equation for the theory of relativity is E = mc<sup>2</sup>.
+
+
 ## Wiki-specific Markdown
 
 The following examples show how links inside wikis behave.
@@ -954,3 +976,4 @@ A link starting with a `/` is relative to the wiki root.
 [katex]: https://github.com/Khan/KaTeX "KaTeX website"
 [katex-subset]: https://github.com/Khan/KaTeX/wiki/Function-Support-in-KaTeX "Macros supported by KaTeX"
 [asciidoctor-manual]: http://asciidoctor.org/docs/user-manual/#activating-stem-support "Asciidoctor user manual"
+[commonmarker]: https://github.com/gjtorikian/commonmarker

lib/api/markdown.rb

@@ -10,9 +10,7 @@ module API
         detail "This feature was introduced in GitLab 11.0."
       end
       post do
-        # Explicitly set CommonMark as markdown engine to use.
-        # Remove this set when https://gitlab.com/gitlab-org/gitlab-ce/issues/43011 is done.
-        context = { markdown_engine: :common_mark, only_path: false }
+        context = { only_path: false }
 
         if params[:project]
           project = Project.find_by_full_path(params[:project])

lib/banzai/filter/markdown_filter.rb

@@ -14,7 +14,7 @@ module Banzai
 
       private
 
-      DEFAULT_ENGINE = :redcarpet
+      DEFAULT_ENGINE = :common_mark
 
       def engine(engine_from_context)
         engine_from_context ||= DEFAULT_ENGINE

spec/features/markdown/copy_as_gfm_spec.rb

@@ -502,6 +502,13 @@ describe 'Copy as GFM', :js do
               1. Numbered lists
         GFM
 
+        # list item followed by an HR
+        <<-GFM.strip_heredoc,
+          - list item
+
+          -----
+        GFM
+
         '# Heading',
         '## Heading',
         '### Heading',
@@ -515,8 +522,6 @@ describe 'Copy as GFM', :js do
 
         '~~Strikethrough~~',
 
-        '2^2',
-
         '-----',
 
         # table

spec/features/markdown/markdown_spec.rb

@@ -44,7 +44,7 @@ describe 'GitLab Markdown', :aggregate_failures do
 
   # Shared behavior that all pipelines should exhibit
   shared_examples 'all pipelines' do
-    it 'includes Redcarpet extensions' do
+    it 'includes extensions' do
       aggregate_failures 'does not parse emphasis inside of words' do
         expect(doc.to_html).not_to match('foo<em>bar</em>baz')
       end
@@ -72,10 +72,6 @@ describe 'GitLab Markdown', :aggregate_failures do
       aggregate_failures 'parses strikethroughs' do
         expect(doc).to have_selector(%{del:contains("and this text doesn't")})
       end
-
-      aggregate_failures 'parses superscript' do
-        expect(doc).to have_selector('sup', count: 2)
-      end
     end
 
     it 'includes SanitizationFilter' do
@@ -123,16 +119,24 @@ describe 'GitLab Markdown', :aggregate_failures do
         expect(doc).to have_selector('details summary:contains("collapsible")')
       end
 
-      aggregate_failures 'permits style attribute in th elements' do
-        expect(doc.at_css('th:contains("Header")')['style']).to eq 'text-align: center'
-        expect(doc.at_css('th:contains("Row")')['style']).to eq 'text-align: right'
-        expect(doc.at_css('th:contains("Example")')['style']).to eq 'text-align: left'
+      aggregate_failures 'permits align attribute in th elements' do
+        expect(doc.at_css('th:contains("Header")')['align']).to eq 'center'
+        expect(doc.at_css('th:contains("Row")')['align']).to eq 'right'
+        expect(doc.at_css('th:contains("Example")')['align']).to eq 'left'
       end
 
-      aggregate_failures 'permits style attribute in td elements' do
-        expect(doc.at_css('td:contains("Foo")')['style']).to eq 'text-align: center'
-        expect(doc.at_css('td:contains("Bar")')['style']).to eq 'text-align: right'
-        expect(doc.at_css('td:contains("Baz")')['style']).to eq 'text-align: left'
+      aggregate_failures 'permits align attribute in td elements' do
+        expect(doc.at_css('td:contains("Foo")')['align']).to eq 'center'
+        expect(doc.at_css('td:contains("Bar")')['align']).to eq 'right'
+        expect(doc.at_css('td:contains("Baz")')['align']).to eq 'left'
+      end
+
+      aggregate_failures 'permits superscript elements' do
+        expect(doc).to have_selector('sup', count: 2)
+      end
+
+      aggregate_failures 'permits subscript elements' do
+        expect(doc).to have_selector('sub', count: 3)
       end
 
       aggregate_failures 'removes `rel` attribute from links' do
@@ -320,6 +324,31 @@ describe 'GitLab Markdown', :aggregate_failures do
     end
   end
 
+  context 'Redcarpet documents' do
+    before do
+      allow_any_instance_of(Banzai::Filter::MarkdownFilter).to receive(:engine).and_return('Redcarpet')
+      @html = markdown(@feat.raw_markdown)
+    end
+
+    it 'processes certain elements differently' do
+      aggregate_failures 'parses superscript' do
+        expect(doc).to have_selector('sup', count: 3)
+      end
+
+      aggregate_failures 'permits style attribute in th elements' do
+        expect(doc.at_css('th:contains("Header")')['style']).to eq 'text-align: center'
+        expect(doc.at_css('th:contains("Row")')['style']).to eq 'text-align: right'
+        expect(doc.at_css('th:contains("Example")')['style']).to eq 'text-align: left'
+      end
+
+      aggregate_failures 'permits style attribute in td elements' do
+        expect(doc.at_css('td:contains("Foo")')['style']).to eq 'text-align: center'
+        expect(doc.at_css('td:contains("Bar")')['style']).to eq 'text-align: right'
+        expect(doc.at_css('td:contains("Baz")')['style']).to eq 'text-align: left'
+      end
+    end
+  end
+
   # Fake a `current_user` helper
   def current_user
     @feat.user

spec/features/task_lists_spec.rb

@@ -36,6 +36,19 @@ feature 'Task Lists' do
     MARKDOWN
   end
 
+  let(:nested_tasks_markdown_redcarpet) 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(:nested_tasks_markdown) do
     <<-EOT.strip_heredoc
     - [ ] Task a
@@ -141,13 +154,11 @@ feature 'Task Lists' do
       end
     end
 
-    describe 'nested tasks', :js do
-      let(:issue) { create(:issue, description: nested_tasks_markdown, author: user, project: project) }
-
+    shared_examples 'shared nested tasks' do
       before do
+        allow(Banzai::Filter::MarkdownFilter).to receive(:engine).and_return('Redcarpet')
         visit_issue(project, issue)
       end
-
       it 'renders' do
         expect(page).to have_selector('ul.task-list',      count: 2)
         expect(page).to have_selector('li.task-list-item', count: 7)
@@ -171,6 +182,30 @@ feature 'Task Lists' do
         expect(page).to have_content('marked the task Task 1.1 as complete')
       end
     end
+
+    describe 'nested tasks', :js do
+      context 'with Redcarpet' do
+        let(:issue) { create(:issue, description: nested_tasks_markdown_redcarpet, author: user, project: project) }
+
+        before do
+          allow_any_instance_of(Banzai::Filter::MarkdownFilter).to receive(:engine).and_return('Redcarpet')
+          visit_issue(project, issue)
+        end
+
+        it_behaves_like 'shared nested tasks'
+      end
+
+      context 'with CommonMark' do
+        let(:issue) { create(:issue, description: nested_tasks_markdown, author: user, project: project) }
+
+        before do
+          allow_any_instance_of(Banzai::Filter::MarkdownFilter).to receive(:engine).and_return('CommonMark')
+          visit_issue(project, issue)
+        end
+
+        it_behaves_like 'shared nested tasks'
+      end
+    end
   end
 
   describe 'for Notes' do

spec/fixtures/markdown.md.erb

@@ -43,8 +43,14 @@ This text says this, ~~and this text doesn't~~.
 
 ### Superscript
 
-This is my 1^(st) time using superscript in Markdown. Now this is my
-2^(nd).
+This is my 1<sup>(st)</sup> time using superscript in Markdown. Now this is my
+2<sup>(nd)</sup>.
+
+Redcarpet supports this superscript syntax ( x^2 ).
+
+### Subscript
+
+This (C<sub>6</sub>H<sub>12</sub>O<sub>6</sub>) is an example of subscripts in Markdown.
 
 ### Next step
 

spec/helpers/markup_helper_spec.rb

@@ -298,7 +298,7 @@ describe MarkupHelper do
 
       it 'preserves code color scheme' do
         object = create_object("```ruby\ndef test\n  'hello world'\nend\n```")
-        expected = "\n<pre class=\"code highlight js-syntax-highlight ruby\">" \
+        expected = "<pre class=\"code highlight js-syntax-highlight ruby\">" \
           "<code><span class=\"line\"><span class=\"k\">def</span> <span class=\"nf\">test</span>...</span>\n" \
           "</code></pre>"
 

spec/javascripts/shortcuts_issuable_spec.js

@@ -66,7 +66,7 @@ describe('ShortcutsIssuable', function () {
     });
     describe('with a multi-line selection', () => {
       it('quotes the selected lines as a group', () => {
-        stubSelection('<p>Selected line one.</p>\n\n<p>Selected line two.</p>\n\n<p>Selected line three.</p>');
+        stubSelection('<p>Selected line one.</p>\n<p>Selected line two.</p>\n<p>Selected line three.</p>');
         this.shortcut.replyWithSelectedText(true);
         expect($(this.selector).val()).toBe('> Selected line one.\n>\n> Selected line two.\n>\n> Selected line three.\n\n');
       });

spec/lib/banzai/filter/markdown_filter_spec.rb

@@ -7,13 +7,13 @@ describe Banzai::Filter::MarkdownFilter do
     it 'adds language to lang attribute when specified' do
       result = filter("```html\nsome code\n```")
 
-      expect(result).to start_with("\n<pre><code lang=\"html\">")
+      expect(result).to start_with("<pre><code lang=\"html\">")
     end
 
     it 'does not add language to lang attribute when not specified' do
       result = filter("```\nsome code\n```")
 
-      expect(result).to start_with("\n<pre><code>")
+      expect(result).to start_with("<pre><code>")
     end
   end
 end

spec/models/concerns/cache_markdown_field_spec.rb

@@ -156,7 +156,7 @@ describe CacheMarkdownField do
     end
 
     it { expect(thing.foo_html).to eq(updated_html) }
-    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION) }
+    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
   end
 
   describe '#cached_html_up_to_date?' do
@@ -234,7 +234,7 @@ describe CacheMarkdownField do
 
     it 'returns default version when version is nil' do
       thing.cached_markdown_version = nil
-      is_expected.to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
+      is_expected.to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
     end
   end
 
@@ -261,7 +261,7 @@ describe CacheMarkdownField do
       thing.cached_markdown_version = nil
       thing.refresh_markdown_cache
 
-      expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
+      expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
     end
   end
 
@@ -346,7 +346,7 @@ describe CacheMarkdownField do
 
         expect(thing.foo_html).to eq(updated_html)
         expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
+        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
       end
     end
 
@@ -366,7 +366,7 @@ describe CacheMarkdownField do
 
         expect(thing.foo_html).to eq(updated_html)
         expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
+        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
       end
     end
   end