Fixes for Internal Linking/AtLinkPlugin and Firefox #1492
Conversation
In certain circumstances, mostly with Firefox, our plugin would adjust selection from a ZWNJ to the adjacent search node, and something else would adjust selection back to the ZWNJ and we would loop forever. See TryGhost/Ghost#22421
WalkthroughThe changes refactor the handling of the '@' character in the editor by introducing a new function, Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/koenig-lexical/test/e2e/internal-linking-firefox-test.jsOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/test/e2e/internal-linking-firefox-test.js (1)
189-189: Empty test suite for Link toolbar.The Link toolbar test suite is defined but contains no tests. This might be intended if there were no Firefox-specific issues with the link toolbar.
Consider either adding a comment explaining why this section is empty or removing it if no tests are planned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/test/e2e/internal-linking-firefox-test.js(1 hunks)
🔇 Additional comments (13)
packages/koenig-lexical/test/e2e/internal-linking-firefox-test.js (13)
1-3: Appropriate imports for testing the internal linking functionality.The imports include all necessary utilities for the e2e tests, such as HTML assertion, editor focus, and initialization helpers.
4-18: Well-structured test setup with proper lifecycle hooks.The test suite follows best practices by:
- Using separate browser page for Firefox tests
- Properly initializing with the internal linking lab feature enabled
- Including appropriate beforeAll/beforeEach/afterAll hooks
- Cleaning up resources after tests complete
19-58: Good test coverage for bookmark card default options.The test thoroughly validates the bookmark card functionality by:
- Checking visibility of the dropdown
- Asserting the complete HTML structure matches expectations
- Verifying that default options appear correctly
60-89: Comprehensive testing of metadata display on selected items.The test effectively validates that:
- Metadata is shown for the selected item
- Metadata is removed when an item is deselected
- Metadata appears for the newly selected item
- Different metadata types (members only, specific tiers) are correctly displayed
91-101: Effective test for search highlighting functionality.The test correctly verifies that search terms are highlighted in the results using the bold font style.
102-110: Important regression test for special character handling.This test ensures the application doesn't crash when using regexp characters in search, which is particularly important given the PR's focus on fixing Firefox-specific issues.
112-186: Comprehensive tests for URL query handling across different protocols.These tests methodically verify that URL queries work correctly for:
- HTTP URLs
- Anchor links
- Relative paths
- Mailto links
This is particularly valuable for ensuring consistent behavior across browsers.
191-236: Good test for at-linking default behavior.The test properly verifies:
- The at-linking node structure when typing '@'
- The popup appears with default options
- The placeholder text is correctly shown
238-270: Thorough test for at-linking search functionality.The test effectively:
- Validates the node structure during search
- Waits appropriately for search results
- Verifies highlighting of search terms in results
- Checks the correct category header is shown ("Posts")
272-284: Important test for no-results handling.This test correctly verifies that:
- The "No results found" message appears when no matches exist
- Pressing Enter with no results converts the at-link to plain text
286-313: Thorough backspace handling test.The test effectively validates that:
- Backspacing within the at-link input works correctly
- Backspacing when the input is empty reverts to plain text
- The DOM structure is correct at each step
This is particularly important given the PR's focus on fixing Firefox-specific cursor and selection issues.
315-350: Important test for bookmark creation via at-linking.The test properly:
- Types '@' followed by search text
- Waits for search results
- Selects and confirms a result
- Verifies the bookmark card is created with correct content
352-371: Good Firefox-specific clipboard handling test.The test correctly uses a Firefox-compatible approach for pasting content into an at-link node. The comment on line 356 is helpful in explaining why a different approach is needed for Firefox.
1 participant
Fixes for TryGhost/Ghost#21721
Hi 👋🏻 this bug turned into a bit of a rabbit hole, I'm happy to split this into smaller PRs or make any other adjustments as needed. I found 3 separate, but connected problems:
editor.registerUpdateListenerto catch the next update, and then immediately deregistered that listener. I explored and considered a few other solutions such as setTimeout, upgrading to Lexical 17.0 or higher and taking advantage of theeditor.readmethod to force a state reconciliation, but this seemed like a better compromise with the existing code.prevEditorStateand avoid looping.[ZWNJNode, SearchNode, ExtendedTextNode](Firefox) instead of[ExtendedTextNode](Chrome). In Firefox, the existing node is deleted and text copied to another node - this causes the cursor to move left (before the text entered). I've updated the transform code to preserve any existing non-empty node if possible, fixing this issue.I've also gratefully added the tests already written in the spike code.