Skip to content

Commit a168cd8

Browse files
stariy95stariy95
committed
fix(ui): refcount the modal body-scroll lock
1 parent b1315a5 commit a168cd8

6 files changed

Lines changed: 52 additions & 32 deletions

File tree

‎src/main/assets/src/App.vue‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ function reloadPage() {
1616
</script>
1717

1818
<!--
19-
/issue and /pr resolve to the same component, so without a key Vue reuses the instance across them
20-
and onMounted never re-fires - the new route would render the previous route's rows.
19+
/issues and /pull-requests resolve to the same component, so without a key Vue reuses the instance
20+
across them and onMounted never re-fires - the new route would render the previous route's rows.
2121
-->
2222

2323
<template>

‎src/main/assets/src/components/AppModal.vue‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,31 @@ const emit = defineEmits<{ close: [] }>()
1616
1717
const dialog = ref<HTMLElement | null>(null)
1818
19+
/**
20+
* Refcounted, because several modals are mounted at once - a page can hold two ConfirmModals and a
21+
* LogModal, and every table row with a result mounts its own. Releasing unconditionally let a modal
22+
* that had never been open clear the lock a genuinely open one was holding, so the page scrolled
23+
* behind the dialog.
24+
*/
25+
let bodyLocks = 0
26+
27+
function lockBody() {
28+
if (++bodyLocks === 1) {
29+
document.body.classList.add('modal-open')
30+
document.body.style.overflow = 'hidden'
31+
}
32+
}
33+
34+
function unlockBody() {
35+
if (--bodyLocks === 0) {
36+
document.body.classList.remove('modal-open')
37+
document.body.style.overflow = ''
38+
}
39+
}
40+
41+
// whether THIS instance currently holds a lock, so it only ever releases its own
42+
let locked = false
43+
1944
// so closing returns the keyboard to whatever opened the modal, rather than to <body>
2045
let previouslyFocused: HTMLElement | null = null
2146
@@ -29,14 +54,19 @@ function onKeydown(event: KeyboardEvent) {
2954
watch(
3055
() => props.open,
3156
async (open) => {
32-
document.body.classList.toggle('modal-open', open)
33-
document.body.style.overflow = open ? 'hidden' : ''
57+
if (open === locked) {
58+
return
59+
}
60+
locked = open
61+
3462
if (open) {
63+
lockBody()
3564
previouslyFocused = document.activeElement as HTMLElement | null
3665
document.addEventListener('keydown', onKeydown)
3766
await nextTick()
3867
dialog.value?.focus()
3968
} else {
69+
unlockBody()
4070
document.removeEventListener('keydown', onKeydown)
4171
previouslyFocused?.focus()
4272
previouslyFocused = null
@@ -48,8 +78,10 @@ watch(
4878
4979
onBeforeUnmount(() => {
5080
document.removeEventListener('keydown', onKeydown)
51-
document.body.classList.remove('modal-open')
52-
document.body.style.overflow = ''
81+
if (locked) {
82+
locked = false
83+
unlockBody()
84+
}
5385
})
5486
</script>
5587

‎src/main/assets/src/composables/useAgrestList.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ export function useAgrestList<T>(options: AgrestListOptions<T>) {
2626
} = useAsyncResource({
2727
initial: { items: [] as T[], total: 0 },
2828
warmCache: true,
29-
// the onMounted below and the route watcher own the first load between them
29+
// onMounted below owns the first load; the route watcher owns every later one
3030
immediate: false,
3131
load: async () => {
3232
const response = await options.fetch({

‎src/main/assets/src/composables/useAsyncResource.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,21 @@ import { onMounted, ref, shallowRef, type Ref, type ShallowRef } from 'vue'
22
import { errorMessage } from '@/api/http'
33
import { useCacheWarmup } from './useCacheWarmup'
44

5-
export interface AsyncResourceOptions<T, A extends unknown[]> {
5+
export type AsyncResourceOptions<T, A extends unknown[]> = {
66
/** Value before the first successful load. */
77
initial: T
88
load: (...args: A) => Promise<T>
99
/** Wait for the GitHub import to populate the local database first. Default false. */
1010
warmCache?: boolean
11-
/**
12-
* Load once from onMounted, with no arguments. Default true, so a loader that *requires* an
13-
* argument must pass false and drive the first load itself.
14-
*/
15-
immediate?: boolean
16-
}
11+
} & ([] extends A
12+
? {
13+
/** Load once from onMounted, with no arguments. Default true. */
14+
immediate?: boolean
15+
}
16+
: {
17+
/** Required here: the loader takes an argument, so it cannot be called on mount. */
18+
immediate: false
19+
})
1720

1821
export interface AsyncResource<T, A extends unknown[]> {
1922
/** Shallow: replaced wholesale on every load, never mutated in place. */

‎src/main/assets/src/composables/useModuleSelection.ts‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { computed, type Ref, ref } from 'vue'
22

33
/**
4-
* Checkbox selection over a list of modules, tracked by repository name.
5-
* Selection survives the row list being replaced, and drops names that are no longer present.
4+
* Checkbox selection over a list of modules, tracked by repository name rather than by row object,
5+
* so it survives the row list being replaced on reload.
66
*/
77
export function useModuleSelection(names: Ref<string[]>) {
88
const selected = ref<string[]>([])
@@ -18,20 +18,5 @@ export function useModuleSelection(names: Ref<string[]>) {
1818
selected,
1919
allSelected,
2020
hasSelection: computed(() => selected.value.length > 0),
21-
22-
isSelected: (name: string) => selected.value.includes(name),
23-
24-
toggle(name: string): void {
25-
const index = selected.value.indexOf(name)
26-
if (index === -1) {
27-
selected.value.push(name)
28-
} else {
29-
selected.value.splice(index, 1)
30-
}
31-
},
32-
33-
clear(): void {
34-
selected.value = []
35-
},
3621
}
3722
}

‎src/main/assets/src/views/IssueListView.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { listIssues, listPullRequests } from '@/api/issues'
1616
import type { OpenIssue } from '@/api/types'
1717
1818
const route = useRoute()
19-
// one component serves both /issue and /pr; they differ only by source and the milestone column
19+
// one component serves both routes; they differ only by source and the milestone column
2020
const isIssues = computed(() => route.meta.kind !== 'pr')
2121
2222
const {

0 commit comments

Comments
 (0)