From 83d2b3b7faa76867c6c000b98b1baa90bf16b253 Mon Sep 17 00:00:00 2001 From: Otto Richter Date: Mon, 19 Aug 2024 23:35:37 +0200 Subject: [PATCH] Implement CSS-only input toggling, refactor related forms UX/Translation changes: - new teams: remove redundant tooltips that don't add meaningful information - move general information to table fieldset - new teams: rename "general" to "custom" access for clarity - new teams: show labels beside options on mobile Accessibility: - semantic form elements allow easier navigation (fieldset, mostly) - improve better labelling of new teams table - fix accessibility scan issues - TODO: the parts that "disable" form elements were not yet touched and are not really accessible to screenreaders Technical: - replace two JavaScript solutions with one CSS standard - implement a simpler grid (.simple-grid) - simplify markup - remove some webhook settings specific CSS Testing: - check more form content for accessibility issues - but exclude tooltips from the scan :( - reuse existing form tests from previous PR --- options/locale/locale_en-US.ini | 6 +- templates/org/team/new.tmpl | 103 ++++---- templates/webhook/shared-settings.tmpl | 292 ++++++++-------------- tests/e2e/org-settings.test.e2e.js | 11 +- tests/e2e/repo-settings.test.e2e.js | 13 +- tests/e2e/shared/forms.js | 6 +- web_src/css/form.css | 34 ++- web_src/css/modules/grid.css | 11 + web_src/css/repo.css | 10 - web_src/js/features/comp/WebHookEditor.js | 18 +- web_src/js/features/org-team.js | 13 - web_src/js/index.js | 3 +- 12 files changed, 210 insertions(+), 310 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1764d0ad9e..337a356c95 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2861,13 +2861,11 @@ teams.leave.detail = Are you sure you want to leave team "%s"? teams.can_create_org_repo = Create repositories teams.can_create_org_repo_helper = Members can create new repositories in organization. Creator will get administrator access to the new repository. teams.none_access = No access -teams.none_access_helper = Members cannot view or do any other action on this unit. It has no effect for public repositories. -teams.general_access = General access +teams.none_access_helper = The "no access" option only has effect on private repositories. +teams.general_access = Custom access teams.general_access_helper = Members permissions will be decided by below permission table. teams.read_access = Read -teams.read_access_helper = Members can view and clone team repositories. teams.write_access = Write -teams.write_access_helper = Members can read and push to team repositories. teams.admin_access = Administrator access teams.admin_access_helper = Members can pull and push to team repositories and add collaborators to them. teams.no_desc = This team has no description diff --git a/templates/org/team/new.tmpl b/templates/org/team/new.tmpl index 5c0e00fb9b..1776f5e3ae 100644 --- a/templates/org/team/new.tmpl +++ b/templates/org/team/new.tmpl @@ -56,58 +56,65 @@ {{ctx.Locale.Tr "org.teams.general_access"}} {{ctx.Locale.Tr "org.teams.general_access_helper"}} - - -
- - - - - - - - - - - - {{range $t, $unit := $.Units}} - {{if ge $unit.MaxPerm 2}} - - - - - - +
+ {{ctx.Locale.Tr "org.team_unit_desc"}} + {{ctx.Locale.Tr "org.teams.none_access_helper"}} + +
{{ctx.Locale.Tr "units.unit"}}{{ctx.Locale.Tr "org.teams.none_access"}} - {{svg "octicon-question" 16 "tw-ml-1"}}{{ctx.Locale.Tr "org.teams.read_access"}} - {{svg "octicon-question" 16 "tw-ml-1"}}{{ctx.Locale.Tr "org.teams.write_access"}} - {{svg "octicon-question" 16 "tw-ml-1"}}
- - - - - - - -
+ + + + + + + + + + {{range $t, $unit := $.Units}} + {{if ge $unit.MaxPerm 2}} + + + + + + + {{end}} {{end}} + +
{{ctx.Locale.Tr "units.unit"}}{{ctx.Locale.Tr "org.teams.none_access"}}{{ctx.Locale.Tr "org.teams.read_access"}}{{ctx.Locale.Tr "org.teams.write_access"}}
+ + + + + + + +
+
+ {{range $t, $unit := $.Units}} + {{if lt $unit.MaxPerm 2}} + {{end}} - - -
- {{range $t, $unit := $.Units}} - {{if lt $unit.MaxPerm 2}} - {{end}} - {{end}} +
-
+ {{end}}
diff --git a/templates/webhook/shared-settings.tmpl b/templates/webhook/shared-settings.tmpl index 6639327e46..be3a8755d4 100644 --- a/templates/webhook/shared-settings.tmpl +++ b/templates/webhook/shared-settings.tmpl @@ -2,247 +2,159 @@
{{ctx.Locale.Tr "repo.settings.event_desc"}} -
- -
- -
- -
- -
-
-
+
+ +
+ {{ctx.Locale.Tr "repo.settings.event_header_repository"}} + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- - -
-
-
+ + +
-
-
- - -
- -
- -
-
-
+ + + +
+ {{ctx.Locale.Tr "repo.settings.event_header_issue"}} + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- - -
- -
- -
-
-
+ + + +
+ {{ctx.Locale.Tr "repo.settings.event_header_pull_request"}} + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
- -
-
-
+ + +
-
-
-
+ + + +
diff --git a/tests/e2e/org-settings.test.e2e.js b/tests/e2e/org-settings.test.e2e.js index 4f36954848..5ff0975a26 100644 --- a/tests/e2e/org-settings.test.e2e.js +++ b/tests/e2e/org-settings.test.e2e.js @@ -14,12 +14,11 @@ test('org team settings', async ({browser}, workerInfo) => { 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 expect(page.locator('.hide-unless-checked')).toBeHidden(); await page.locator('input[name="permission"][value="read"]').click(); - await expect(page.locator('.team-units')).toBeVisible(); + await expect(page.locator('.hide-unless-checked')).toBeVisible(); + + // we are validating the form here to include the part that could be hidden + await validate_form({page}); }); diff --git a/tests/e2e/repo-settings.test.e2e.js b/tests/e2e/repo-settings.test.e2e.js index 69034edee5..ac8d37a671 100644 --- a/tests/e2e/repo-settings.test.e2e.js +++ b/tests/e2e/repo-settings.test.e2e.js @@ -14,16 +14,15 @@ test('repo webhook settings', async ({browser}, workerInfo) => { await expect(response?.status()).toBe(200); await page.locator('input[name="events"][value="choose_events"]').click(); - await expect(page.locator('.events.fields')).toBeVisible(); + await expect(page.locator('.hide-unless-checked')).toBeVisible(); + + // check accessibility including the custom events (now visible) part + await validate_form({page}, 'fieldset'); await page.locator('input[name="events"][value="push_only"]').click(); - await expect(page.locator('.events.fields')).toBeHidden(); + await expect(page.locator('.hide-unless-checked')).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'); + await expect(page.locator('.hide-unless-checked')).toBeHidden(); }); test('repo branch protection settings', async ({browser}, workerInfo) => { diff --git a/tests/e2e/shared/forms.js b/tests/e2e/shared/forms.js index 47cd004cd4..37ec797faf 100644 --- a/tests/e2e/shared/forms.js +++ b/tests/e2e/shared/forms.js @@ -3,7 +3,11 @@ import AxeBuilder from '@axe-core/playwright'; export async function validate_form({page}, scope) { scope ??= 'form'; - const accessibilityScanResults = await new AxeBuilder({page}).include(scope).analyze(); + const accessibilityScanResults = await new AxeBuilder({page}) + .include(scope) + // exclude automated tooltips from accessibility scan, remove when fixed + .exclude('span[data-tooltip-content') + .analyze(); expect(accessibilityScanResults.violations).toEqual([]); // assert CSS properties that needed to be overriden for forms (ensure they remain active) diff --git a/web_src/css/form.css b/web_src/css/form.css index 85142daf05..e0e2952080 100644 --- a/web_src/css/form.css +++ b/web_src/css/form.css @@ -12,7 +12,12 @@ fieldset label { display: block; } -form fieldset label .help { +fieldset label:has(input[type="text"]), +fieldset label:has(input[type="number"]) { + font-weight: var(--font-weight-medium); +} + +fieldset .help { font-weight: var(--font-weight-normal); display: block !important; /* overrides another rule in this file, remove when obsolete */ } @@ -23,9 +28,22 @@ fieldset input[type="radio"] { 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); +@media (min-width: 768px) { + .optionmatrix input[type="radio"] { + margin: 0; + } + + /* center columns except first */ + .optionmatrix td + td, .optionmatrix th + th { + min-width: 10em; + text-align: center !important; /* overrides table.css "inherit" rule */ + } +} + +/* if an element with class "hide-unless-checked" follows a label + * that has no checked input, it will be hidden.*/ +label:not(:has(input:checked)) + .hide-unless-checked { + display: none; } .ui.input textarea, @@ -495,14 +513,6 @@ textarea:focus, min-width: 14em; /* matches the default min width */ } -.new.webhook form .help { - margin-left: 25px; -} - -.new.webhook .events.fields .column { - padding-left: 40px; -} - .githook textarea { font-family: var(--fonts-monospace); } diff --git a/web_src/css/modules/grid.css b/web_src/css/modules/grid.css index a2c558047d..1df9a116f7 100644 --- a/web_src/css/modules/grid.css +++ b/web_src/css/modules/grid.css @@ -1,3 +1,14 @@ +.simple-grid { + display: grid; + gap: 1em 2em; +} + +@media (min-width: 30em) { + .simple-grid.grid-2 { + grid-template-columns: repeat(2, 1fr); + } +} + /* based on Fomantic UI grid module, with just the parts extracted that we use. If you find any unused rules here after refactoring, please remove them. */ diff --git a/web_src/css/repo.css b/web_src/css/repo.css index c628ac5e0a..0713bceb90 100644 --- a/web_src/css/repo.css +++ b/web_src/css/repo.css @@ -1805,16 +1805,6 @@ td .commit-summary { font-style: italic; } -.repository.settings.webhook .events .column { - padding-bottom: 0; -} - -.repository.settings.webhook .events .help { - font-size: 13px; - margin-left: 26px; - padding-top: 0; -} - .repository .ui.attached.isSigned.isWarning { border-left: 1px solid var(--color-error-border); border-right: 1px solid var(--color-error-border); diff --git a/web_src/js/features/comp/WebHookEditor.js b/web_src/js/features/comp/WebHookEditor.js index ef40b9f155..522daa1db7 100644 --- a/web_src/js/features/comp/WebHookEditor.js +++ b/web_src/js/features/comp/WebHookEditor.js @@ -1,27 +1,11 @@ import {POST} from '../../modules/fetch.js'; -import {hideElem, showElem, toggleElem} from '../../utils/dom.js'; +import {toggleElem} from '../../utils/dom.js'; export function initCompWebHookEditor() { if (!document.querySelectorAll('.new.webhook').length) { return; } - for (const input of document.querySelectorAll('label.events input')) { - input.addEventListener('change', function () { - if (this.checked) { - showElem('.events.fields'); - } - }); - } - - for (const input of document.querySelectorAll('label.non-events input')) { - input.addEventListener('change', function () { - if (this.checked) { - hideElem('.events.fields'); - } - }); - } - // some webhooks (like Gitea) allow to set the request method (GET/POST), and it would toggle the "Content Type" field const httpMethodInput = document.getElementById('http_method'); if (httpMethodInput) { diff --git a/web_src/js/features/org-team.js b/web_src/js/features/org-team.js index c216fdf6a2..9b059b3a46 100644 --- a/web_src/js/features/org-team.js +++ b/web_src/js/features/org-team.js @@ -1,20 +1,7 @@ import $ from 'jquery'; -import {hideElem, showElem} from '../utils/dom.js'; const {appSubUrl} = window.config; -export function initOrgTeamSettings() { - // Change team access mode - $('.organization.new.team input[name=permission]').on('change', () => { - const val = $('input[name=permission]:checked', '.organization.new.team').val(); - if (val === 'admin') { - hideElem('.organization.new.team .team-units'); - } else { - showElem('.organization.new.team .team-units'); - } - }); -} - export function initOrgTeamSearchRepoBox() { const $searchRepoBox = $('#search-repo-box'); $searchRepoBox.search({ diff --git a/web_src/js/index.js b/web_src/js/index.js index 51ff56fdce..77014a74c0 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -60,7 +60,7 @@ import { initRepoSettingSearchTeamBox, } from './features/repo-settings.js'; import {initRepoDiffView} from './features/repo-diff.js'; -import {initOrgTeamSearchRepoBox, initOrgTeamSettings} from './features/org-team.js'; +import {initOrgTeamSearchRepoBox} from './features/org-team.js'; import {initUserAuthWebAuthn, initUserAuthWebAuthnRegister} from './features/user-auth-webauthn.js'; import {initRepoRelease, initRepoReleaseNew} from './features/repo-release.js'; import {initRepoEditor} from './features/repo-editor.js'; @@ -138,7 +138,6 @@ onDomReady(() => { initNotificationsTable(); initOrgTeamSearchRepoBox(); - initOrgTeamSettings(); initRepoActivityTopAuthorsChart(); initRepoArchiveLinks();