Nav button fixes (BL-15345) #7565

andrew-polk · 2026-01-06T21:59:26Z

This change is

JohnThomson

@JohnThomson reviewed 5 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @andrew-polk).

src/BloomBrowserUI/react_components/LinkTargetChooser/URLEditor.tsx line 108 at r1 (raw file):

    function isExternalUrl(url: string): boolean {
        return /^(https?:)?\/\//i.test(url);
    }

Not quite sure which problem is being corrected here. It feels like a fairly drastic change...before we let C# handle any url not starting with /book, now we only let it handle https?: ones. I suppose it's unlikely we'd have any others, but a mailto: url could conceivably be wanted. Is there anything useful to say in a comment about why only http urls should be considered external?

src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx line 1152 at r1 (raw file):

    if (
        !isNavigationButton(canvasElement) &&

I think it would be helpful to have a distinct comment here on why nav buttons don't do this...especially since having a link is the main reason for some kinds to exist. IIRC the reason is that it duplicates another command that is unique to nave buttons?

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 4927 at r1 (raw file):

            undefined,
            undefined,
            keepInsideCanvas,

This is getting painful. Maybe it's time for finishAddingCanvasElement to take an object param, so you can include by name just the params you don't want a default for? I don't want to make a lot of extra work, so it's OK to say "too much trouble for now", but it seemed worth suggesting.

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 5216 at r1 (raw file):

            { width: 120, height: 120 },
            doAfterElementCreated,
            true,

another example where it would be more readable as keepInsideCanvas: true

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 5460 at r1 (raw file):

        // Technically, it only tries to move up and left if necessary to keep inside the canvas.
        if (keepInsideCanvas) {

Could we use all or part of the logic of ensureCanvasElementsIntersectParent() here? Or is it trying to do something distinctly different? I think it would be worth commenting, e.g., that here we want to make it entirely visible (if possible) whereas the other routine only wants to make sure it's at least partly visible.

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 5463 at r1 (raw file):

            const bloomCanvas = bloomCanvasJQuery.get(0) as HTMLElement;
            const canvasSize = getExactClientSize(bloomCanvas);
            const elementWidth = canvasElement.clientWidth;

It should have a style.width and height, too...would we get any useful improvement in precision by using them?

src/BloomBrowserUI/bookEdit/js/CanvasElementManager.ts line 5465 at r1 (raw file):

            const elementWidth = canvasElement.clientWidth;
            const elementHeight = canvasElement.clientHeight;
            const currentLeft = CanvasElementManager.pxToNumber(

It's odd that we have elementWidth and elementHeight along with currentLeft and currentTop for variables that hold the current dimensions of the same element. I think it would be more readable if all four used one or the other. (I'd be inclined to use currentCeLeft etc.)

andrew-polk

@andrew-polk made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @JohnThomson).

src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx line 1152 at r1 (raw file):