Rich-text fixes (Mobile, Enter, Range errors, alignment etc.)#1802
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update enhances the rich text editor’s schema, focus, and selection handling, especially for keyboard and mobile interactions. It introduces new utilities for tabbable elements, adds comprehensive tests, adjusts Storybook and test scripts, and provides new Storybook stories for editor fields. Minor changes affect component initialization and styling. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
github-actions
Bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/viewer/src/project/NewEntryButton.svelte (1)
25-33: Add missingidprop declaration and ensure backward compatibility.The
idprop is used on line 36 but not declared in the props interface. This could cause TypeScript errors and runtime failures if the prop is not provided.Apply this diff to add the missing prop declaration:
const { onclick, active, shortForm = false, + id, }: { onclick: () => void; shortForm?: boolean; active?: boolean; + id: string; } = $props();
🧹 Nitpick comments (3)
frontend/viewer/src/lib/utils/tabbable.ts (1)
22-50: Consider adding infinite loop protection tofindNextTabbable.The function logic is sound, but the while loop could potentially run indefinitely if the DOM structure is malformed or circular. Consider adding a maximum iteration limit as a safeguard.
export function findNextTabbable( current: HTMLElement | SVGElement | null | undefined ): HTMLElement | SVGElement | undefined { if (!current?.parentElement) { return current ?? undefined; } if (!isTabbable(current, {displayCheck})) { throw new Error('Current element is not tabbable, so can\'t find relative tabbable element'); } let container = current.parentElement; let tabbables = tabbable(container, {displayCheck}); + let iterations = 0; + const MAX_ITERATIONS = 100; // Reasonable limit for DOM depth while (tabbables.length === 0 // should never happen, but whatever || tabbables.at(-1) === current // we're still last, so haven't found a "next" yet ) { + if (++iterations > MAX_ITERATIONS) { + console.warn('Max iterations reached in findNextTabbable, possible DOM issue'); + return tabbables[0] ?? current; + } if (!container.parentElement) return tabbables[0]; // loop back to first container = container.parentElement; tabbables = tabbable(container, {displayCheck}); } const currentIndex = tabbables.indexOf(current); if (currentIndex === -1) { throw new Error('Current tabbable element should always be in an ancestor\'s list of tabbables'); } return tabbables[currentIndex + 1] ?? tabbables[0]; // loop back to first }frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte (2)
132-135: The 100ms timeout for pointer detection may be fragile.The hardcoded 100ms delay could lead to race conditions on slower devices or under heavy load. Consider using a more reliable method to distinguish pointer from keyboard interactions.
Instead of a timeout, consider tracking the pointer state more explicitly:
- pointerdown() { - pointerDown = true; - setTimeout(() => pointerDown = false, 100); // yes, apparently we need a decently high timeout value - }, + pointerdown() { + pointerDown = true; + }, + pointerup() { + pointerDown = false; + },
210-218: Document the root cause of the Enter key range errors.The comment mentions that Enter causes range errors on both mobile and desktop. It would be helpful to understand and document the specific ProseMirror issue causing these errors to ensure this workaround remains appropriate in future updates.
Would you like me to help investigate the specific range errors caused by Enter key handling in ProseMirror?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/viewer/package.json(1 hunks)frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte(16 hunks)frontend/viewer/src/lib/components/ui/input/input-shell.svelte(1 hunks)frontend/viewer/src/lib/utils/tabbable.test.ts(1 hunks)frontend/viewer/src/lib/utils/tabbable.ts(2 hunks)frontend/viewer/src/project/NewEntryButton.svelte(1 hunks)frontend/viewer/src/stories/editor/entity-primitives/entry-editor-primitive.stories.svelte(1 hunks)frontend/viewer/src/stories/editor/fields/1_overview.stories.svelte(1 hunks)frontend/viewer/vitest.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1758
File: frontend/viewer/src/project/browse/BrowseView.svelte:78-85
Timestamp: 2025-06-18T09:23:29.799Z
Learning: For keyboard accessibility in TabsList components, myieye implements Enter key handlers on the TabsList rather than relying on change events that would interfere with arrow key navigation.
frontend/viewer/src/project/NewEntryButton.svelte (1)
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/lib/components/ui/input/input-shell.svelte (3)
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1758
File: frontend/viewer/src/project/browse/BrowseView.svelte:78-85
Timestamp: 2025-06-18T09:23:29.799Z
Learning: In Svelte Tabs components, using `on:change` event handler would cause popups to close when users navigate with arrow keys, which creates poor UX. The user myieye prefers the current activation mode over `activationMode="manual"` and considers arrow key popup closing to be "too eager" behavior.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/stories/editor/fields/1_overview.stories.svelte (3)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1536
File: backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs:43-44
Timestamp: 2025-03-17T13:16:24.769Z
Learning: In the SIL FieldWorks Language Explorer (FLEx) system, only certain fields support rich text formatting (embedded styles): Definition, Example sentences, Translations, Notes, and Literal meaning fields. Other fields like Gloss, Lexeme form, and Citation form are plain text only. The codebase reflects this distinction by using RichMultiString for rich-text capable fields and MultiString for plain text fields.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1536
File: backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs:43-44
Timestamp: 2025-03-17T13:16:24.769Z
Learning: In the SIL FieldWorks Language Explorer (FLEx) system, only certain fields support rich text formatting (embedded styles): Definition, Example sentence, Translation, Note, and Literal meaning fields. Other fields like Gloss, Lexeme form, and Citation form are plain text only. The codebase reflects this by using RichMultiString for rich-text capable fields and MultiString for plain text fields.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte (5)
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1758
File: frontend/viewer/src/project/browse/BrowseView.svelte:78-85
Timestamp: 2025-06-18T09:23:29.799Z
Learning: For keyboard accessibility in TabsList components, myieye implements Enter key handlers on the TabsList rather than relying on change events that would interfere with arrow key navigation.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1758
File: frontend/viewer/src/project/browse/BrowseView.svelte:78-85
Timestamp: 2025-06-18T09:23:29.799Z
Learning: In Svelte Tabs components, using `on:change` event handler would cause popups to close when users navigate with arrow keys, which creates poor UX. The user myieye prefers the current activation mode over `activationMode="manual"` and considers arrow key popup closing to be "too eager" behavior.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1612
File: frontend/viewer/src/lib/entry-editor/DeleteDialog.svelte:39-52
Timestamp: 2025-04-18T10:33:51.961Z
Learning: Svelte 5 uses standard HTML attribute syntax for event handlers (e.g., `onclick={handler}`) instead of the Svelte-specific directive syntax (e.g., `on:click={handler}`) used in previous versions.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1612
File: frontend/viewer/src/lib/entry-editor/DeleteDialog.svelte:39-52
Timestamp: 2025-04-18T10:33:51.961Z
Learning: Svelte 5 uses standard HTML attribute syntax for event handlers (e.g., `onclick={handler}`) instead of the Svelte-specific directive syntax (e.g., `on:click={handler}`) used in previous versions.
🧬 Code Graph Analysis (1)
frontend/viewer/src/lib/utils/tabbable.test.ts (1)
frontend/viewer/src/lib/utils/tabbable.ts (2)
findFirstTabbable(6-20)findNextTabbable(22-50)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: frontend
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
frontend/viewer/src/stories/editor/entity-primitives/entry-editor-primitive.stories.svelte (1)
11-11: LGTM! Using a hardcoded UUID improves test reliability.Using a fixed UUID in test stories is a good practice as it ensures reproducible test results and makes debugging easier. The hardcoded UUID follows the correct v4 format.
frontend/viewer/src/lib/components/ui/input/input-shell.svelte (1)
11-11: Good simplification of focus ring styling.The refactoring to apply
ring-ring,outline-none, andring-offset-2unconditionally while keeping onlyring-2conditional on focus-visible improves clarity and maintainability.frontend/viewer/src/lib/utils/tabbable.ts (1)
1-4: Good environment-aware configuration for tabbable checks.The
displayCheckconfiguration appropriately handles different environments, using 'none' for tests (jsdom) where display checks aren't reliable, and 'full' for production.frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte (3)
36-41: Verify the unusualinline: trueconfiguration for the doc node.Setting
inline: trueon the doc node is atypical in ProseMirror schemas, as doc nodes are usually block-level containers. While this may be intentional for your specific rich-text input behavior, please confirm this doesn't cause unexpected side effects with ProseMirror's internal assumptions.
341-356: Good improvement to input consistency with InputShell wrapper.Using InputShell as the wrapper component with
tabindex={-1}elegantly solves the focus ring styling while keeping the editor as the actual tab target. This aligns well with the PR's goal of making rich-text inputs behave consistently with standard inputs.
312-339: Excellent CSS fixes for rich-text behavior and alignment.The changes effectively address the PR objectives:
white-space: preensures lines only break on actual line breaks, matching standard input behavior- The zero-width space
::beforecontent cleverly maintains baseline alignment when empty- Horizontal scrolling with hidden scrollbar provides a clean appearance
These improvements create the consistent behavior between rich-text and standard inputs.
frontend/viewer/vitest.config.ts (1)
37-37: LGTM! Simplified project naming.The rename from "browser (non storybook)" to "browser" improves clarity and aligns with the new
test:browsernpm script.frontend/viewer/package.json (2)
33-33: Good addition of browser-specific test command.The
test:browserscript provides a convenient way to run browser-specific tests in isolation, complementing the existing test commands.
39-39: Note: Storybook will be accessible on all network interfaces.Adding
--host 0.0.0.0makes Storybook accessible from any network interface, not just localhost. This is useful for testing on different devices but be aware of the security implications when running on untrusted networks.frontend/viewer/src/stories/editor/fields/1_overview.stories.svelte (3)
1-16: Well-structured Storybook setup with comprehensive component imports.The imports and setup properly include all necessary field editor components, demo data, and writing systems. The organization supports thorough testing of the rich-text editor improvements mentioned in the PR objectives.
18-249: Excellent comprehensive demo covering all field types and states.This story effectively showcases all editor field components in various states (standard, readonly, empty, readonly/empty), which is perfect for testing the rich-text fixes mentioned in the PR objectives:
- Alignment issues between readonly and standard fields
- Multi-line text handling with newlines
- Mixed writing system spans in rich text fields
- Empty state handling
The grid layout organization makes it easy to visually compare different field types and their behaviors.
251-260: Perfect alignment testing story for the rich-text fixes.This story directly addresses the alignment issues mentioned in the PR objectives by placing standard and rich inputs side-by-side in a flex container. This will help verify that the baseline alignment fixes for empty, readonly rich-text field labels are working correctly.
frontend/viewer/src/lib/utils/tabbable.test.ts (3)
1-16: Proper test setup with DOM container management.The test setup correctly creates a fresh DOM container for each test and cleans up afterward, ensuring test isolation and preventing side effects between tests.
18-55: Comprehensive testing of findFirstTabbable function.The test cases thoroughly cover all scenarios:
- Null/undefined input handling
- No tabbable elements case (correctly expects
nullreturn fromwalker.nextNode())- Finding first tabbable element in DOM tree
- Explicit tabindex handling
The test structure is clear and validates the function's behavior accurately.
57-206: Excellent comprehensive testing of findNextTabbable function.This test suite covers all critical scenarios for tabbable element navigation:
- Edge cases: null/undefined inputs, no parent element
- Error handling: non-tabbable current element
- Navigation patterns: next element, wrapping, DOM traversal
- Complex DOM structures: nested containers, mixed element types
- Special cases: custom tabindex, single element wrapping, deep nesting
The test expectations align perfectly with the function implementation, and the DOM structures used are realistic and representative of actual usage scenarios. This thorough testing ensures the keyboard navigation improvements mentioned in the PR objectives will work reliably.
Wrapping is good
myieye
force-pushed
the
heavy-rich-text-bug-fixing
branch
from
0041f96 to
fd55835
Compare
# Conflicts: # frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
looks good to me. I merged in develop as an i18n fix had moved the schema to another file
2 participants
Resolves #1699
Resolves #1792
And fixes numerous other issues 😆.
With this PR, I basically can't find any more differences between our rich-text input and standard inputs on Chrome, Firefox or Chrome on my phone.
Fixes:
whitespace: 'pre', inline: true(as values given to prose-mirror) fixed our backspace/delete issues. That all seems fine now.