Merge pull request 'Highlight user mention in comments and commit messages' (#5899) from 0ko/forgejo:mention-highlight into forgejo

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

.forgejo/workflows/testing.yml

@@ -122,6 +122,13 @@ jobs:
           USE_REPO_TEST_DIR: 1
           PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: 1
           CHANGED_FILES: ${{steps.changed-files.outputs.all_changed_files}}
+      - name: Upload screenshots on failure
+        if: failure()
+        uses: https://code.forgejo.org/forgejo/upload-artifact@v4
+        with:
+          name: screenshots.zip
+          path: /workspace/forgejo/forgejo/tests/e2e/test-artifacts/*/*.png
+          retention-days: 3
   test-remote-cacher:
     if: vars.ROLE == 'forgejo-coding' || vars.ROLE == 'forgejo-testing'
     runs-on: docker

templates/base/head.tmpl

@@ -26,6 +26,7 @@
 			.ui.secondary.menu .dropdown.item > .menu { margin-top: 0; }
 		</style>
 	</noscript>
+	{{template "shared/user/mention_highlight" .}}
 	{{template "base/head_opengraph" .}}
 	{{template "base/head_style" .}}
 	{{template "custom/header" .}}

templates/shared/user/mention_highlight.tmpl

@@ -0,0 +1,14 @@
+{{if .IsSigned}}
+	<style>
+		.comment,
+		.commit-summary,
+		.commit-body {
+			.mention[href="{{AppSubUrl}}/{{.SignedUser.Name}}" i] {
+				background-color: var(--color-primary-alpha-30);
+				color: var(--color-primary-dark-2);
+				border-radius: 5px;
+				padding: 1px 4px;
+			}
+		}
+	</style>
+{{end}}

tests/e2e/declare_repos_test.go

@@ -5,7 +5,6 @@ package e2e
 
 import (
 	"fmt"
-	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -23,14 +22,31 @@ import (
 
 // first entry represents filename
 // the following entries define the full file content over time
-type FileChanges [][]string
+type FileChanges struct {
+	Filename  string
+	CommitMsg string
+	Versions  []string
+}
 
 // put your Git repo declarations in here
 // feel free to amend the helper function below or use the raw variant directly
 func DeclareGitRepos(t *testing.T) func() {
 	cleanupFunctions := []func(){
-		newRepo(t, 2, "diff-test", FileChanges{
+		newRepo(t, 2, "diff-test", []FileChanges{{
-			{"testfile", "hello", "hallo", "hola", "native", "ubuntu-latest", "- runs-on: ubuntu-latest", "- runs-on: debian-latest"},
+			Filename: "testfile",
+			Versions: []string{"hello", "hallo", "hola", "native", "ubuntu-latest", "- runs-on: ubuntu-latest", "- runs-on: debian-latest"},
+		}}),
+		newRepo(t, 2, "mentions-highlighted", []FileChanges{
+			{
+				Filename:  "history1.md",
+				Versions:  []string{""},
+				CommitMsg: "A commit message which mentions @user2 in the title\nand has some additional text which mentions @user1",
+			},
+			{
+				Filename:  "history2.md",
+				Versions:  []string{""},
+				CommitMsg: "Another commit which mentions @user1 in the title\nand @user2 in the text",
+			},
 		}),
 		// add your repo declarations here
 	}
@@ -42,7 +58,7 @@ func DeclareGitRepos(t *testing.T) func() {
 	}
 }
 
-func newRepo(t *testing.T, userID int64, repoName string, fileChanges FileChanges) func() {
+func newRepo(t *testing.T, userID int64, repoName string, fileChanges []FileChanges) func() {
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userID})
 	somerepo, _, cleanupFunc := tests.CreateDeclarativeRepo(t, user, repoName,
 		[]unit_model.Type{unit_model.TypeCode, unit_model.TypeIssues}, nil,
@@ -50,19 +66,25 @@ func newRepo(t *testing.T, userID int64, repoName string, fileChanges FileChange
 	)
 
 	for _, file := range fileChanges {
-		changeLen := len(file)
+		for i, version := range file.Versions {
-		for i := 1; i < changeLen; i++ {
+			operation := "update"
-			operation := "create"
+			if i == 0 {
-			if i != 1 {
+				operation = "create"
-				operation = "update"
 			}
+
+			// default to unique commit messages
+			commitMsg := file.CommitMsg
+			if commitMsg == "" {
+				commitMsg = fmt.Sprintf("Patch: %s-%d", file.Filename, i+1)
+			}
+
 			resp, err := files_service.ChangeRepoFiles(git.DefaultContext, somerepo, user, &files_service.ChangeRepoFilesOptions{
 				Files: []*files_service.ChangeRepoFile{{
 					Operation:     operation,
-					TreePath:      file[0],
+					TreePath:      file.Filename,
-					ContentReader: strings.NewReader(file[i]),
+					ContentReader: strings.NewReader(version),
 				}},
-				Message:   fmt.Sprintf("Patch: %s-%s", file[0], strconv.Itoa(i)),
+				Message:   commitMsg,
 				OldBranch: "main",
 				NewBranch: "main",
 				Author: &files_service.IdentityOptions{

tests/e2e/repo-code.test.e2e.ts

@@ -5,7 +5,12 @@
 // @watch end
 
 import {expect} from '@playwright/test';
-import {test} from './utils_e2e.ts';
+import {test, login_user, login} from './utils_e2e.ts';
+import {accessibilityCheck} from './shared/accessibility.ts';
+
+test.beforeAll(async ({browser}, workerInfo) => {
+  await login_user(browser, workerInfo, 'user2');
+});
 
 async function assertSelectedLines(page, nums) {
   const pageAssertions = async () => {
@@ -75,3 +80,19 @@ test('Readable diff', async ({page}, workerInfo) => {
     }
   }
 });
+
+test('Username highlighted in commits', async ({browser}, workerInfo) => {
+  const page = await login({browser}, workerInfo);
+  await page.goto('/user2/mentions-highlighted/commits/branch/main');
+  // check first commit
+  await page.getByRole('link', {name: 'A commit message which'}).click();
+  await expect(page.getByRole('link', {name: '@user2'})).toHaveCSS('background-color', /(.*)/);
+  await expect(page.getByRole('link', {name: '@user1'})).toHaveCSS('background-color', 'rgba(0, 0, 0, 0)');
+  await accessibilityCheck({page}, ['.commit-header'], [], []);
+  // check second commit
+  await page.goto('/user2/mentions-highlighted/commits/branch/main');
+  await page.locator('tbody').getByRole('link', {name: 'Another commit which mentions'}).click();
+  await expect(page.getByRole('link', {name: '@user2'})).toHaveCSS('background-color', /(.*)/);
+  await expect(page.getByRole('link', {name: '@user1'})).toHaveCSS('background-color', 'rgba(0, 0, 0, 0)');
+  await accessibilityCheck({page}, ['.commit-header'], [], []);
+});

tests/e2e/shared/accessibility.ts

@@ -0,0 +1,35 @@
+import {expect, type Page} from '@playwright/test';
+import {AxeBuilder} from '@axe-core/playwright';
+
+export async function accessibilityCheck({page}: {page: Page}, includes: string[], excludes: string[], disabledRules: string[]) {
+  // contrast of inline links is still a global issue in Forgejo
+  disabledRules += 'link-in-text-block';
+
+  let accessibilityScanner = await new AxeBuilder({page})
+    .disableRules(disabledRules);
+  // passing the whole array seems to be not supported,
+  // iterating has the nice side-effectof skipping this if the array is empty
+  for (const incl of includes) {
+    // passing the whole array seems to be not supported
+    accessibilityScanner = accessibilityScanner.include(incl);
+  }
+  for (const excl of excludes) {
+    accessibilityScanner = accessibilityScanner.exclude(excl);
+  }
+
+  // scan the page both in dark and light theme
+  let accessibilityScanResults = await accessibilityScanner.analyze();
+  expect(accessibilityScanResults.violations).toEqual([]);
+  await page.emulateMedia({colorScheme: 'dark'});
+  // in https://codeberg.org/forgejo/forgejo/pulls/5899 there have been
+  // some weird failures related to contrast scanning,
+  // reporting for colours that haven't been used and no trace in the
+  // screenshots.
+  // Since this was only happening with some browsers and not always,
+  // my bet is on a transition effect on dark/light mode switch.
+  // Waiting a little seems to work around this.
+  await page.waitForTimeout(100); // eslint-disable-line playwright/no-wait-for-timeout
+  accessibilityScanResults = await accessibilityScanner.analyze();
+  expect(accessibilityScanResults.violations).toEqual([]);
+  await page.emulateMedia({colorScheme: 'light'});
+}

tests/e2e/shared/forms.ts

@@ -1,17 +1,14 @@
 import {expect, type Page} from '@playwright/test';
-import {AxeBuilder} from '@axe-core/playwright';
+import {accessibilityCheck} from './accessibility.ts';
 
 export async function validate_form({page}: {page: Page}, scope: 'form' | 'fieldset' = 'form') {
-  const accessibilityScanResults = await new AxeBuilder({page})
+  const excludedElements = [
-    // disable checking for link style - should be fixed, but not now
-    .disableRules('link-in-text-block')
-    .include(scope)
     // exclude automated tooltips from accessibility scan, remove when fixed
-    .exclude('span[data-tooltip-content')
+    'span[data-tooltip-content',
     // exclude weird non-semantic HTML disabled content
-    .exclude('.disabled')
+    '.disabled',
-    .analyze();
+  ];
-  expect(accessibilityScanResults.violations).toEqual([]);
+  await accessibilityCheck({page}, [scope], excludedElements, []);
 
   // assert CSS properties that needed to be overriden for forms (ensure they remain active)
   const boxes = page.getByRole('checkbox').or(page.getByRole('radio'));

tests/integration/mention_test.go

@@ -0,0 +1,21 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package integration
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestHeadMentionCSS(t *testing.T) {
+	userSession := loginUser(t, "user2")
+	resp := userSession.MakeRequest(t, NewRequest(t, "GET", "/"), http.StatusOK)
+	assert.Contains(t, resp.Body.String(), `.mention[href="/user2" i]`)
+
+	guestSession := emptyTestSession(t)
+	resp = guestSession.MakeRequest(t, NewRequest(t, "GET", "/"), http.StatusOK)
+	assert.NotContains(t, resp.Body.String(), `.mention[href="`)
+}