fix(ui): prevent JS from removing icon from close/reopen button (#9959)

Followup to https://codeberg.org/forgejo/forgejo/pulls/9598. A bug surfaced.

When typing text in, JS was updating button's `textContent`, which also affected the icon included in the button. To avoid complex rebuilding of the button element in JS I just placed the text it it's own span, `textContent` of which is now updated by JS instead of the whole button.

Preview

Normal state from template:
https://codeberg.org/attachments/f504bcc3-c1bc-4b10-96ae-e8b666c4a828
https://codeberg.org/attachments/5e72109c-04c7-49b0-ba05-e8f17c949fcb

After editing text, without the fix:
https://codeberg.org/attachments/2c61b02f-b36a-4b80-8816-98bed3fc48e0
https://codeberg.org/attachments/923d4419-97ee-48c6-b60d-9719e36ae6ff

With the fix:
https://codeberg.org/attachments/458c1f6a-549e-4ef7-8822-8529351b7bc4
https://codeberg.org/attachments/839032f7-2f4a-488a-a8a5-d3fbfc45504b

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9959
Reviewed-by: Otto <otto@codeberg.org>

templates/repo/issue/view_content.tmpl

@@ -105,7 +105,9 @@
 										{{if .Issue.IsClosed}}
 											<button id="status-button" class="secondary button" data-status="{{ctx.Locale.Tr "repo.issues.reopen_issue"}}" data-status-and-comment="{{ctx.Locale.Tr "repo.issues.reopen_comment_issue"}}" name="status" value="reopen">
 												{{svg "octicon-issue-reopened"}}
-												{{ctx.Locale.Tr "repo.issues.reopen_issue"}}
+												<span>
+													{{ctx.Locale.Tr "repo.issues.reopen_issue"}}
+												</span>
 											</button>
 										{{else}}
 											{{$closeTranslationKey := "repo.issues.close"}}
@@ -114,7 +116,9 @@
 											{{end}}
 											<button id="status-button" class="secondary button" data-status="{{ctx.Locale.Tr $closeTranslationKey}}" data-status-and-comment="{{ctx.Locale.Tr "repo.issues.close_comment_issue"}}" name="status" value="close">
 												{{svg "octicon-issue-closed"}}
-												{{ctx.Locale.Tr $closeTranslationKey}}
+												<span>
+													{{ctx.Locale.Tr $closeTranslationKey}}
+												</span>
 											</button>
 										{{end}}
 									{{end}}

tests/e2e/issue-comment.test.e2e.ts

@@ -74,6 +74,49 @@ test('Menu accessibility', async ({page}) => {
   await expect(page.getByLabel('user1, user2 reacted laugh. Remove laugh')).toBeVisible();
 });
 
+test.describe('Button text replaced by JS', () => {
+  async function testPage(page, path, closeLabel) {
+    await page.goto(path);
+
+    const statusButton = page.locator('#status-button');
+    const statusButtonIcon = page.locator('#status-button svg');
+    const commentField = page.locator('#comment-form').getByPlaceholder('Leave a comment');
+
+    // Reset issue status before running the test
+    if (await statusButton.getByText('Reopen').isVisible()) await statusButton.click();
+
+    // Assert that normal Close button text is present
+    await expect(statusButton.getByText(closeLabel)).toBeVisible();
+    await expect(statusButtonIcon).toBeVisible();
+
+    // Type in some text to make button text change
+    await commentField.fill('Blah blah');
+    await expect(statusButton.getByText('Close with comment')).toBeVisible();
+    await expect(statusButtonIcon).toBeVisible();
+
+    // Close issue/PR and assert that normal Reopen button text is present
+    await statusButton.click();
+    await expect(statusButton.getByText('Reopen')).toBeVisible();
+    await expect(statusButtonIcon).toBeVisible();
+
+    // Type in some text to make button text change
+    await commentField.fill('Blah blah');
+    await expect(statusButton.getByText('Reopen with comment')).toBeVisible();
+    await expect(statusButtonIcon).toBeVisible();
+
+    return true;
+  }
+
+  test('Issue', async ({page}) => {
+    // All actual expect() are happening in the helper
+    expect(await testPage(page, '/user2/repo1/issues/1', 'Close issue')).toBeTruthy();
+  });
+
+  test('PR', async ({page}) => {
+    expect(await testPage(page, '/user2/repo1/pulls/3', 'Close pull request')).toBeTruthy();
+  });
+});
+
 test('Hyperlink paste behaviour', async ({page}, workerInfo) => {
   test.skip(['Mobile Safari', 'Mobile Chrome', 'webkit'].includes(workerInfo.project.name), 'Mobile clients seem to have very weird behaviour with this test, which I cannot confirm with real usage');
   await page.goto('/user2/repo1/issues/new');

web_src/js/features/repo-issue.js

@@ -745,8 +745,8 @@ export function initSingleCommentEditor($commentForm) {
   const statusButton = document.getElementById('status-button');
   if (statusButton) {
     opts.onContentChanged = (editor) => {
-      const statusText = statusButton.getAttribute(editor.value().trim() ? 'data-status-and-comment' : 'data-status');
-      statusButton.textContent = statusText;
+      const newText = statusButton.getAttribute(editor.value().trim() ? 'data-status-and-comment' : 'data-status');
+      statusButton.querySelector('span').textContent = newText;
     };
   }
   initComboMarkdownEditor($commentForm.find('.combo-markdown-editor'), opts);