Redmine

Go MAEDA wrote in #note-1:

Thank you for the proposal to add an "Underline" button to the CommonMark Markdown toolbar.

However, I think it is better to use <u> instead of <ins>, as in Redcarpet-based Markdown. This is because <ins> is semantically intended to represent inserted text, so using <ins> only to express underline is not semantically appropriate.

I have updated the patch to use the <u> element accordingly.

Thank you for the correction and explanation.
But, I am a little concerned about the `ins` in the comments, including other text formatting. Why?

I investigated why the comment // ins is used for the "Underline" button in markdown.js.

First, in Textile, which was the only available formatter before Redcarpet-based Markdown was implemented in Redmine 2.5.0, the <ins> tag is used to represent underlined text. Therefore, it makes sense for the comment // ins to be used for the "Underline" button in textile.js (source:trunk/app/assets/javascripts/jstoolbar/textile.js@22937#L29).

However, in Markdown based on Redcarpet, the <u> tag is used for underlined text. Despite this, the comment in markdown.js still says // ins (source:trunk/app/assets/javascripts/jstoolbar/markdown.js@22937#L29). It would be more appropriate for it to say // u. It is likely that when the underline button was added to the Markdown toolbar in r17385, the comment // ins from textile.js was copied over without being changed.

Therefore, I suggest proceeding as follows:

• For the CommonMark Markdown toolbar, follow the implementation of Redcarpet-based Markdown and use the <u> tag to represent underlined text, as I proposed in 40939-v2.patch.
• Change the comment // ins in 40939-v2.patch to // u.
• Change the existing comments in markdown.js to // u to match the HTML output by Redcarpet.

Thank you Mr. Go MAEDA.

I have understood the historical context in which the //ins comment was added.
In addition, I think it would be better to change jsToolBar.prototype.elements.ins to jsToolBar.prototype.elements.u.
What do you think?

diff --git a/app/assets/javascripts/jstoolbar/common_mark.js b/app/assets/javascripts/jstoolbar/common_mark.js
index f9e5ba7..5c3627a 100644
--- a/app/assets/javascripts/jstoolbar/common_mark.js
+++ b/app/assets/javascripts/jstoolbar/common_mark.js
@@ -26,6 +26,16 @@ jsToolBar.prototype.elements.em = {
   }
 }

+// u
+jsToolBar.prototype.elements.u = {
+  type: 'button',
+  title: 'Underline',
+  shortcut: 'u',
+  fn: {
+    wiki: function() { this.singleTag('<u>', '</u>') }
+  }
+}
+
 // del
 jsToolBar.prototype.elements.del = {
   type: 'button',
diff --git a/app/assets/stylesheets/jstoolbar.css b/app/assets/stylesheets/jstoolbar.css
index 6870261..38c70c1 100644
--- a/app/assets/stylesheets/jstoolbar.css
+++ b/app/assets/stylesheets/jstoolbar.css
@@ -113,6 +113,9 @@
 .jstb_ins {
     background-image: url(/jstoolbar/bt_ins.png);
 }
+.jstb_u {
+    background-image: url(/jstoolbar/bt_ins.png);
+}
 .jstb_del {
     background-image: url(/jstoolbar/bt_del.png);
 }

Yasu Saku wrote in #note-6:

In addition, I think it would be better to change jsToolBar.prototype.elements.ins to jsToolBar.prototype.elements.u.

To make that change, probably we need to change textile.js as well as jstoolbar.css. I am not willing to change so many files for a simple change of adding a single button to the CommonMark Markdown toolbar.

Go MAEDA wrote in #note-7:

To make that change, probably we need to change textile.js as well as jstoolbar.css. I am not willing to change so many files for a simple change of adding a single button to the CommonMark Markdown toolbar.

Thank you for your opinion. Understood. I appreciate your assistance.

The InlineAutocompleteSystemTest#test_keyboard_shortcuts_for_wiki_toolbar_buttons_using_markdown test is failing because the system test expectation has not been changed.
I would like to see the expected value changed to “<u></u>” as follows.

diff --git a/test/system/keyboard_shortcuts_test.rb b/test/system/keyboard_shortcuts_test.rb
index 32f5dbe56..361326be1 100644
--- a/test/system/keyboard_shortcuts_test.rb
+++ b/test/system/keyboard_shortcuts_test.rb
@@ -100,7 +100,7 @@ class InlineAutocompleteSystemTest < ApplicationSystemTestCase
       # Clear textarea value
       fill_in 'Description', :with => ''
       find('#issue_description').send_keys([modifier_key, 'u'])
-      assert_equal '__', find('#issue_description').value
+      assert_equal '<u></u>', find('#issue_description').value

       # Clear textarea value
       fill_in 'Description', :with => ''

Mizuki ISHIKAWA wrote in #note-11:

The InlineAutocompleteSystemTest#test_keyboard_shortcuts_for_wiki_toolbar_buttons_using_markdown test is failing because the system test expectation has not been changed.
I would like to see the expected value changed to “<u></u>” as follows.

[...]

Committed the fix in r23160. Thank you for catching and fixing the test failure.