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..c6b40ed4 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 { @@ -1626,12 +1631,53 @@ 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 { 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-*` 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 + 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); + --tag-color: var(--text-muted); + --tag-background: transparent; +} + +.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 {