From c9e402afdc573a9706fa45a1052c122f34264b89 Mon Sep 17 00:00:00 2001 From: Otto Richter Date: Sat, 17 Aug 2024 20:24:31 +0200 Subject: [PATCH] feat(tmpl): Introduce semantic HTML in forms Modifies forms: - (new) org team - (new) repo webhook - (new) repo protected branch The forms are not completely rewritten to semantic HTML yet. The focus of this change was on standard elements, some custom solutions were left untouched for now. - swaps the order fo permission radio buttons as per https://codeberg.org/forgejo/forgejo/issues/4983 - uses fieldsets to group related inputs - ensures consistent styling across forms - can be improved later, e.g. using horizontal lines between sections - fixes: previous font size of labels was smaller than the font size of the help text - help text are now part of the label, clicking them now also activates the input - drop unused CSS (no required checkboxes in grouped class remain) - playwright testing: - move login boilerplate to utils - automated form accessibility checking - allow defining the scope, because legacy parts of the forms are not yet accessible - assert some CSS properties that should not be overriden - the Makefile adjustment was necessary, because eslint scanned some internal files in the tests/e2e/reports directory --- Makefile | 2 +- package-lock.json | 22 ++ package.json | 1 + templates/org/team/new.tmpl | 113 ++++------ templates/repo/settings/protected_branch.tmpl | 199 ++++++++---------- templates/webhook/shared-settings.tmpl | 48 ++--- tests/e2e/issue-sidebar.test.e2e.js | 7 +- tests/e2e/org-settings.test.e2e.js | 25 +++ tests/e2e/repo-settings.test.e2e.js | 37 ++++ tests/e2e/shared/forms.js | 16 ++ tests/e2e/utils_e2e.js | 5 + web_src/css/form.css | 33 ++- web_src/js/features/comp/WebHookEditor.js | 4 +- 13 files changed, 296 insertions(+), 216 deletions(-) create mode 100644 tests/e2e/org-settings.test.e2e.js create mode 100644 tests/e2e/repo-settings.test.e2e.js create mode 100644 tests/e2e/shared/forms.js diff --git a/Makefile b/Makefile index cf4828948a..933e6d37f8 100644 --- a/Makefile +++ b/Makefile @@ -154,7 +154,7 @@ TAR_EXCLUDES := .git data indexers queues log node_modules $(EXECUTABLE) $(FOMAN GO_DIRS := build cmd models modules routers services tests WEB_DIRS := web_src/js web_src/css -ESLINT_FILES := web_src/js tools *.js tests/e2e +ESLINT_FILES := web_src/js tools *.js tests/e2e/*.js tests/e2e/shared/*.js STYLELINT_FILES := web_src/css web_src/js/components/*.vue SPELLCHECK_FILES := $(GO_DIRS) $(WEB_DIRS) docs/content templates options/locale/locale_en-US.ini .github $(wildcard *.go *.js *.md *.yml *.yaml *.toml) diff --git a/package-lock.json b/package-lock.json index 17d4b584bb..3e8e05af17 100644 --- a/package-lock.json +++ b/package-lock.json @@ -60,6 +60,7 @@ "wrap-ansi": "9.0.0" }, "devDependencies": { + "@axe-core/playwright": "4.9.1", "@eslint-community/eslint-plugin-eslint-comments": "4.4.0", "@playwright/test": "1.46.1", "@stoplight/spectral-cli": "6.11.1", @@ -136,6 +137,27 @@ "@types/json-schema": "^7.0.11" } }, + "node_modules/@axe-core/playwright": { + "version": "4.9.1", + "resolved": "https://registry.npmjs.org/@axe-core/playwright/-/playwright-4.9.1.tgz", + "integrity": "sha512-8m4WZbZq7/aq7ZY5IG8GqV+ZdvtGn/iJdom+wBg+iv/3BAOBIfNQtIu697a41438DzEEyptXWmC3Xl5Kx/o9/g==", + "dev": true, + "dependencies": { + "axe-core": "~4.9.1" + }, + "peerDependencies": { + "playwright-core": ">= 1.0.0" + } + }, + "node_modules/@axe-core/playwright/node_modules/axe-core": { + "version": "4.9.1", + "resolved": "https://registry.npmjs.org/axe-core/-/axe-core-4.9.1.tgz", + "integrity": "sha512-QbUdXJVTpvUTHU7871ppZkdOLBeGUKBQWHkHrvN2V9IQWGMt61zf3B45BtzjxEJzYuj0JBjBZP/hmYS/R9pmAw==", + "dev": true, + "engines": { + "node": ">=4" + } + }, "node_modules/@babel/code-frame": { "version": "7.24.7", "resolved": "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.24.7.tgz", diff --git a/package.json b/package.json index 24c05b0d11..340f00d170 100644 --- a/package.json +++ b/package.json @@ -59,6 +59,7 @@ "wrap-ansi": "9.0.0" }, "devDependencies": { + "@axe-core/playwright": "4.9.1", "@eslint-community/eslint-plugin-eslint-comments": "4.4.0", "@playwright/test": "1.46.1", "@stoplight/spectral-cli": "6.11.1", diff --git a/templates/org/team/new.tmpl b/templates/org/team/new.tmpl index 9608eac154..5c0e00fb9b 100644 --- a/templates/org/team/new.tmpl +++ b/templates/org/team/new.tmpl @@ -25,53 +25,40 @@ {{ctx.Locale.Tr "org.team_desc_helper"}} {{if not (eq .Team.LowerName "owners")}} -
- -
-
-
- - - {{ctx.Locale.Tr "org.teams.specific_repositories_helper"}} -
-
-
-
- - - {{ctx.Locale.Tr "org.teams.all_repositories_helper"}} -
-
+
+ {{ctx.Locale.Tr "org.team_access_desc"}} + + -
-
- - - {{ctx.Locale.Tr "org.teams.can_create_org_repo_helper"}} -
-
-
-
- -
-
-
- - - {{ctx.Locale.Tr "org.teams.general_access_helper"}} -
-
-
-
- - - {{ctx.Locale.Tr "org.teams.admin_access_helper"}} -
-
-
-
+ + +
+ {{ctx.Locale.Tr "org.team_permission_desc"}} + + +
-
+
@@ -90,44 +77,36 @@ {{if ge $unit.MaxPerm 2}} {{end}} {{end}}
-
-
- - {{ctx.Locale.Tr $unit.DescKey}} -
-
+ 		-
- -
+ 		-
- -
+ 		-
- -
+
+
{{range $t, $unit := $.Units}} {{if lt $unit.MaxPerm 2}} -
-
- - - {{ctx.Locale.Tr $unit.DescKey}} -
-
+ {{end}} {{end}} +
{{end}} diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index a991e68e6f..89e4c7f971 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -1,52 +1,44 @@ {{template "repo/settings/layout_head" (dict "ctxData" . "pageClass" "repository settings branches")}}
-
-

