diff --git a/src/ui-mobile/MobileShell.tsx b/src/ui-mobile/MobileShell.tsx index 5df9c93..a4c1891 100644 --- a/src/ui-mobile/MobileShell.tsx +++ b/src/ui-mobile/MobileShell.tsx @@ -57,7 +57,7 @@ import ensoUrl from '../assets/enso.png' import { getStoragePref } from '../bridge/icloud' import { backgroundSyncEnabled, setBackgroundSyncEnabled } from '../bridge/background-sync' import { getPinnedNotes, loadPins } from './pins' -import { vaultSettingsAccessForLayout } from './vault-settings-access' +import { placeVaultSettingsIsland, vaultSettingsAccessForLayout } from './vault-settings-access' import { getLayoutMode, isPhoneViewport, setLayoutMode, type LayoutMode } from '../viewport' import { applySystemBarsPreference, @@ -2436,7 +2436,10 @@ function useVaultSettingsRows(): void { // which reads as the whole card blinking. Moving the same container // back in preserves the live React tree and its state. if (!host) return - if (container?.parentElement === host) return + // contains, not parentElement: on a tablet the container lives in the + // row's button group, and re-placing it on every mutation would fire + // this observer again forever. + if (container && host.contains(container)) return if (!container) { container = document.createElement('div') // The phone wrapper — not the list inside it — is the row's flex child, @@ -2452,18 +2455,7 @@ function useVaultSettingsRows(): void { access === 'quick-switch' ? : ) } - if (access === 'quick-switch') { - // The host is the desktop two-column row (label + "Change…" button), - // which the phone stylesheet wraps into a stack. Mount between the - // label and the row's own buttons, so the card reads location → - // picker list → actions and mobile.css can hide those buttons in - // favor of the island's own action group. - const firstBtn = host.querySelector('button') - if (firstBtn) host.insertBefore(container, firstBtn) - else host.appendChild(container) - } else { - host.appendChild(container) - } + placeVaultSettingsIsland(host, container, access) } const observer = new MutationObserver(() => sync()) observer.observe(document.body, { childList: true, subtree: true }) diff --git a/src/ui-mobile/mobile.css b/src/ui-mobile/mobile.css index 49cd0dc..fc0dadb 100644 --- a/src/ui-mobile/mobile.css +++ b/src/ui-mobile/mobile.css @@ -2130,35 +2130,32 @@ /* ---- Settings vault quick-switch (useVaultSettingsRows) ------------------- */ /* Sits inside the location row (stacked on the phone), between the label and - the row's own buttons — the row's px-5 padding is already around it. */ + the row's own actions; the row's px-5 padding is already around it. */ .zn-settings-vaults-host { flex-basis: 100%; min-width: 100%; } -/* In the tablet settings row, keep only the compact manager entry point. The - wrapper disappears from flex layout so its button sits beside Change… - instead of forcing the phone quick-switch card into the row. */ +/* In the tablet settings row, keep only the compact manager entry point. + placeVaultSettingsIsland puts the wrapper at the end of the row's button + group, and the wrapper disappears from flex layout so its button sits + beside Change… and Quick Connect… and wraps with them (core wraps that + group under the label on its own since 2.58.0). */ .zn-settings-vaults-manage-host { display: contents; } -[data-settings-search-id='vault-location']:has(> .zn-settings-vaults-manage-host) { - flex-wrap: wrap; - row-gap: 0.5rem; -} - .zn-settings-vaults { padding-top: 0.625rem; border-top: 0.5px solid rgb(var(--z-bg-4) / 0.55); } -/* The card's own actions ("Change…", and in a remote workspace its siblings) - are hidden while the island is mounted — the island's action group renders - its own Change… (same store flows) so all actions live in ONE clearly - separated button row under the picker list (Adib: buttons must read apart - from the list). Scoped with :has() so this only applies where the island - actually mounted (phone width). */ +/* The row's own actions (Change…, and in a remote workspace its siblings) + are hidden while the island is mounted: the island's action group carries + New Vault… and Manage…, so all actions live in ONE clearly separated + button row under the picker list (Adib: buttons must read apart from the + list). Scoped with :has() so this only applies where the island actually + mounted (phone layout). */ .zn-mobile [data-zn-settings] div.flex.items-center.justify-between[class*='px-']:has(> .zn-settings-vaults-host) { @@ -2170,7 +2167,12 @@ padding-bottom: 0.625rem; } -[data-settings-search-id='vault-location']:has(> .zn-settings-vaults-host) > button { +/* Everything after the island that is or holds a button is the row's own + actions: bare buttons before core 2.58.0, one wrapping group since (#871). + A rule for bare buttons alone stopped matching when core wrapped them. */ +[data-settings-search-id='vault-location'] + > .zn-settings-vaults-host + ~ :is(button, :has(button)) { display: none; } diff --git a/src/ui-mobile/vault-settings-access.test.ts b/src/ui-mobile/vault-settings-access.test.ts index 444d70d..5414f50 100644 --- a/src/ui-mobile/vault-settings-access.test.ts +++ b/src/ui-mobile/vault-settings-access.test.ts @@ -1,6 +1,10 @@ import assert from 'node:assert/strict' import { describe, it } from 'node:test' -import { vaultSettingsAccessForLayout } from './vault-settings-access.ts' +import { + placeVaultSettingsIsland, + vaultRowActionGroup, + vaultSettingsAccessForLayout +} from './vault-settings-access.ts' describe('vaultSettingsAccessForLayout', () => { it('keeps the full quick-switch controls in the phone settings layout', () => { @@ -11,3 +15,146 @@ describe('vaultSettingsAccessForLayout', () => { assert.equal(vaultSettingsAccessForLayout(false), 'manage-button') }) }) + +// The tests run under plain node, so the row is a small stand-in for the DOM +// that keeps the rule that broke the island: insertBefore throws unless the +// reference node is a direct child. +class FakeElement { + readonly tagName: string + readonly label: string + parentElement: FakeElement | null = null + private kids: FakeElement[] = [] + + constructor(tagName: string, label: string, kids: FakeElement[] = []) { + this.tagName = tagName.toUpperCase() + this.label = label + for (const kid of kids) this.appendChild(kid) + } + + get children(): FakeElement[] { + return [...this.kids] + } + + querySelector(selector: string): FakeElement | null { + assert.equal(selector, 'button') + for (const kid of this.kids) { + if (kid.tagName === 'BUTTON') return kid + const found = kid.querySelector(selector) + if (found) return found + } + return null + } + + appendChild(node: FakeElement): FakeElement { + return this.insertBefore(node, null) + } + + insertBefore(node: FakeElement, ref: FakeElement | null): FakeElement { + if (ref && ref.parentElement !== this) { + throw new Error( + 'NotFoundError: The node before which the new node is to be inserted is not a child of this node.' + ) + } + node.parentElement?.removeChild(node) + const at = ref ? this.kids.indexOf(ref) : this.kids.length + this.kids.splice(at, 0, node) + node.parentElement = this + return node + } + + removeChild(node: FakeElement): void { + this.kids = this.kids.filter((kid) => kid !== node) + node.parentElement = null + } +} + +const el = (tag: string, label: string, kids: FakeElement[] = []): FakeElement => + new FakeElement(tag, label, kids) +const labels = (node: FakeElement): string[] => node.children.map((kid) => kid.label) +const place = (row: FakeElement, island: FakeElement, phone: boolean): void => + placeVaultSettingsIsland( + row as unknown as Element, + island as unknown as Element, + vaultSettingsAccessForLayout(phone) + ) + +// SettingsModal's Vault Location row since core 2.58.0 (#871): the label, +// then ONE wrapping group that holds the buttons. +const coreRow = (buttons: string[]): FakeElement => + el('div', 'row', [ + el('div', 'label', [el('div', 'title'), el('div', 'path')]), + el( + 'div', + 'actions', + buttons.map((name) => el('button', name)) + ) + ]) + +// The same row before 2.58.0: the buttons sat bare in the row. +const bareRow = (buttons: string[]): FakeElement => + el('div', 'row', [el('div', 'label'), ...buttons.map((name) => el('button', name))]) + +const phoneIsland = (): FakeElement => + el('div', 'island', [el('button', 'My Vault'), el('div', 'btns', [el('button', 'New Vault…')])]) + +describe('placeVaultSettingsIsland on the phone', () => { + it('mounts between the label and the wrapped button group', () => { + const row = coreRow(['Change…']) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island', 'actions']) + }) + + it('mounts the same way in a remote workspace, with four buttons', () => { + const row = coreRow([ + 'Change Remote Vault…', + 'Return to Local Vault', + 'Open Local Vault…', + 'Quick Connect…' + ]) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island', 'actions']) + }) + + it('still mounts before bare buttons', () => { + const row = bareRow(['Change…']) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island', 'Change…']) + }) + + it('appends when the row has no buttons at all', () => { + const row = el('div', 'row', [el('div', 'label')]) + const island = phoneIsland() + place(row, island, true) + assert.deepEqual(labels(row), ['label', 'island']) + }) +}) + +describe('placeVaultSettingsIsland on a tablet', () => { + it('joins the button group so Manage… sits beside Change…', () => { + const row = coreRow(['Change…']) + const island = el('div', 'manage', [el('button', 'Manage…')]) + place(row, island, false) + assert.deepEqual(labels(row), ['label', 'actions']) + assert.deepEqual(labels(row.children[1]), ['Change…', 'manage']) + }) + + it('appends to the row itself when the buttons are bare', () => { + const row = bareRow(['Change…']) + const island = el('div', 'manage', [el('button', 'Manage…')]) + place(row, island, false) + assert.deepEqual(labels(row), ['label', 'Change…', 'manage']) + }) +}) + +describe('vaultRowActionGroup', () => { + it("never mistakes the island's own buttons for the row's", () => { + const row = coreRow(['Change…']) + const island = phoneIsland() + row.insertBefore(island, row.children[1]) + const group = vaultRowActionGroup(row as unknown as Element, island as unknown as Element) + assert.equal((group as unknown as FakeElement | null)?.label, 'actions') + }) +}) diff --git a/src/ui-mobile/vault-settings-access.ts b/src/ui-mobile/vault-settings-access.ts index 6779f53..3b4d683 100644 --- a/src/ui-mobile/vault-settings-access.ts +++ b/src/ui-mobile/vault-settings-access.ts @@ -5,3 +5,38 @@ export type VaultSettingsAccess = 'quick-switch' | 'manage-button' export function vaultSettingsAccessForLayout(isPhoneLayout: boolean): VaultSettingsAccess { return isPhoneLayout ? 'quick-switch' : 'manage-button' } + +/** The Vault Location row's own actions: the first direct child of the row + * that is, or holds, a button. Core has shipped both shapes, bare buttons in + * the row before 2.58.0 and one wrapping button group since (#871). Looking + * for a direct child, not the first button, is what keeps the island + * mountable: a nested button is not a child of the row, insertBefore throws + * on it, and the island silently never appeared on the phone. The island is + * skipped so its own buttons never count as the row's. */ +export function vaultRowActionGroup(row: Element, island: Element): Element | null { + for (const child of Array.from(row.children)) { + if (child === island) continue + if (child.tagName === 'BUTTON' || child.querySelector('button')) return child + } + return null +} + +/** Mount a vault island into the Vault Location row. + * + * Phone: between the label and the row's actions, so the card reads + * location, picker list, actions, and mobile.css hides the row's actions + * after the island in favor of the island's own group. + * + * Tablet: at the end of the row's button group, so Manage… sits beside + * Change… instead of wrapping onto a line of its own. With bare buttons the + * row itself is the group. */ +export function placeVaultSettingsIsland( + row: Element, + island: Element, + access: VaultSettingsAccess +): void { + const actions = vaultRowActionGroup(row, island) + if (access === 'quick-switch') row.insertBefore(island, actions) + else if (actions && actions.tagName !== 'BUTTON') actions.appendChild(island) + else row.appendChild(island) +}