Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/react/src/components/Popovers/FloatingUIOptions.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
FloatingFocusManagerProps,
UseDismissProps,
UseFloatingOptions,
UseHoverProps,
Expand All @@ -14,4 +15,10 @@ export type FloatingUIOptions = {
useDismissProps?: UseDismissProps;
useHoverProps?: UseHoverProps;
elementProps?: HTMLAttributes<HTMLDivElement>;
/**
* Props to pass to the `FloatingFocusManager` component.
*
* If omitted, no `FloatingFocusManager` will be used.
*/
focusManagerProps?: Omit<FloatingFocusManagerProps, "context" | "children">;
};
19 changes: 15 additions & 4 deletions packages/react/src/components/Popovers/GenericPopover.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import {
useFloating,
useTransitionStyles,
autoUpdate,
FloatingFocusManager,
useDismiss,
useFloating,
useHover,
useInteractions,
useMergeRefs,
useTransitionStatus,
autoUpdate,
useHover,
useTransitionStyles,
} from "@floating-ui/react";
import { HTMLAttributes, ReactNode, useEffect, useRef } from "react";

Expand Down Expand Up @@ -175,6 +176,16 @@ export const GenericPopover = (
);
}

if (props.focusManagerProps) {
return (
<FloatingFocusManager {...props.focusManagerProps} context={context}>
<div ref={mergedRefs} {...mergedProps}>
{props.children}
</div>
</FloatingFocusManager>
);
}