- {{ctx.Locale.Tr "repo.settings.branch_protection" .Rule.RuleName}} -

-
-
{{ctx.Locale.Tr "repo.settings.protect_patterns"}}
-
- +

+ {{ctx.Locale.Tr "repo.settings.branch_protection" .Rule.RuleName}} +

+ + {{.CsrfTokenHtml}} + +
+ {{ctx.Locale.Tr "repo.settings.protect_patterns"}} +
-
- + {{ctx.Locale.Tr "repo.settings.protect_branch_name_pattern_desc"}} + +
-
- + {{ctx.Locale.Tr "repo.settings.protect_protected_file_patterns_desc"}} + +
+ {{ctx.Locale.Tr "repo.settings.protect_unprotected_file_patterns_desc"}} + + - {{.CsrfTokenHtml}} -
{{ctx.Locale.Tr "repo.settings.event_push"}}
-
-
- - -

{{ctx.Locale.Tr "repo.settings.protect_disable_push_desc"}}

-
-
-
-
- - -

{{ctx.Locale.Tr "repo.settings.protect_enable_push_desc"}}

-
-
-
-
-
- - -

{{ctx.Locale.Tr "repo.settings.protect_whitelist_committers_desc"}}

-
-
+
+ {{ctx.Locale.Tr "repo.settings.event_push"}} + + +
@@ -86,28 +78,25 @@
-
-
-
- - -

{{ctx.Locale.Tr "repo.settings.require_signed_commits_desc"}}

-
-
-
{{ctx.Locale.Tr "repo.settings.event_pull_request_approvals"}}
-
- + + +
+ {{ctx.Locale.Tr "repo.settings.event_pull_request_approvals"}} +
-
-
-
- - -

{{ctx.Locale.Tr "repo.settings.protect_approvals_whitelist_enabled_desc"}}

-
-
+ {{ctx.Locale.Tr "repo.settings.protect_required_approvals_desc"}} + +
+
@@ -141,14 +130,12 @@
{{end}}
-
-
-
- - -

{{ctx.Locale.Tr "repo.settings.dismiss_stale_approvals_desc"}}

-
-
+ +
@@ -156,7 +143,7 @@

{{ctx.Locale.Tr "repo.settings.ignore_stale_approvals_desc"}}

-
+
@@ -188,8 +175,10 @@
-
-
{{ctx.Locale.Tr "repo.settings.event_pull_request_merge"}}
+
+ +
+ {{ctx.Locale.Tr "repo.settings.event_pull_request_merge"}}
@@ -239,41 +228,31 @@ {{end}}
-
-
- - -

