Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lexical-react] fix: ensure attributes are set immediately on menu #7237

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sodenn
Copy link

@sodenn sodenn commented Feb 23, 2025

Description

With the changes introduced in Lexical 0.25.0 (notably #7181 and #7164), the menu now renders immediately. However, this has led to two issues:

1. Missing attributes on initial render
The menu container <div> is rendered before its attributes are assigned. As a result:

  • Styles defined via anchorClassName are not applied immediately, causing visibility issues. The attributes only appear after the user makes further inputs. In my case, the menu is covered by a dialog because the z-index from my custom class is not applied initially.
  • Other missing attributes, such as role and aria-label, make automated testing more difficult.

2. Persistent DOM Element (not addressed in this PR)
The menu container <div> remains in the DOM even when the menu is closed. This can lead to duplicate IDs, potentially causing accessibility and testing issues.

Fix in This PR
This PR ensures that all attributes are correctly assigned as soon as the menu is rendered.

Before

before

After

after1 after2

Copy link

vercel bot commented Feb 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 23, 2025 0:49am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 23, 2025 0:49am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 23, 2025
etrepum
etrepum previously approved these changes Feb 24, 2025
@etrepum
Copy link
Collaborator

etrepum commented Feb 24, 2025

As you've mentioned this does fix some problems, but other issues remain primarily due to the id conflict that now arises since all of the menus are rendered concurrently.

e.g. this code no longer works correctly (due to prior regressions):

const scrollIntoViewIfNeeded = (target: HTMLElement) => {
  const typeaheadContainerNode = document.getElementById('typeahead-menu');
  if (!typeaheadContainerNode) {
    return;
  }

@etrepum
Copy link
Collaborator

etrepum commented Feb 24, 2025

Perhaps a path for a short term fix might be to only assign the id to the active menu, so there's only one typeahead-menu in the document at a time. That probably wouldn't require changing the tests and other logic that depends on the typeahead-menu id

@etrepum etrepum dismissed their stale review February 24, 2025 01:54

Test suite failures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants