diff --git a/README.md b/README.md index 5a143be..c10c8a4 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,13 @@ attached to the most recent release about once a day and upgrades in place; there is nothing to re-download by hand. Open a compose window and the button appears in the format toolbar. +Code blocks go into **HTML compose windows only**. Thunderbird hides the format +toolbar in a plain-text composer, and the button lives in that toolbar, so there +is nothing to click - and the add-on keeps its context-menu item out of a +plain-text composer for the same reason, rather than offering an insert it +cannot carry out. Which editor a composer gets is the account's own setting, +and holding Shift as you start a message opens the other one for that message. + ## Building the archive You do not need this to use the add-on - releases are built by CI from a tag. @@ -191,14 +198,14 @@ console. That is `tests/thunderbird/insertion.test.js`, and every assertion in it used to be a line on the release checklist. The harness itself is `tests/thunderbird/harness/`, and its interface is -documented in `tests/thunderbird/harness/index.js` - including four limits -found while building it, which are worth reading before writing a test that -runs into them. The popup's document cannot be read from outside; the popup has +documented in `tests/thunderbird/harness/index.js` - including four things +worth reading before writing a test that runs into them. The popup's document cannot be read from outside; the popup has to be handed the keyboard before it hears anything, and a test that forgets can pass while asserting nothing; a letter-key shortcut cannot be delivered to Thunderbird 128 by synthesised input; and the popup cannot be opened in a -plain-text composer at all, which is a defect in the add-on rather than a limit -of the harness. +plain-text composer at all, which is the add-on's scope rather than a limit of +the harness - it offers no route in there, so a test of the plain-text insert +has to reach past that by hand. What is still checked by hand is anything that is a claim about Thunderbird rather than about this project's own logic; that list is diff --git a/docs/release-checklist.md b/docs/release-checklist.md index cf565fa..d1ff906 100644 --- a/docs/release-checklist.md +++ b/docs/release-checklist.md @@ -43,9 +43,13 @@ unless another file is named: - `&`, `<`, `>` and `"` in the source reaching the message as those characters. - The popup closing when the insert lands. - A plain-text composer receiving the source as text with no markup in it. - The test unhides the format toolbar to get there, because as things stand the - popup cannot be opened in a plain-text composer at all - issue #12. What is - covered is the insert; what is broken is reaching it. + The test unhides the format toolbar to get there, because the add-on offers a + plain-text composer no route to the popup and is not meant to: it inserts into + HTML mail. What is covered is the insert; reaching it is not something a user + can do. +- The context-menu item being in an HTML composer's body menu and not in a + plain-text composer's, which is the one route that could have offered an + insert nothing could carry out. - The shortcut inserting exactly what the button inserts, and the manifest's `Ctrl+Shift+C` having become the key element Thunderbird derives from it. **Delivering that key press is not covered** - see the first item under diff --git a/src/background/background.js b/src/background/background.js index d222a15..62fdaa7 100644 --- a/src/background/background.js +++ b/src/background/background.js @@ -12,9 +12,10 @@ import { TAKE_PENDING_SELECTION } from "../messaging/take-pending-selection.js"; * * 1. This file's scope is re-executed every time an event wakes the page, so * top-level work must be safe to repeat. - * 2. Anything held in module scope is lost when the page is suspended. The one - * piece of state here - the parked selection - is written and read within a - * single user gesture, which is the only lifetime it can rely on. + * 2. Anything held in module scope is lost when the page is suspended. Both + * pieces of state here - the parked selection and the menu that is open - + * are written and read within a single user gesture, which is the only + * lifetime either can rely on. */ /** @@ -36,6 +37,27 @@ const MENU_ID = "thundercode-insert-code-block"; */ const pendingSelections = new Map(); +/** + * How many menus have opened since this page was woken, which is only ever + * read as a way of telling one of them from the next. + */ +let menusOpened = 0; + +/** + * Which of those menus is on screen, and zero when none is. + * + * Deciding whether the item belongs in a menu means asking the composer what + * format it is in, and the menu is already drawn by the time the answer comes + * back. Without this, an answer that arrives late would set the item's + * visibility for whichever menu is open by then - so a menu the add-on has + * nothing to say about would be handed the previous one's answer. + * + * Module scope, so both of these are lost when the event page is suspended. + * That costs nothing: a menu cannot outlive the page that is being woken to + * answer it. + */ +let menuOnScreen = 0; + /** * Creating the item at file scope means it exists as soon as the page runs, * which on an event page is also every time it is woken. The duplicate-id error @@ -53,6 +75,13 @@ function createMenu() { // would also match selections in the message reader and put the item in // menus that have no composer to insert into. contexts: ["compose_body"], + + // Hidden until a composer has been asked what format it is in, which is + // what `menus.onShown` below does. Created visible instead, the item + // would be in the menu for as long as that answer takes to arrive - and + // the composer where a false offer costs something is precisely the one + // where the answer is "plain text". + visible: false, }, () => void browser.runtime.lastError, ); @@ -65,11 +94,77 @@ createMenu(); // before the user opens their first composer. browser.runtime.onStartup.addListener(createMenu); +/** + * Whether this composer is one the add-on can put a code block into. + * + * The block goes in as markup, and the popup is reached through a button in + * the format toolbar - which Thunderbird hides in a plain-text composer, there + * being no formatting to offer. So a plain-text composer is a composer this + * add-on has nothing to do in, and the honest thing is to offer it nothing. + * + * A tab that cannot be asked - it has closed, or was never a composer - + * answers the same way. An offer this add-on could not check is one it cannot + * promise to keep. + */ +async function canTakeACodeBlock(tabId) { + try { + const { isPlainText } = await browser.compose.getComposeDetails(tabId); + return !isPlainText; + } catch { + return false; + } +} + +/** + * The item's visibility, decided per menu rather than once at creation. + * + * `onShown` fires for every menu this add-on could have an item in, including + * menus it has nothing in, so the context is checked before anything is + * touched: the item is one piece of state shared by every window, and an + * update made on behalf of a menu it is not in would be waiting in the next + * menu it is. + * + * `refresh` is the half without which none of this is visible. The menu is + * already on screen when this runs, so an item whose visibility has just + * changed keeps being drawn the old way until the menu is rebuilt. + */ +browser.menus.onShown.addListener(async (info, tab) => { + if (!info.contexts.includes("compose_body") || !tab) { + return; + } + + const menu = ++menusOpened; + menuOnScreen = menu; + + const visible = await canTakeACodeBlock(tab.id); + if (menuOnScreen !== menu) { + return; + } + + await browser.menus.update(MENU_ID, { visible }); + await browser.menus.refresh(); +}); + +browser.menus.onHidden.addListener(() => { + menuOnScreen = 0; +}); + browser.menus.onClicked.addListener(async (info, tab) => { if (info.menuItemId !== MENU_ID || !tab) { return; } + // Asked again rather than taken on trust from the item being visible: that + // visibility is state Thunderbird holds between one menu and the next, so a + // click can come from a menu drawn before `onShown` above had its say. + // Returning before anything is parked is the whole of what this has to get + // right - a selection left behind here would surface in the next popup this + // tab opens by some other route. + if (!(await canTakeACodeBlock(tab.id))) { + pendingSelections.delete(tab.id); + return; + } + // Plain text extracted from HTML by Thunderbird, so its indentation may // already be damaged before this extension sees it - the popup treats it as // a convenience, not as the source of truth. It is present only because the diff --git a/tests/node/background.test.js b/tests/node/background.test.js index f1dde86..9845538 100644 --- a/tests/node/background.test.js +++ b/tests/node/background.test.js @@ -35,11 +35,25 @@ describe("the background", () => { /** The ids the menu already holds, which outlive any one wake of the page. */ let created; + /** + * The tab ids that are plain-text composers. Everything else the fake is + * asked about is an HTML one, so a test that says nothing about format is a + * test about an HTML composer - which is what every test here was before + * the format started to matter. + */ + let plainText; + beforeEach(() => { unhandled = []; created = new Set(); + plainText = new Set(); fake = installBrowserFake({ + compose: { + getComposeDetails: async (tabId) => ({ + isPlainText: plainText.has(tabId), + }), + }, menus: { create: (properties, callback) => { const duplicate = created.has(properties.id); @@ -55,7 +69,11 @@ describe("the background", () => { } browser.runtime.lastError = undefined; }, + update: async () => {}, + refresh: async () => {}, onClicked: event(), + onShown: event(), + onHidden: event(), }, runtime: { lastError: undefined, @@ -105,6 +123,32 @@ describe("the background", () => { await handled; }; + /** + * Thunderbird showing the compose body's context menu, which is the moment + * the add-on gets to say whether its item belongs in it. Answered with the + * listener's own promise rather than awaited here, so that a test can fire + * something else while the add-on is still deciding. + */ + const showMenu = (tab, contexts = ["compose_body"]) => { + const [shown] = fake.fire( + "menus.onShown", + { contexts, menuIds: [menuProperties().id] }, + tab, + ); + return shown; + }; + + /** + * A composer that cannot answer at all - the tab has closed, or was never + * one. The other half of the format axis that the `plainText` set above is + * the first half of. + */ + const cannotBeAsked = () => { + browser.compose.getComposeDetails = async (tabId) => { + throw new Error(`Invalid tab ID: ${tabId}`); + }; + }; + /** * What the popup does when it opens: asks for the selection parked for the * tab it resolved for itself. The sender is empty because the background @@ -162,6 +206,146 @@ describe("the background", () => { const [first, second] = fake.calls("menus.create"); expect(second[0]).toEqual(first[0]); }); + + /** + * Created hidden, and revealed by `menus.onShown` once the composer has + * been asked what format it is in. The other order - created visible and + * hidden when the composer turns out to be plain text - is a menu that + * offers the insert for as long as the answer takes to arrive, in exactly + * the composer where the offer is false. + */ + it("creates the item hidden, before any composer has been asked", async () => { + await wake(); + + expect(menuProperties().visible).toBe(false); + }); + }); + + /** + * This is an HTML-mail add-on, and a plain-text composer has no route into + * it: the button sits in the format toolbar, which Thunderbird hides there, + * and the popup is anchored to that button. The context menu is the one + * route that could still offer an insert nothing can carry out, so what the + * add-on does about it is keep the item out of the menu. + */ + describe("the offer it makes, by the composer's format", () => { + it("shows the item in an HTML composer's menu", async () => { + await wake(); + await showMenu(composeTab(1, 11)); + + expect(fake.calls("menus.update")).toEqual([ + [menuProperties().id, { visible: true }], + ]); + expect(fake.calls("menus.refresh")).toHaveLength(1); + }); + + /** + * `refresh` is the half that is easy to leave out and impossible to see + * without: the menu is already on screen when this runs, so an item whose + * visibility changed is drawn as it was until the menu is rebuilt. + */ + it("keeps the item out of a plain-text composer's menu", async () => { + await wake(); + plainText.add(1); + await showMenu(composeTab(1, 11)); + + expect(fake.calls("menus.update")).toEqual([ + [menuProperties().id, { visible: false }], + ]); + expect(fake.calls("menus.refresh")).toHaveLength(1); + }); + + /** + * The tab has gone, or was never a composer. Not offering is the answer + * that cannot be wrong: an item that is not there is a route the user does + * not take, while an item that is there is a promise this add-on has just + * failed to check it can keep. + */ + it("offers nothing when the format cannot be read", async () => { + await wake(); + cannotBeAsked(); + await showMenu(composeTab(1, 11)); + + expect(fake.calls("menus.update")).toEqual([ + [menuProperties().id, { visible: false }], + ]); + }); + + /** + * `menus.onShown` fires for every menu the add-on holds the permission to + * see, not only for the one it has an item in. Touching the item from a + * menu it is not in would set its visibility for whichever menu opens + * next. + */ + it("leaves a menu it has nothing in alone", async () => { + await wake(); + await showMenu(composeTab(1, 11), ["selection", "message_list"]); + + expect(fake.calls("compose.getComposeDetails")).toEqual([]); + expect(fake.calls("menus.update")).toEqual([]); + expect(fake.calls("menus.refresh")).toEqual([]); + }); + + it("leaves a menu with no tab behind it alone", async () => { + await wake(); + await showMenu(undefined); + + expect(fake.calls("compose.getComposeDetails")).toEqual([]); + expect(fake.calls("menus.update")).toEqual([]); + }); + + /** + * The menu is already on screen when the listener runs, so the format can + * arrive after the user has dismissed it. Setting the item's visibility + * then would be setting it for whatever menu opens next, which is how an + * item hidden for a plain-text composer finds its way back into one. + */ + it("says nothing about a menu that has already closed", async () => { + await wake(); + const shown = showMenu(composeTab(1, 11)); + fake.fire("menus.onHidden"); + await shown; + + expect(fake.calls("menus.update")).toEqual([]); + expect(fake.calls("menus.refresh")).toEqual([]); + }); + + /** The same guard from the other side: two menus, and the second wins. */ + it("answers only the menu that is open when the format arrives", async () => { + await wake(); + plainText.add(2); + const first = showMenu(composeTab(1, 11)); + const second = showMenu(composeTab(2, 22)); + await Promise.all([first, second]); + + expect(fake.calls("menus.update")).toEqual([ + [menuProperties().id, { visible: false }], + ]); + }); + + /** + * Whether the item is visible is state Thunderbird holds between one menu + * and the next, so a click can arrive from a menu that was painted before + * the add-on had its say. The handler is correct on its own rather than on + * the strength of the item being hidden: no popup to open, and no + * selection left parked for whatever opens this tab's popup next. + */ + it("refuses a click that reaches it in a plain-text composer", async () => { + await wake(); + plainText.add(1); + await rightClick(composeTab(1, 11), "SELECT 1;"); + + expect(fake.calls("composeAction.openPopup")).toEqual([]); + expect(await claim(1)).toBe(""); + }); + + it("refuses a click whose composer cannot be asked", async () => { + await wake(); + cannotBeAsked(); + await rightClick(composeTab(1, 11), "SELECT 1;"); + + expect(fake.calls("composeAction.openPopup")).toEqual([]); + }); }); describe("the selection handover", () => { diff --git a/tests/thunderbird/harness/index.js b/tests/thunderbird/harness/index.js index 0806366..be8f086 100644 --- a/tests/thunderbird/harness/index.js +++ b/tests/thunderbird/harness/index.js @@ -31,8 +31,9 @@ * await compose.confirmActionPopup(); // Ctrl+Enter, then wait for the * // popup to close * await compose.selectInBody("text"); // something to right-click - * const items = await compose.openBodyContextMenu(); // the add-on's items + * const items = await compose.openBodyContextMenu({ expecting: 1 }); * await compose.activateMenuItem(items[0].id); + * await compose.closeBodyContextMenu(); // for a menu only looked at * await compose.editorState(); // { canUndo, modificationCount } * await compose.undo(); * await session.consoleMessages(); // which path the insert took @@ -44,8 +45,9 @@ * Marionette's chrome context, so a script sees `Services`, `ChromeUtils`, `Cc` * and `Ci`, and `window` is the window it was called on. * - * Four limits worth knowing before writing a test against this. Each was found - * the hard way and each is explained where it bites, in session.js: + * Four things worth knowing before writing a test against this. Each was found + * the hard way and each is explained where it bites, in session.js. The first + * three are limits of the harness; the fourth is the add-on's own shape: * * - `openActionPopup()` cannot see *inside* the popup. What it can do is hand * the popup the keyboard and read the result out of the message body, which @@ -59,8 +61,11 @@ * and says what that does and does not cover. * - The popup cannot be opened in a plain-text composer at all, because * Thunderbird hides the toolbar this add-on's button sits in and the popup - * is anchored to that button. That is a defect in the add-on rather than a - * limit of the harness - issue #12, with the details in insertion.test.js. + * is anchored to that button. That is the add-on's scope rather than a limit + * of the harness: it inserts into HTML mail, and a plain-text composer is + * offered no route in - which is why `openBodyContextMenu()` finds no item + * in one. A test that wants the plain-text *insert* has to reach past that + * by hand, and insertion.test.js says how and why. * * Every test file in this tier imports from here and not from the files * behind it, so this list is what the tier actually uses: a name that stops diff --git a/tests/thunderbird/harness/session.js b/tests/thunderbird/harness/session.js index c9d2678..7ead297 100644 --- a/tests/thunderbird/harness/session.js +++ b/tests/thunderbird/harness/session.js @@ -498,10 +498,47 @@ class ComposeWindow { return this.findInBody(text, { collapseAfter: true }); } + /** True while Thunderbird's compose context menu is on screen. */ + async bodyContextMenuIsOpen() { + return this.chrome( + `const [menuId] = arguments; + return document.getElementById(menuId)?.state === "open";`, + COMPOSE_CONTEXT_MENU_ID, + ); + } + + /** + * This add-on's items in the compose context menu as it is drawn right now, + * found by the prefix the extension framework gives them. + * + * Hidden items are left out, because "in the menu" here means what a person + * would see: an item the add-on has asked to hide is still an element in the + * popup, and counting it would make withholding the item look the same as + * offering it. + */ + async addonMenuItems() { + return this.chrome( + `const [menuId, prefix] = arguments; + return Array.from(document.getElementById(menuId).querySelectorAll("menuitem")) + .filter((item) => item.id.startsWith(prefix) && !item.hidden) + .map((item) => ({ id: item.id, label: item.getAttribute("label") }));`, + COMPOSE_CONTEXT_MENU_ID, + MENU_ITEM_ID_PREFIX, + ); + } + /** * Right-clicks the current selection in the message body, waits for * Thunderbird's compose context menu, and answers with this add-on's items - * in it. + * in it once there are `expecting` of them. + * + * The count is not a convenience. Whether this add-on's item belongs in this + * menu is decided after the menu is already on screen - `menus.onShown` asks + * the composer what format it is in and calls `menus.refresh()` with the + * answer - so a menu read the moment it opens still shows what the last one + * left behind. Saying how many items are expected is what makes that a wait + * rather than a race, and a count that never arrives fails as a timeout + * naming the number it wanted. * * A real widget-level event, synthesised into the editor's own window at the * selection's coordinates. Not a `dispatchEvent`: the menu is built from @@ -511,7 +548,7 @@ class ComposeWindow { * a selection that a right-click misses, and the selection is the whole * subject here. */ - async openBodyContextMenu() { + async openBodyContextMenu({ expecting }) { await this.chrome(` const editor = GetCurrentEditor(); const view = editor.document.defaultView; @@ -526,20 +563,37 @@ class ComposeWindow { ); `); await waitFor(`the ${COMPOSE_CONTEXT_MENU_ID} menu to open`, () => - this.chrome( - `const [menuId] = arguments; - return document.getElementById(menuId)?.state === "open";`, - COMPOSE_CONTEXT_MENU_ID, - ), + this.bodyContextMenuIsOpen(), ); - return this.chrome( - `const [menuId, prefix] = arguments; - return Array.from(document.getElementById(menuId).querySelectorAll("menuitem")) - .filter((item) => item.id.startsWith(prefix)) - .map((item) => ({ id: item.id, label: item.getAttribute("label") }));`, + + let items; + await waitFor( + `${expecting} of the add-on's items in the ${COMPOSE_CONTEXT_MENU_ID} menu`, + async () => { + items = await this.addonMenuItems(); + return items.length === expecting; + }, + ); + return items; + } + + /** + * Dismisses the menu without activating anything - the ending + * `activateMenuItem` provides for the tests that do activate something. A + * test that only looked at the menu still has to close it: a context menu + * left open is a popup that whatever comes next has to open behind. + */ + async closeBodyContextMenu() { + await this.chrome( + `const [menuId] = arguments; + document.getElementById(menuId).hidePopup();`, COMPOSE_CONTEXT_MENU_ID, - MENU_ITEM_ID_PREFIX, ); + await waitFor( + `the ${COMPOSE_CONTEXT_MENU_ID} menu to close`, + async () => !(await this.bodyContextMenuIsOpen()), + ); + return this; } /** @@ -563,12 +617,9 @@ class ComposeWindow { COMPOSE_CONTEXT_MENU_ID, id, ); - await waitFor(`the ${COMPOSE_CONTEXT_MENU_ID} menu to close`, () => - this.chrome( - `const [menuId] = arguments; - return document.getElementById(menuId)?.state !== "open";`, - COMPOSE_CONTEXT_MENU_ID, - ), + await waitFor( + `the ${COMPOSE_CONTEXT_MENU_ID} menu to close`, + async () => !(await this.bodyContextMenuIsOpen()), ); return this; } diff --git a/tests/thunderbird/insertion.test.js b/tests/thunderbird/insertion.test.js index 2acc34f..97b984d 100644 --- a/tests/thunderbird/insertion.test.js +++ b/tests/thunderbird/insertion.test.js @@ -353,9 +353,9 @@ describe("a right-click carrying a selection", () => { // The add-on's item, in Thunderbird's own context menu for the message // body, found by the prefix the extension framework gives it. Its id is // the background's and is not exported, so the prefix is what there is; - // one item is what this add-on creates. - const [item, ...rest] = await compose.openBodyContextMenu(); - expect(rest).toEqual([]); + // one item is what this add-on creates, and waiting for exactly one is + // how the menu is read after the add-on has had its say about it. + const [item] = await compose.openBodyContextMenu({ expecting: 1 }); expect(item.label).toBeTruthy(); await compose.activateMenuItem(item.id); @@ -396,36 +396,72 @@ describe("a right-click carrying a selection", () => { }); }); +describe("the item in the compose body's context menu", () => { + /** + * Both halves in one test, because they are one decision made in one place: + * the same `menus.onShown` handler shows the item and withholds it, and a + * test that only saw the plain-text half would pass just as well against an + * add-on that had no menu item left at all. + * + * The HTML composer goes first, and that order is the assertion's other + * half. The item's visibility is one piece of state shared by every window, + * so the menu below leaves it visible - and the plain-text menu then has to + * take the item out rather than finding it already gone. + */ + it("is offered in an HTML composer and withheld from a plain-text one", async () => { + const html = await session.openCompose(); + const plainText = await session.openCompose({ format: "plaintext" }); + try { + for (const compose of [html, plainText]) { + await compose.typeIntoBody("before SELECTED after"); + await compose.selectInBody("SELECTED"); + } + + const [item] = await html.openBodyContextMenu({ expecting: 1 }); + expect(item.label).toBeTruthy(); + await html.closeBodyContextMenu(); + + // Nothing of this add-on's in the menu: not a disabled item and not a + // greyed one, because either still advertises an insert that no route + // in this composer can carry out. + expect(await plainText.openBodyContextMenu({ expecting: 0 })).toEqual([]); + await plainText.closeBodyContextMenu(); + } finally { + await plainText.close(); + await html.close(); + } + }); +}); + describe("a plain-text composer", () => { /** - * A plain-text composer cannot open this add-on's popup at all, and that is - * a defect in the add-on rather than a limit of this harness. It is filed as - * issue #12. + * A plain-text composer has no route into this add-on's popup, and that is + * the add-on's scope rather than a limit of this harness. * * `compose_action.default_area` is `formattoolbar`, and Thunderbird hides * the format toolbar in a plain-text composer - there is no formatting to * offer. The popup is anchored to that button (`triggerAction` in * `ExtensionToolbarButtons.sys.mjs` calls * `openPopup(button, "bottomleft topleft")`), so with the button in a hidden - * toolbar the panel opens and rolls straight back up. Every route in goes - * through that same call, so the button, `Ctrl+Shift+C` and the right-click - * item all fail the same way. Confirmed on a real X server as well as - * headless, so it is not a headless artefact. + * toolbar the panel would open and roll straight back up. Every route goes + * through that same call, which is why there is no route: the button is + * hidden with its toolbar, `Ctrl+Shift+C` is inert, and the context-menu + * item is kept out of the menu - the test above. * - * What is broken is reaching the popup, and what this test is about is what - * happens after that - a different editor receiving text rather than markup. - * So the toolbar is unhidden for the length of the test, which changes - * nothing about the insert: the same button, the same popup, the same - * `scripting.executeScript` into the same composer. When the add-on is fixed - * this call comes out and nothing else here changes. + * So this call is permanent. What it reveals is the button, for the length + * of one test, and it changes nothing about the insert it makes possible: + * the same button, the same popup, the same `scripting.executeScript` into + * the same composer. It is here because the plain-text insert is code this + * add-on still has - a different editor taking text rather than markup - and + * this tier is the only place that can watch it run. */ const revealTheButton = (compose) => compose.chrome( `const [toolbarId] = arguments; document.getElementById(toolbarId).hidden = false;`, - // The toolbar the manifest asks for, not the literal, so that moving the - // button to the compose toolbar - which is one of the ways issue #12 - // could be fixed - makes this a harmless no-op instead of a lie. + // The toolbar the manifest asks for, not the literal, so that a button + // that ever moves to the compose toolbar makes this a harmless no-op + // instead of a lie. ACTION_TOOLBAR_ID, );