{{ctx.Locale.Tr "repo.settings.block_rejected_reviews_desc"}}

-
-
-
-
- - -

{{ctx.Locale.Tr "repo.settings.block_on_official_review_requests_desc"}}

-
-
-
-
- - -

{{ctx.Locale.Tr "repo.settings.block_outdated_branch_desc"}}

-
-
-
{{ctx.Locale.Tr "repo.settings.event_pull_request_enforcement"}}
-
-
- - -

{{ctx.Locale.Tr "repo.settings.enforce_on_admins_desc"}}

-
-
-
- -
- -
-
+ + + +
+
+ {{ctx.Locale.Tr "repo.settings.event_pull_request_enforcement"}} + +
+
{{template "repo/settings/layout_footer" .}} diff --git a/templates/webhook/shared-settings.tmpl b/templates/webhook/shared-settings.tmpl index 80caad7279..6639327e46 100644 --- a/templates/webhook/shared-settings.tmpl +++ b/templates/webhook/shared-settings.tmpl @@ -1,26 +1,20 @@ {{$isNew:=or .PageIsSettingsHooksNew .PageIsAdminDefaultHooksNew .PageIsAdminSystemHooksNew}}
-

{{ctx.Locale.Tr "repo.settings.event_desc"}}

-
-
-
- - -
-
-
-
- - -
-
-
-
- - -
-
-
+
+ {{ctx.Locale.Tr "repo.settings.event_desc"}} + + + +
@@ -270,20 +264,18 @@
-
-
+
+
-
-
+ {{if $isNew}} {{else}} {{ctx.Locale.Tr "repo.settings.delete_webhook"}} {{end}} -
+ {{template "repo/settings/webhook/delete_modal" .}} diff --git a/tests/e2e/issue-sidebar.test.e2e.js b/tests/e2e/issue-sidebar.test.e2e.js index c64cc538c5..f06748c7a3 100644 --- a/tests/e2e/issue-sidebar.test.e2e.js +++ b/tests/e2e/issue-sidebar.test.e2e.js @@ -1,16 +1,11 @@ // @ts-check import {expect} from '@playwright/test'; -import {test, login_user, load_logged_in_context} from './utils_e2e.js'; +import {test, login_user, login} from './utils_e2e.js'; test.beforeAll(async ({browser}, workerInfo) => { await login_user(browser, workerInfo, 'user2'); }); -async function login({browser}, workerInfo) { - const context = await load_logged_in_context(browser, workerInfo, 'user2'); - return await context.newPage(); -} - // belongs to test: Pull: Toggle WIP const prTitle = 'pull5'; diff --git a/tests/e2e/org-settings.test.e2e.js b/tests/e2e/org-settings.test.e2e.js new file mode 100644 index 0000000000..4f36954848 --- /dev/null +++ b/tests/e2e/org-settings.test.e2e.js @@ -0,0 +1,25 @@ +// @ts-check +import {expect} from '@playwright/test'; +import {test, login_user, login} from './utils_e2e.js'; +import {validate_form} from './shared/forms.js'; + +test.beforeAll(async ({browser}, workerInfo) => { + await login_user(browser, workerInfo, 'user2'); +}); + +test('org team settings', async ({browser}, workerInfo) => { + test.skip(workerInfo.project.name === 'Mobile Safari', 'Cannot get it to work - as usual'); + const page = await login({browser}, workerInfo); + const response = await page.goto('/org/org3/teams/team1/edit'); + await expect(response?.status()).toBe(200); + + await page.locator('input[name="permission"][value="admin"]').click(); + await expect(page.locator('.team-units')).toBeHidden(); + + // we are validating the form here, because the now hidden part has accessibility issues anyway + // this should be moved up or down once they are fixed. + await validate_form({page}); + + await page.locator('input[name="permission"][value="read"]').click(); + await expect(page.locator('.team-units')).toBeVisible(); +}); diff --git a/tests/e2e/repo-settings.test.e2e.js b/tests/e2e/repo-settings.test.e2e.js new file mode 100644 index 0000000000..69034edee5 --- /dev/null +++ b/tests/e2e/repo-settings.test.e2e.js @@ -0,0 +1,37 @@ +// @ts-check +import {expect} from '@playwright/test'; +import {test, login_user, login} from './utils_e2e.js'; +import {validate_form} from './shared/forms.js'; + +test.beforeAll(async ({browser}, workerInfo) => { + await login_user(browser, workerInfo, 'user2'); +}); + +test('repo webhook settings', async ({browser}, workerInfo) => { + test.skip(workerInfo.project.name === 'Mobile Safari', 'Cannot get it to work - as usual'); + const page = await login({browser}, workerInfo); + const response = await page.goto('/user2/repo1/settings/hooks/forgejo/new'); + await expect(response?.status()).toBe(200); + + await page.locator('input[name="events"][value="choose_events"]').click(); + await expect(page.locator('.events.fields')).toBeVisible(); + + await page.locator('input[name="events"][value="push_only"]').click(); + await expect(page.locator('.events.fields')).toBeHidden(); + await page.locator('input[name="events"][value="send_everything"]').click(); + await expect(page.locator('.events.fields')).toBeHidden(); + + // restrict to improved semantic HTML, the rest of the page fails the accessibility check + // only execute when the ugly part is hidden - would benefit from refactoring, too + await validate_form({page}, 'fieldset'); +}); + +test('repo branch protection settings', async ({browser}, workerInfo) => { + test.skip(workerInfo.project.name === 'Mobile Safari', 'Cannot get it to work - as usual'); + const page = await login({browser}, workerInfo); + const response = await page.goto('/user2/repo1/settings/branches/edit'); + await expect(response?.status()).toBe(200); + + // not yet accessible :( + // await validate_form({page}, 'fieldset'); +}); diff --git a/tests/e2e/shared/forms.js b/tests/e2e/shared/forms.js new file mode 100644 index 0000000000..47cd004cd4 --- /dev/null +++ b/tests/e2e/shared/forms.js @@ -0,0 +1,16 @@ +import {expect} from '@playwright/test'; +import AxeBuilder from '@axe-core/playwright'; + +export async function validate_form({page}, scope) { + scope ??= 'form'; + const accessibilityScanResults = await new AxeBuilder({page}).include(scope).analyze(); + expect(accessibilityScanResults.violations).toEqual([]); + + // assert CSS properties that needed to be overriden for forms (ensure they remain active) + const boxes = page.getByRole('checkbox').or(page.getByRole('radio')); + for (const b of await boxes.all()) { + await expect(b).toHaveCSS('margin-left', '0px'); + await expect(b).toHaveCSS('margin-top', '0px'); + await expect(b).toHaveCSS('vertical-align', 'baseline'); + } +} diff --git a/tests/e2e/utils_e2e.js b/tests/e2e/utils_e2e.js index 4cc2b17d40..f514fc9c49 100644 --- a/tests/e2e/utils_e2e.js +++ b/tests/e2e/utils_e2e.js @@ -58,6 +58,11 @@ export async function load_logged_in_context(browser, workerInfo, user) { return context; } +export async function login({browser}, workerInfo) { + const context = await load_logged_in_context(browser, workerInfo, 'user2'); + return await context.newPage(); +} + export async function save_visual(page) { // Optionally include visual testing if (process.env.VISUAL_TEST) { diff --git a/web_src/css/form.css b/web_src/css/form.css index 4a5ac8130b..85142daf05 100644 --- a/web_src/css/form.css +++ b/web_src/css/form.css @@ -1,3 +1,33 @@ +fieldset { + margin: 0.5em 0 1em; + padding: 0; +} + +fieldset legend { + font-weight: var(--font-weight-medium); + margin-bottom: 0.75em; +} + +fieldset label { + display: block; +} + +form fieldset label .help { + font-weight: var(--font-weight-normal); + display: block !important; /* overrides another rule in this file, remove when obsolete */ +} + +fieldset input[type="checkbox"], +fieldset input[type="radio"] { + margin-right: 0.75em; + vertical-align: initial !important; /* overrides a semantic.css rule, remove when obsolete */ +} + +fieldset label:has(input[type="text"]), +fieldset label:has(input[type="number"]) { + font-weight: var(--font-weight-medium); +} + .ui.input textarea, .ui.form textarea, .ui.form input:not([type]), @@ -98,8 +128,7 @@ textarea:focus, color: var(--color-text); } -.ui.form .required.fields:not(.grouped) > .field > label::after, -.ui.form .required.fields.grouped > label::after, +.ui.form .required.fields > .field > label::after, .ui.form .required.field > label::after, .ui.form label.required::after { color: var(--color-red); diff --git a/web_src/js/features/comp/WebHookEditor.js b/web_src/js/features/comp/WebHookEditor.js index d74b59fd2a..ef40b9f155 100644 --- a/web_src/js/features/comp/WebHookEditor.js +++ b/web_src/js/features/comp/WebHookEditor.js @@ -6,7 +6,7 @@ export function initCompWebHookEditor() { return; } - for (const input of document.querySelectorAll('.events.checkbox input')) { + for (const input of document.querySelectorAll('label.events input')) { input.addEventListener('change', function () { if (this.checked) { showElem('.events.fields'); @@ -14,7 +14,7 @@ export function initCompWebHookEditor() { }); } - for (const input of document.querySelectorAll('.non-events.checkbox input')) { + for (const input of document.querySelectorAll('label.non-events input')) { input.addEventListener('change', function () { if (this.checked) { hideElem('.events.fields');