From 0476d65a09ceae7d564b0539428ed7e7cef2a131 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:00:01 +0200 Subject: [PATCH 1/2] fix(picker): show empty folders as a dead end before the click, not after it A Multi choice with nothing in it rendered exactly like a folder with content. Selecting it could only answer with a notice after the fact (#1550), and that notice was delivered by closing and re-opening the picker, which discarded the typed query, the scroll position and the selection. The row now says so up front: dimmed name and a trailing "Empty" marker - the same word the settings tree already uses for this state - plus aria-disabled for assistive tech. renderSuggestion is the single render path, so plain level rows and cross-level search results are covered by one branch. Activation is refused in selectSuggestion, above SuggestModal's close(), so the picker simply stays put with its query intact instead of being torn down and rebuilt. Two consequences of no longer closing are handled there: focus is returned to the input (a trusted mousedown on a suggestion row moves focus to , which nothing noticed while every click closed the modal), and key auto-repeat is ignored so a held Enter cannot stack notices. Empty is deliberately not recursive: a folder containing only empty folders still has rows to show, and its children carry the marker one level down. Empty folders are marked, never hidden or down-ranked - a configured folder disappearing from the picker would be a worse, unexplained dead end. Closes #1554 --- src/gui/suggesters/choiceSuggester.test.ts | 240 +++++++++++++++++++-- src/gui/suggesters/choiceSuggester.ts | 108 ++++++++-- src/styles.css | 35 +++ 3 files changed, 346 insertions(+), 37 deletions(-) diff --git a/src/gui/suggesters/choiceSuggester.test.ts b/src/gui/suggesters/choiceSuggester.test.ts index 63433776..1d0f6ac8 100644 --- a/src/gui/suggesters/choiceSuggester.test.ts +++ b/src/gui/suggesters/choiceSuggester.test.ts @@ -602,36 +602,27 @@ describe("ChoiceSuggester", () => { expect(passedChoices.some((c) => c.id === BACK_CHOICE_ID)).toBe(true); }); - // The command/URI path (choiceExecutor.onChooseMultiType) already refuses to - // open an empty folder; the picker used to open a level whose only row was - // "← Back". Both entry points to the same folder now say the same thing — - // and because SuggestModal closes before onChooseItem runs, the picker has - // to be re-opened on the SAME level or the mis-click ejects the user. - it("stays on the current level and says why for an empty folder", () => { + // Backstop only: the picker itself never reaches this, because + // selectSuggestion refuses an empty folder before the modal closes (#1554). + // Programmatic callers still get the notice rather than a level whose only + // row is "← Back". + it("refuses to open an empty folder and says why", () => { const openSpy = vi .spyOn(ChoiceSuggester, "Open") .mockImplementation(() => {}); const empty = multi("Reading", []); - const level = [topNote, empty]; (Notice as unknown as { instances: unknown[] }).instances = []; - makeSuggester(level).onChooseItem(empty, new MouseEvent("click")); + makeSuggester([topNote, empty]).onChooseItem( + empty, + new MouseEvent("click"), + ); expect( (Notice as unknown as { instances: Array<{ message: string }> }) .instances.map((n) => n.message), ).toEqual(['Folder "Reading" is empty.']); - - const [, passedChoices, options] = openSpy.mock.calls[0] as unknown as [ - QuickAdd, - IChoice[], - { placeholder?: string; placeholderStack?: Array }, - ]; - // Same level, same navigation state — no extra Back item is pushed. - expect(passedChoices).toEqual(level); - expect(passedChoices.some((c) => c.id === BACK_CHOICE_ID)).toBe(false); - expect(options.placeholder).toBe("Select a choice"); - expect(options.placeholderStack).toEqual([]); + expect(openSpy).not.toHaveBeenCalled(); }); // ...but Back must still work even when the level it returns to is empty, @@ -652,6 +643,126 @@ describe("ChoiceSuggester", () => { }); }); + // An empty folder is a dead end, and the row now says so before the click + // (#1554). Activation is refused in selectSuggestion — the only seam above + // SuggestModal's close() — so the picker keeps its query and selection. + describe("selectSuggestion (empty folders)", () => { + function activate( + suggester: ChoiceSuggester, + item: IChoice, + evt: MouseEvent | KeyboardEvent = new MouseEvent("click"), + ) { + suggester.selectSuggestion({ item, match: { score: 0, matches: [] } }, evt); + } + + function noticeMessages(): string[] { + return ( + Notice as unknown as { instances: Array<{ message: string }> } + ).instances.map((n) => n.message); + } + + beforeEach(() => { + (Notice as unknown as { instances: unknown[] }).instances = []; + }); + + it("refuses the activation, keeps the picker open, and restores input focus", () => { + const openSpy = vi + .spyOn(ChoiceSuggester, "Open") + .mockImplementation(() => {}); + const empty = multi("Reading", []); + const suggester = makeSuggester([topNote, empty]); + // Real Obsidian's close() and onChooseSuggestion() are one indivisible + // expression, so "onChooseItem did not run" is what actually pins that + // super.selectSuggestion was skipped. + const onChooseItem = vi.spyOn(suggester, "onChooseItem"); + const close = vi.spyOn(suggester, "close"); + const focus = vi.spyOn(suggester.inputEl, "focus"); + + activate(suggester, empty); + + expect(noticeMessages()).toEqual(['Folder "Reading" is empty.']); + expect(onChooseItem).not.toHaveBeenCalled(); + expect(close).not.toHaveBeenCalled(); + expect(openSpy).not.toHaveBeenCalled(); + // A trusted mousedown on the row moves focus to ; nothing closes the + // modal any more, so the input has to be handed focus back. + expect(focus).toHaveBeenCalledTimes(1); + }); + + it("ignores key auto-repeat so a held Enter cannot stack notices", () => { + const empty = multi("Reading", []); + const suggester = makeSuggester([empty]); + + activate(suggester, empty, new KeyboardEvent("keydown")); + for (let i = 0; i < 5; i++) { + activate(suggester, empty, new KeyboardEvent("keydown", { repeat: true })); + } + + expect(noticeMessages()).toEqual(['Folder "Reading" is empty.']); + }); + + it("treats a folder with no choices array (corrupt data.json) as empty", () => { + const broken = { + ...multi("Broken", []), + choices: undefined, + } as unknown as IChoice; + const suggester = makeSuggester([broken]); + const onChooseItem = vi.spyOn(suggester, "onChooseItem"); + + expect(() => activate(suggester, broken)).not.toThrow(); + expect(noticeMessages()).toEqual(['Folder "Broken" is empty.']); + expect(onChooseItem).not.toHaveBeenCalled(); + }); + + it("lets a folder with choices through to the drill-down", () => { + const openSpy = vi + .spyOn(ChoiceSuggester, "Open") + .mockImplementation(() => {}); + const suggester = makeSuggester(rootChoices); + + activate(suggester, work); + + expect(noticeMessages()).toEqual([]); + const [, passedChoices] = openSpy.mock.calls[0] as unknown as [ + QuickAdd, + IChoice[], + ]; + expect(passedChoices.map((c) => c.id)).toEqual([ + ...work.choices.map((c) => c.id), + BACK_CHOICE_ID, + ]); + }); + + it("lets the back row through even when the level it returns to is empty", () => { + const openSpy = vi + .spyOn(ChoiceSuggester, "Open") + .mockImplementation(() => {}); + const back = makeBack([]); + const suggester = makeSuggester([back]); + + activate(suggester, back); + + expect(noticeMessages()).toEqual([]); + const [, passedChoices] = openSpy.mock.calls[0] as unknown as [ + QuickAdd, + IChoice[], + ]; + expect(passedChoices).toEqual([]); + }); + + it("keeps empty folders in the results — they are marked, never hidden", () => { + const empty = multi("Reading", []); + const suggester = makeSuggester([topNote, empty]); + + expect( + suggester.getSuggestions("").map((s) => s.item.id), + ).toContain(empty.id); + expect( + suggester.getSuggestions("read").map((s) => s.item.id), + ).toContain(empty.id); + }); + }); + describe("renderSuggestion", () => { async function render( suggester: ChoiceSuggester, @@ -753,6 +864,97 @@ describe("ChoiceSuggester", () => { ).toBe(false); }); + // #1554: the dead end has to be visible before the click, not only after it. + describe("empty folders", () => { + function flairOf(el: HTMLElement): HTMLElement | null { + return el.querySelector( + ".quickadd-choice-suggestion-content > .quickadd-choice-suggestion-empty-flair", + ); + } + + it("marks an empty folder with a dimming class, aria-disabled and a flair", async () => { + const empty = multi("Reading", []); + const suggester = makeSuggester([topNote, empty]); + + const el = await render(suggester, empty); + + expect( + el.classList.contains("quickadd-choice-suggestion-empty"), + ).toBe(true); + expect(el.getAttribute("aria-disabled")).toBe("true"); + expect(flairOf(el)?.textContent).toBe("Empty"); + }); + + it("marks nested-search results too, keeping the flair last in the row", async () => { + const nestedEmpty = multi("Reading", []); + const parent = multi("Library", [nestedEmpty]); + const suggester = makeSuggester([parent]); + suggester.getSuggestions("read"); + + const el = await render(suggester, nestedEmpty); + + // The breadcrumb branch renders a different subtree; the marker has to + // survive it, since renderSuggestion is the single render path. + expect(el.classList.contains("mod-complex")).toBe(true); + expect(el.querySelector(".suggestion-note")?.textContent).toBe( + "Library", + ); + expect( + el.classList.contains("quickadd-choice-suggestion-empty"), + ).toBe(true); + const row = el.querySelector(".quickadd-choice-suggestion-content"); + expect(row?.lastElementChild).toBe(flairOf(el)); + }); + + it("marks a folder whose choices array is missing (corrupt data.json)", async () => { + const broken = { + ...multi("Broken", []), + choices: undefined, + } as unknown as IChoice; + const suggester = makeSuggester([broken]); + + const el = await render(suggester, broken); + + expect(flairOf(el)?.textContent).toBe("Empty"); + }); + + it("leaves folders with choices, leaf choices and the back row unmarked", async () => { + const back = makeBack([]); + const suggester = makeSuggester([work, topNote, back]); + + for (const item of [work, topNote, back]) { + const el = await render(suggester, item); + expect( + el.classList.contains("quickadd-choice-suggestion-empty"), + ).toBe(false); + expect(el.hasAttribute("aria-disabled")).toBe(false); + expect(flairOf(el)).toBeNull(); + } + }); + + it("clears the marker when a row element is reused for a normal choice", async () => { + const empty = multi("Reading", []); + const suggester = makeSuggester([empty, topNote]); + const el = document.createElement("div"); + + suggester.renderSuggestion( + { item: empty, match: { score: 0, matches: [] } }, + el, + ); + suggester.renderSuggestion( + { item: topNote, match: { score: 0, matches: [] } }, + el, + ); + await Promise.resolve(); + + expect( + el.classList.contains("quickadd-choice-suggestion-empty"), + ).toBe(false); + expect(el.hasAttribute("aria-disabled")).toBe(false); + expect(flairOf(el)).toBeNull(); + }); + }); + it("does not render an icon for the sentinel back item", async () => { const back = makeBack(rootChoices); const suggester = makeSuggester([back]); diff --git a/src/gui/suggesters/choiceSuggester.ts b/src/gui/suggesters/choiceSuggester.ts index 65207cbe..6ba0dc44 100644 --- a/src/gui/suggesters/choiceSuggester.ts +++ b/src/gui/suggesters/choiceSuggester.ts @@ -45,6 +45,13 @@ export function emptyFolderNoticeText(folderName: string): string { return `Folder "${folderName}" is empty.`; } +/** + * Trailing marker on a folder row the picker cannot drill into. Same word the + * settings tree already uses for this exact state ("Empty — add a choice or drag + * one here.", ChoiceList.svelte), so the two surfaces name it the same way. + */ +export const EMPTY_FOLDER_FLAIR = "Empty"; + /** * Sentinel id for the synthetic "New note from template" launcher row (#1023). * Like {@link BACK_CHOICE_ID}, it is navigation/action — never a real choice — @@ -64,6 +71,25 @@ const runTemplateFromFolderLabel = "New note from template…"; */ export const BACK_CHOICE_ID = "quickadd:back"; +/** + * A folder row the picker cannot drill into (#1554): a Multi choice with no + * children. Deliberately NOT recursive — a folder containing only empty folders + * still has rows to show, so calling it empty would be false; its children carry + * the marker one level down. `?.length` rather than `.length === 0` tolerates a + * data.json whose `choices` is missing or not an array, which the loader + * preserves rather than heals. + * + * The Back row is excluded: it is navigation, and the level it wraps is + * legitimately allowed to be empty. + */ +export function isEmptyFolderChoice(choice: IChoice): boolean { + return ( + choice.type === "Multi" && + choice.id !== BACK_CHOICE_ID && + !(choice as IMultiChoice).choices?.length + ); +} + /** * Applied to matches that fall entirely within the breadcrumb prefix, so a * query hitting a folder's name ranks the folder itself and genuine name @@ -270,6 +296,40 @@ export default class ChoiceSuggester extends FuzzySuggestModal { return this.nestedSearchCandidates; } + /** + * Refuses to open an empty folder (#1554), which the row already announces + * with its "Empty" marker. This is the only seam where the activation can be + * refused without ejecting the user: `SuggestModal.selectSuggestion` closes + * the modal *before* `onChooseItem` runs, so returning above `super` is what + * keeps the picker open with its typed query, scroll position and selection + * intact. Both input paths land here — the chooser routes clicks and Enter + * through the owner's `selectSuggestion`. + */ + selectSuggestion( + value: FuzzyMatch, + evt: MouseEvent | KeyboardEvent + ): void { + if (isEmptyFolderChoice(value.item)) { + // OS key auto-repeat: the chooser's Enter binding filters composition but + // not `repeat`, and nothing closes the modal any more, so a held Enter + // would stack one identical notice per repeat. Property read rather than + // `instanceof KeyboardEvent` — a popout window is a separate realm. + if ("repeat" in evt && evt.repeat) return; + new Notice(emptyFolderNoticeText(value.item.name)); + // A trusted mousedown on `.suggestion-item` — a non-focusable div with no + // focusable ancestor, since neither `.modal` nor `.modal-container` carries + // a tabindex — moves focus to . Obsidian never has to handle that + // because every other activation closes the modal; this is the first path + // that keeps it open, so without this the input goes dead while the picker + // still looks alive (arrow keys and Escape keep working, typing does not). + // No-op on the keyboard path, and on mobile it keeps the keyboard up. + this.inputEl.focus(); + return; + } + + super.selectSuggestion(value, evt); + } + renderSuggestion(item: FuzzyMatch, el: HTMLElement): void { el.empty(); el.classList.remove("mod-complex"); @@ -301,6 +361,25 @@ export default class ChoiceSuggester extends FuzzySuggestModal { el.classList.add("quickadd-choice-suggestion-back"); if (item.item.id === RUN_TEMPLATE_FROM_FOLDER_ID) el.classList.add("quickadd-choice-suggestion-run-template"); + + // Say the folder is a dead end before the click, not after it (#1554). The + // row keeps its place in the list — hiding it would make a configured folder + // vanish, which is a worse, unexplained dead end. `el.empty()` above clears + // children but never attributes, so the marker is cleared explicitly for + // rows Obsidian recycles (matching the defensive `mod-complex` reset). + const isEmptyFolder = isEmptyFolderChoice(item.item); + el.classList.toggle("quickadd-choice-suggestion-empty", isEmptyFolder); + if (isEmptyFolder) { + // Not operable, but still focusable and still able to explain itself — + // which is exactly what aria-disabled means, unlike HTML `disabled`. + el.setAttribute("aria-disabled", "true"); + const flair = createOwnedElement(row, "span"); + flair.classList.add("quickadd-choice-suggestion-empty-flair"); + flair.textContent = EMPTY_FOLDER_FLAIR; + row.appendChild(flair); + } else { + el.removeAttribute("aria-disabled"); + } } private renderChoiceIcon(choice: IChoice, parent: HTMLElement): void { @@ -365,31 +444,24 @@ export default class ChoiceSuggester extends FuzzySuggestModal { } private onChooseMultiType(multi: IMultiChoice) { - const choices = [...multi.choices]; const isBack = multi.id === BACK_CHOICE_ID; - // Drilling into an empty folder would open a level whose only row is "← Back". - // The command/URI path already refuses this with a Notice (choiceExecutor - // .onChooseMultiType); say the same thing here so both entry points to the - // same folder behave the same. + // Unreachable from the picker: selectSuggestion refuses empty folders above + // `super`, so the modal never closes and this never dispatches. Kept as a + // backstop for programmatic onChooseItem calls (it is public API on + // FuzzySuggestModal) so any future regression degrades to "notice, nothing + // happens" instead of opening a level whose only row is "← Back". It is also + // what makes the spread below safe on a folder with no `choices` array. // - // SuggestModal.selectSuggestion closes the modal BEFORE onChooseItem runs, - // so returning here would eject the user from the picker entirely. Re-open - // the level they were on (this.choices already carries its Back row, and the - // synthetic template row at the top level) so a mis-click on an empty folder - // costs a notice, not the whole navigation stack. - if (!isBack && choices.length === 0) { + // #1550's close-and-reopen is gone with the ejection it worked around: it + // discarded the typed query, the scroll position and the selection. + if (!isBack && isEmptyFolderChoice(multi)) { new Notice(emptyFolderNoticeText(multi.name)); - ChoiceSuggester.Open(this.plugin, this.choices, { - choiceExecutor: this.choiceExecutor, - focusedProperty: this.focusedProperty, - triggerContext: this.triggerContext, - placeholder: this.currentPlaceholder, - placeholderStack: this.placeholderStack, - }); return; } + const choices = [...multi.choices]; + if (!isBack) { const back = new MultiChoice(backLabel).addChoices(this.choices); back.id = BACK_CHOICE_ID; diff --git a/src/styles.css b/src/styles.css index 11c56cc6..41fb428a 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1609,6 +1609,11 @@ body:not(.is-mobile) align-items: center; gap: 8px; min-width: 0; + /* Breadcrumb rows nest this inside Obsidian's `.suggestion-item.mod-complex` + flex container, where it would otherwise shrink-wrap and strand the trailing + "Empty" marker beside the name instead of at the row's edge. Inert in the + plain level view, where the row is already a full-width block. */ + flex: 1 1 auto; } .quickadd-choice-icon { @@ -1632,6 +1637,36 @@ body:not(.is-mobile) margin: 0; } +/* Empty folders (#1554). A folder with no choices in it cannot be drilled into, + so its row is de-emphasised and labelled before the click instead of only + answering with a notice after it — Obsidian's own dim-but-still-clickable + convention for unresolved links. + + Colour rather than `opacity`: opacity would composite the whole row, marker + included, and cannot be undone on a descendant. The `--link-*` pins are + load-bearing because choice names are markdown-rendered and `a { color: + var(--link-color) }` is global — a folder named `[[Reading]] queue` would + otherwise keep full accent brightness inside a row that claims to be inert. + + `--text-muted`, not `--text-faint`: faint is ~2.3:1 on the light theme. + No `is-selected` override either — the selected row only changes its + background, and un-dimming there would delete the signal at the exact moment + the user is about to press Enter. */ +.quickadd-choice-suggestion-empty { + color: var(--text-muted); + --link-color: var(--text-muted); + --link-unresolved-color: var(--text-muted); + --link-external-color: var(--text-muted); +} + +.quickadd-choice-suggestion-empty-flair { + flex: 0 0 auto; + margin-inline-start: auto; + padding-inline-start: 8px; + color: var(--text-muted); + font-size: var(--font-ui-smaller); +} + /* UpdateModal.ts */ .quickadd-update-modal-container { From 6453c00f6c71c0a91b2bcfb9cfb24a0c5d1f833a Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 26 Jul 2026 23:52:30 +0200 Subject: [PATCH 2/2] fix(picker): keep unbreakable names off the empty-folder marker, and dim tags with the row A choice name with no break opportunities (CamelCase, a URL) overran its box - min-width: 0 shrinks the box, not the glyphs - painting through the trailing "Empty" marker and giving the results list a horizontal scrollbar. That overflow predates this branch; the marker is what makes it visible. The dim also enumerated only the --link-* variables, so a folder named "#projects folder" kept a full-accent tag pill on the one row that is supposed to read as inert - exactly the failure the block's own comment describes for links. --- src/styles.css | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/styles.css b/src/styles.css index 41fb428a..c6b40ed4 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1631,6 +1631,12 @@ body:not(.is-mobile) .quickadd-choice-suggestion-text { min-width: 0; flex: 1 1 auto; + /* A name with no break opportunities (CamelCase, a URL) would otherwise + overrun its box - `min-width: 0` shrinks the box, not the glyphs - painting + through the trailing "Empty" marker and giving the results list a horizontal + scrollbar. `anywhere` rather than `overflow: hidden` + ellipsis, because rows + are allowed to wrap and clipping would hide part of the name. */ + overflow-wrap: anywhere; } .quickadd-choice-suggestion p { @@ -1643,10 +1649,13 @@ body:not(.is-mobile) convention for unresolved links. Colour rather than `opacity`: opacity would composite the whole row, marker - included, and cannot be undone on a descendant. The `--link-*` pins are - load-bearing because choice names are markdown-rendered and `a { color: - var(--link-color) }` is global — a folder named `[[Reading]] queue` would - otherwise keep full accent brightness inside a row that claims to be inert. + included, and cannot be undone on a descendant. The `--link-*` and `--tag-*` + pins are load-bearing because choice names are markdown-rendered and both + `a { color: var(--link-color) }` and `.tag` are global — a folder named + `[[Reading]] queue` or `#projects folder` would otherwise keep full accent + brightness inside a row that claims to be inert. Only constructs Obsidian + themes through variables are pinned; `` and `` keep their theme + defaults rather than being chased with element selectors. `--text-muted`, not `--text-faint`: faint is ~2.3:1 on the light theme. No `is-selected` override either — the selected row only changes its @@ -1657,6 +1666,8 @@ body:not(.is-mobile) --link-color: var(--text-muted); --link-unresolved-color: var(--text-muted); --link-external-color: var(--text-muted); + --tag-color: var(--text-muted); + --tag-background: transparent; } .quickadd-choice-suggestion-empty-flair {