return (
<div ref={mergedRefs} {...mergedProps}>
{props.children}
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/editor/ComponentsContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
ComponentType,
createContext,
CSSProperties,
ForwardedRef,
HTMLInputAutoCompleteAttribute,
KeyboardEvent,
MouseEvent,
Expand Down Expand Up @@ -274,6 +275,7 @@ export type ComponentProps = {
onSubmit?: () => void;
autoComplete?: HTMLInputAutoCompleteAttribute;
"aria-activedescendant"?: string;
ref?: ForwardedRef<HTMLInputElement>;
};
};
Menu: {
Expand Down
122 changes: 63 additions & 59 deletions packages/xl-ai/src/components/AIMenu/AIMenuController.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,71 +26,75 @@ export const AIMenuController = (props: {
const blockId = aiMenuState === "closed" ? undefined : aiMenuState.blockId;

const floatingUIOptions = useMemo<FloatingUIOptions>(
() => ({
...props.floatingUIOptions,
useFloatingOptions: {
open: aiMenuState !== "closed",
placement: "bottom",
middleware: [
offset(10),
flip(),
size({
apply({ rects, elements }) {
Object.assign(elements.floating.style, {
width: `${rects.reference.width}px`,
});
},
}),
],
onOpenChange: (open) => {
if (open || aiMenuState === "closed") {
return;
}
() =>
({
...props.floatingUIOptions,
useFloatingOptions: {
open: aiMenuState !== "closed",
placement: "bottom",
middleware: [
offset(10),
flip(),
size({
apply({ rects, elements }) {
Object.assign(elements.floating.style, {
width: `${rects.reference.width}px`,
});
},
}),
],
onOpenChange: (open) => {
if (open || aiMenuState === "closed") {
return;
}

if (aiMenuState.status === "user-input") {
ai.closeAIMenu();
} else if (
aiMenuState.status === "user-reviewing" ||
aiMenuState.status === "error"
) {
ai.rejectChanges();
}
},
whileElementsMounted(reference, floating, update) {
return autoUpdate(reference, floating, update, {
animationFrame: true,
});
},
...props.floatingUIOptions?.useFloatingOptions,
},
useDismissProps: {
enabled:
aiMenuState === "closed" || aiMenuState.status === "user-input",
// We should just be able to set `referencePress: true` instead of
// using this listener, but this doesn't seem to trigger.
// (probably because we don't assign the referenceProps to the reference element)
outsidePress: (event) => {
if (event.target instanceof Element) {
const blockElement = event.target.closest(".bn-block");
if (
blockElement &&
blockElement.getAttribute("data-id") === blockId
) {
if (aiMenuState.status === "user-input") {
ai.closeAIMenu();
} else if (
aiMenuState.status === "user-reviewing" ||
aiMenuState.status === "error"
) {
ai.rejectChanges();
}
},
whileElementsMounted(reference, floating, update) {
return autoUpdate(reference, floating, update, {
animationFrame: true,
});
},
...props.floatingUIOptions?.useFloatingOptions,
},
useDismissProps: {
enabled:
aiMenuState === "closed" || aiMenuState.status === "user-input",
// We should just be able to set `referencePress: true` instead of
// using this listener, but this doesn't seem to trigger.
// (probably because we don't assign the referenceProps to the reference element)
outsidePress: (event) => {
if (event.target instanceof Element) {
const blockElement = event.target.closest(".bn-block");
if (
blockElement &&
blockElement.getAttribute("data-id") === blockId
) {
ai.closeAIMenu();
}
}
}

return true;
return true;
},
...props.floatingUIOptions?.useDismissProps,
},
...props.floatingUIOptions?.useDismissProps,
},
elementProps: {
style: {
zIndex: 100,
elementProps: {
style: {
zIndex: 100,
},
...props.floatingUIOptions?.elementProps,
},
...props.floatingUIOptions?.elementProps,
},
}),
// we use the focus manager instead of `autoFocus={true}` to prevent "page-scrolls-to-top-when-opening-the-floating-element"
// see https://floating-ui.com/docs/floatingfocusmanager#page-scrolls-to-top-when-opening-the-floating-element
focusManagerProps: {},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sort of a smell no? This is equivalent to enabling it, but it doesn't feel right.

Shouldn't we always render the focus manager and just toggle the disabled prop?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I improved this in latest commit:

  • was a bit cumbersome as now I make sure it's disabled by default on all call sites
  • good news is that I was able to fix a bug so that FloatingComposer (for comments) now also autofocuses correctly
  • side note; do you know if TableCellPopover can be removed? it's not used anywhere afaik

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I was confused why you needed this, but then it made sense that you needed to pass through these props. I think this is fine for now, and is explicit.

We don't need TableCellPopover, I think it stayed from the refactor. I removed it

}) satisfies FloatingUIOptions,
[ai, aiMenuState, blockId, props.floatingUIOptions],
);

Expand Down
23 changes: 19 additions & 4 deletions packages/xl-ai/src/components/AIMenu/PromptSuggestionMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from "react";

Expand All @@ -30,7 +31,8 @@ export const PromptSuggestionMenu = (props: PromptSuggestionMenuProps) => {
// const dict = useAIDictionary();
const Components = useComponentsContext()!;

const { onManualPromptSubmit, promptText, onPromptTextChange } = props;
const { onManualPromptSubmit, promptText, onPromptTextChange, disabled } =
props;

// Only used internal state when `props.prompText` is undefined (i.e., uncontrolled mode)
const [internalPromptText, setInternalPromptText] = useState<string>("");
Expand Down Expand Up @@ -95,18 +97,31 @@ export const PromptSuggestionMenu = (props: PromptSuggestionMenuProps) => {
setSelectedIndex(0);
}, [promptTextToUse, setSelectedIndex]);

const inputRef = useRef<HTMLInputElement>(null);
const hasBeenDisabled = useRef(disabled);

useEffect(() => {
// This effect is used so that after the input has been disabled (for example, when AI results are loaded),
// the input is focused again.
if (inputRef.current && hasBeenDisabled.current && !disabled) {
inputRef.current.focus();
}

if (disabled) {
hasBeenDisabled.current = true;
}
}, [disabled]);

return (
<div className={"bn-combobox"}>
<Components.Generic.Form.Root>
<Components.Generic.Form.TextInput
// Change the key when disabled change, so that autofocus is retriggered
key={"input-" + props.disabled}
ref={inputRef}
className={"bn-combobox-input"}
name={"ai-prompt"}
variant={"large"}
icon={props.icon}
value={promptTextToUse || ""}
autoFocus={true}
placeholder={props.placeholder}
disabled={props.disabled}
onKeyDown={handleKeyDown}
Expand Down
79 changes: 79 additions & 0 deletions tests/src/end-to-end/ai/ai.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { expect } from "@playwright/test";
import { test } from "../../setup/setupScript.js";
import { AI_URL, EDITOR_SELECTOR } from "../../utils/const.js";

// Use a small viewport so the editor content requires scrolling.
test.use({ viewport: { width: 800, height: 400 } });

test.beforeEach(async ({ page }) => {
await page.goto(AI_URL);
});

test.describe("AI Menu Scroll Regression", () => {
test("opening the AI menu should not scroll the page to the top", async ({
page,
}) => {
// Wait for the editor to be ready
await page.waitForSelector(EDITOR_SELECTOR);

// Click on the last paragraph so the cursor is near the bottom of the content
const lastParagraph = page
.locator("[data-content-type='paragraph']")
.last();
await lastParagraph.click();

// Ensure the page is scrolled down (editor puts cursor near bottom of content)
// We scroll down explicitly to make sure we're not at the top
await page.evaluate(() => {
window.scrollTo(0, document.body.scrollHeight);
});
await page.waitForTimeout(200);

// Record the scroll position before opening the AI menu
const scrollYBefore = await page.evaluate(() => window.scrollY);

// Sanity check: we should actually be scrolled down
expect(scrollYBefore).toBeGreaterThan(0);

// Open the AI menu via the slash command
// First, focus back on the editor at the last paragraph
await lastParagraph.click();
await page.waitForTimeout(100);

// Type /ai to open the slash menu and select the AI option
await page.keyboard.type("/ai", { delay: 50 });
await page.waitForTimeout(300);

// Wait for the suggestion menu to appear
const suggestionMenu = page.locator(".bn-suggestion-menu");
await suggestionMenu.waitFor({ state: "visible", timeout: 3000 });

// Click the AI suggestion menu item to open the AI menu
const aiMenuItem = suggestionMenu
.locator(".bn-suggestion-menu-item")
.first();
await aiMenuItem.click();

// Wait for the AI menu (combobox input) to appear
const aiMenuInput = page.locator(
".bn-combobox-input input, .bn-combobox input",
);
await aiMenuInput.waitFor({ state: "visible", timeout: 3000 });

// Brief wait for any scroll side effects to take place
await page.waitForTimeout(300);

// Screenshot after opening AI menu
expect(await page.screenshot()).toMatchSnapshot(
"ai_menu_scroll_position.png",
);

// Check that the scroll position has not jumped to the top
const scrollYAfter = await page.evaluate(() => window.scrollY);
expect(scrollYAfter).toBeGreaterThan(0);
expect(scrollYAfter).toBeGreaterThanOrEqual(scrollYBefore * 0.2);

// Verify the AI menu input is actually focused
await expect(aiMenuInput).toBeFocused();
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions tests/src/utils/const.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ export const ARIAKIT_URL = !process.env.RUN_IN_DOCKER
? `http://localhost:${PORT}/basic/ariakit?hideMenu`
: `http://host.docker.internal:${PORT}/basic/ariakit?hideMenu`;

export const AI_URL = !process.env.RUN_IN_DOCKER
? `http://localhost:${PORT}/ai/minimal?hideMenu`
: `http://host.docker.internal:${PORT}/ai/minimal?hideMenu`;

export const STATIC_URL = !process.env.RUN_IN_DOCKER
? `http://localhost:${PORT}/backend/rendering-static-documents?hideMenu`
: `http://host.docker.internal:${PORT}/backend/rendering-static-documents?hideMenu`;
Expand Down
Loading