Skip to content

Commit f63822f

Browse files
authored
Fix autofocus behavior (#34397)
The "autofocus" was abused or misbehaved: 1. When users visit a page but they are not going to change a field, then the field shouldn't get "autofocus" * the "auth" / "user" page: in most cases, users do not want to change the names * see also the GitHub's "settings" page behavior. 2. There shouldn't be duplicate "autofocus" inputs in most cases, only the first one focuses 3. When a panel is shown, the "autofocus" should get focus * "add ssh key" panel This PR fixes all these problems and by the way remove duplicate "isElemHidden" function.
1 parent 71a1187 commit f63822f

File tree

11 files changed

+45
-49
lines changed

11 files changed

+45
-49
lines changed

templates/admin/auth/edit.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
</div>
1616
<div class="required inline field {{if .Err_Name}}error{{end}}">
1717
<label for="auth_name">{{ctx.Locale.Tr "admin.auths.auth_name"}}</label>
18-
<input id="auth_name" name="name" value="{{.Source.Name}}" autofocus required>
18+
<input id="auth_name" name="name" value="{{.Source.Name}}" required>
1919
</div>
2020
<div class="inline field">
2121
<div class="ui checkbox">

templates/admin/user/edit.tmpl

+3-3
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
{{.CsrfTokenHtml}}
1010
<div class="field {{if .Err_UserName}}error{{end}}">
1111
<label for="user_name">{{ctx.Locale.Tr "username"}}</label>
12-
<input id="user_name" name="user_name" value="{{.User.Name}}" autofocus {{if not .User.IsLocal}}disabled{{end}} maxlength="40">
12+
<input id="user_name" name="user_name" value="{{.User.Name}}" {{if not .User.IsLocal}}disabled{{end}} maxlength="40">
1313
</div>
1414
<!-- Types and name -->
1515
<div class="inline required field {{if .Err_LoginType}}error{{end}}">
@@ -55,15 +55,15 @@
5555

5656
<div class="required non-local field {{if .Err_LoginName}}error{{end}} {{if eq .User.LoginSource 0}}tw-hidden{{end}}">
5757
<label for="login_name">{{ctx.Locale.Tr "admin.users.auth_login_name"}}</label>
58-
<input id="login_name" name="login_name" value="{{.User.LoginName}}" autofocus>
58+
<input id="login_name" name="login_name" value="{{.User.LoginName}}">
5959
</div>
6060
<div class="field {{if .Err_FullName}}error{{end}}">
6161
<label for="full_name">{{ctx.Locale.Tr "settings.full_name"}}</label>
6262
<input id="full_name" name="full_name" value="{{.User.FullName}}" maxlength="100">
6363
</div>
6464
<div class="required field {{if .Err_Email}}error{{end}}">
6565
<label for="email">{{ctx.Locale.Tr "email"}}</label>
66-
<input id="email" name="email" type="email" value="{{.User.Email}}" autofocus required>
66+
<input id="email" name="email" type="email" value="{{.User.Email}}" required>
6767
</div>
6868
<div class="local field {{if .Err_Password}}error{{end}} {{if not (or (.User.IsLocal) (.User.IsOAuth2))}}tw-hidden{{end}}">
6969
<label for="password">{{ctx.Locale.Tr "password"}}</label>

templates/org/settings/options.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
<br>{{ctx.Locale.Tr "org.settings.change_orgname_prompt"}}<br>{{ctx.Locale.Tr "org.settings.change_orgname_redirect_prompt"}}
1313
</span>
1414
</label>
15-
<input id="org_name" name="name" value="{{.Org.Name}}" data-org-name="{{.Org.Name}}" autofocus required maxlength="40">
15+
<input id="org_name" name="name" value="{{.Org.Name}}" data-org-name="{{.Org.Name}}" required maxlength="40">
1616
</div>
1717
<div class="field {{if .Err_FullName}}error{{end}}">
1818
<label for="full_name">{{ctx.Locale.Tr "org.org_full_name_holder"}}</label>

templates/repo/settings/collaboration.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@
9090
<form class="ui form" id="repo-collab-team-form" action="{{.Link}}/team" method="post">
9191
{{.CsrfTokenHtml}}
9292
<div id="search-team-box" class="ui search input tw-align-middle" data-org-name="{{.OrgName}}">
93-
<input class="prompt" name="team" placeholder="{{ctx.Locale.Tr "search.team_kind"}}" autocomplete="off" autofocus required>
93+
<input class="prompt" name="team" placeholder="{{ctx.Locale.Tr "search.team_kind"}}" autocomplete="off" required>
9494
</div>
9595
<button class="ui primary button">{{ctx.Locale.Tr "repo.settings.add_team"}}</button>
9696
</form>

templates/repo/settings/options.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
<input type="hidden" name="action" value="update">
1111
<div class="required field {{if .Err_RepoName}}error{{end}}">
1212
<label>{{ctx.Locale.Tr "repo.repo_name"}}</label>
13-
<input name="repo_name" value="{{.Repository.Name}}" data-repo-name="{{.Repository.Name}}" autofocus required>
13+
<input name="repo_name" value="{{.Repository.Name}}" data-repo-name="{{.Repository.Name}}" required>
1414
</div>
1515
<div class="inline field">
1616
<label>{{ctx.Locale.Tr "repo.repo_size"}}</label>

templates/user/settings/profile.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
<span class="text red tw-hidden" id="name-change-prompt"> {{ctx.Locale.Tr "settings.change_username_prompt"}}</span>
1313
<span class="text red tw-hidden" id="name-change-redirect-prompt"> {{ctx.Locale.Tr "settings.change_username_redirect_prompt"}}</span>
1414
</label>
15-
<input id="username" name="name" value="{{.SignedUser.Name}}" data-name="{{.SignedUser.Name}}" autofocus required {{if or (not .SignedUser.IsLocal) ($.UserDisabledFeatures.Contains "change_username") .IsReverseProxy}}disabled{{end}} maxlength="40">
15+
<input id="username" name="name" value="{{.SignedUser.Name}}" data-name="{{.SignedUser.Name}}" required {{if or (not .SignedUser.IsLocal) ($.UserDisabledFeatures.Contains "change_username") .IsReverseProxy}}disabled{{end}} maxlength="40">
1616
{{if or (not .SignedUser.IsLocal) ($.UserDisabledFeatures.Contains "change_username") .IsReverseProxy}}
1717
<p class="help text blue">{{ctx.Locale.Tr "settings.password_username_disabled"}}</p>
1818
{{end}}

web_src/js/features/common-button.ts

+6-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import {POST} from '../modules/fetch.ts';
2-
import {addDelegatedEventListener, hideElem, showElem, toggleElem} from '../utils/dom.ts';
2+
import {addDelegatedEventListener, hideElem, isElemVisible, showElem, toggleElem} from '../utils/dom.ts';
33
import {fomanticQuery} from '../modules/fomantic/base.ts';
44
import {camelize} from 'vue';
55

@@ -79,10 +79,11 @@ function onShowPanelClick(el: HTMLElement, e: MouseEvent) {
7979
// if it has "toggle" class, it toggles the panel
8080
e.preventDefault();
8181
const sel = el.getAttribute('data-panel');
82-
if (el.classList.contains('toggle')) {
83-
toggleElem(sel);
84-
} else {
85-
showElem(sel);
82+
const elems = el.classList.contains('toggle') ? toggleElem(sel) : showElem(sel);
83+
for (const elem of elems) {
84+
if (isElemVisible(elem as HTMLElement)) {
85+
elem.querySelector<HTMLElement>('[autofocus]')?.focus();
86+
}
8687
}
8788
}
8889

web_src/js/features/common-issue-list.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {isElemHidden, onInputDebounce, submitEventSubmitter, toggleElem} from '../utils/dom.ts';
1+
import {isElemVisible, onInputDebounce, submitEventSubmitter, toggleElem} from '../utils/dom.ts';
22
import {GET} from '../modules/fetch.ts';
33

44
const {appSubUrl} = window.config;
@@ -28,7 +28,7 @@ export function parseIssueListQuickGotoLink(repoLink: string, searchText: string
2828
}
2929

3030
export function initCommonIssueListQuickGoto() {
31-
const goto = document.querySelector('#issue-list-quick-goto');
31+
const goto = document.querySelector<HTMLElement>('#issue-list-quick-goto');
3232
if (!goto) return;
3333

3434
const form = goto.closest('form');
@@ -37,7 +37,7 @@ export function initCommonIssueListQuickGoto() {
3737

3838
form.addEventListener('submit', (e) => {
3939
// if there is no goto button, or the form is submitted by non-quick-goto elements, submit the form directly
40-
let doQuickGoto = !isElemHidden(goto);
40+
let doQuickGoto = isElemVisible(goto);
4141
const submitter = submitEventSubmitter(e);
4242
if (submitter !== form && submitter !== input && submitter !== goto) doQuickGoto = false;
4343
if (!doQuickGoto) return;

web_src/js/features/repo-issue-list.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import {updateIssuesMeta} from './repo-common.ts';
2-
import {toggleElem, isElemHidden, queryElems} from '../utils/dom.ts';
2+
import {toggleElem, queryElems, isElemVisible} from '../utils/dom.ts';
33
import {htmlEscape} from 'escape-goat';
44
import {confirmModal} from './comp/ConfirmModal.ts';
55
import {showErrorToast} from '../modules/toast.ts';
@@ -33,8 +33,8 @@ function initRepoIssueListCheckboxes() {
3333
toggleElem('#issue-filters', !anyChecked);
3434
toggleElem('#issue-actions', anyChecked);
3535
// there are two panels but only one select-all checkbox, so move the checkbox to the visible panel
36-
const panels = document.querySelectorAll('#issue-filters, #issue-actions');
37-
const visiblePanel = Array.from(panels).find((el) => !isElemHidden(el));
36+
const panels = document.querySelectorAll<HTMLElement>('#issue-filters, #issue-actions');
37+
const visiblePanel = Array.from(panels).find((el) => isElemVisible(el));
3838
const toolbarLeft = visiblePanel.querySelector('.issue-list-toolbar-left');
3939
toolbarLeft.prepend(issueSelectAll);
4040
};

web_src/js/utils/dom.test.ts

+5-1
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,14 @@ test('createElementFromAttrs', () => {
2525
});
2626

2727
test('querySingleVisibleElem', () => {
28-
let el = createElementFromHTML('<div><span>foo</span></div>');
28+
let el = createElementFromHTML('<div></div>');
29+
expect(querySingleVisibleElem(el, 'span')).toBeNull();
30+
el = createElementFromHTML('<div><span>foo</span></div>');
2931
expect(querySingleVisibleElem(el, 'span').textContent).toEqual('foo');
3032
el = createElementFromHTML('<div><span style="display: none;">foo</span><span>bar</span></div>');
3133
expect(querySingleVisibleElem(el, 'span').textContent).toEqual('bar');
34+
el = createElementFromHTML('<div><span class="some-class tw-hidden">foo</span><span>bar</span></div>');
35+
expect(querySingleVisibleElem(el, 'span').textContent).toEqual('bar');
3236
el = createElementFromHTML('<div><span>foo</span><span>bar</span></div>');
3337
expect(() => querySingleVisibleElem(el, 'span')).toThrowError('Expected exactly one visible element');
3438
});

web_src/js/utils/dom.ts

+20-29
Original file line numberDiff line numberDiff line change
@@ -9,24 +9,24 @@ type ElementsCallback<T extends Element> = (el: T) => Promisable<any>;
99
type ElementsCallbackWithArgs = (el: Element, ...args: any[]) => Promisable<any>;
1010
export type DOMEvent<E extends Event, T extends Element = HTMLElement> = E & { target: Partial<T>; };
1111

12-
function elementsCall(el: ElementArg, func: ElementsCallbackWithArgs, ...args: any[]) {
12+
function elementsCall(el: ElementArg, func: ElementsCallbackWithArgs, ...args: any[]): ArrayLikeIterable<Element> {
1313
if (typeof el === 'string' || el instanceof String) {
1414
el = document.querySelectorAll(el as string);
1515
}
1616
if (el instanceof Node) {
1717
func(el, ...args);
18+
return [el];
1819
} else if (el.length !== undefined) {
1920
// this works for: NodeList, HTMLCollection, Array, jQuery
20-
for (const e of (el as ArrayLikeIterable<Element>)) {
21-
func(e, ...args);
22-
}
23-
} else {
24-
throw new Error('invalid argument to be shown/hidden');
21+
const elems = el as ArrayLikeIterable<Element>;
22+
for (const elem of elems) func(elem, ...args);
23+
return elems;
2524
}
25+
throw new Error('invalid argument to be shown/hidden');
2626
}
2727

28-
export function toggleClass(el: ElementArg, className: string, force?: boolean) {
29-
elementsCall(el, (e: Element) => {
28+
export function toggleClass(el: ElementArg, className: string, force?: boolean): ArrayLikeIterable<Element> {
29+
return elementsCall(el, (e: Element) => {
3030
if (force === true) {
3131
e.classList.add(className);
3232
} else if (force === false) {
@@ -43,23 +43,16 @@ export function toggleClass(el: ElementArg, className: string, force?: boolean)
4343
* @param el ElementArg
4444
* @param force force=true to show or force=false to hide, undefined to toggle
4545
*/
46-
export function toggleElem(el: ElementArg, force?: boolean) {
47-
toggleClass(el, 'tw-hidden', force === undefined ? force : !force);
48-
}
49-
50-
export function showElem(el: ElementArg) {
51-
toggleElem(el, true);
46+
export function toggleElem(el: ElementArg, force?: boolean): ArrayLikeIterable<Element> {
47+
return toggleClass(el, 'tw-hidden', force === undefined ? force : !force);
5248
}
5349

54-
export function hideElem(el: ElementArg) {
55-
toggleElem(el, false);
50+
export function showElem(el: ElementArg): ArrayLikeIterable<Element> {
51+
return toggleElem(el, true);
5652
}
5753

58-
export function isElemHidden(el: ElementArg) {
59-
const res: boolean[] = [];
60-
elementsCall(el, (e) => res.push(e.classList.contains('tw-hidden')));
61-
if (res.length > 1) throw new Error(`isElemHidden doesn't work for multiple elements`);
62-
return res[0];
54+
export function hideElem(el: ElementArg): ArrayLikeIterable<Element> {
55+
return toggleElem(el, false);
6356
}
6457

6558
function applyElemsCallback<T extends Element>(elems: ArrayLikeIterable<T>, fn?: ElementsCallback<T>): ArrayLikeIterable<T> {
@@ -275,14 +268,12 @@ export function initSubmitEventPolyfill() {
275268
document.body.addEventListener('focus', submitEventPolyfillListener);
276269
}
277270

278-
/**
279-
* Check if an element is visible, equivalent to jQuery's `:visible` pseudo.
280-
* Note: This function doesn't account for all possible visibility scenarios.
281-
*/
282-
export function isElemVisible(element: HTMLElement): boolean {
283-
if (!element) return false;
284-
// checking element.style.display is not necessary for browsers, but it is required by some tests with happy-dom because happy-dom doesn't really do layout
285-
return Boolean((element.offsetWidth || element.offsetHeight || element.getClientRects().length) && element.style.display !== 'none');
271+
export function isElemVisible(el: HTMLElement): boolean {
272+
// Check if an element is visible, equivalent to jQuery's `:visible` pseudo.
273+
// This function DOESN'T account for all possible visibility scenarios, its behavior is covered by the tests of "querySingleVisibleElem"
274+
if (!el) return false;
275+
// checking el.style.display is not necessary for browsers, but it is required by some tests with happy-dom because happy-dom doesn't really do layout
276+
return !el.classList.contains('tw-hidden') && Boolean((el.offsetWidth || el.offsetHeight || el.getClientRects().length) && el.style.display !== 'none');
286277
}
287278

288279
// replace selected text in a textarea while preserving editor history, e.g. CTRL-Z works after this

0 commit comments

Comments
 (0)