TINY-11394: fix contextsizeinputform keyboard navigation
#10027
Conversation
WalkthroughThis pull request introduces several enhancements to the TinyMCE project, focusing on improving keyboard navigation and accessibility for size inputs in context forms. Key changes include the addition of a new changelog entry, the implementation of a new CSS class for better interaction behavior, and significant modifications to the rendering logic of context form size inputs. Additionally, test coverage has been expanded to include keyboard navigation scenarios, ensuring a more robust user experience. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 (
|
lorenzo-pomili
requested review from
a team,
hamza0867,
HAFRMO and
ztomaszyk
and removed request for
a team
![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
🧹 Outside diff range and nitpick comments (1)
modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts (1)
207-251: Consider adding reverse keyboard navigation testsIncluding tests for reverse navigation using
Shift+Tabwould ensure comprehensive coverage of keyboard accessibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml(1 hunks)modules/oxide/src/less/theme/components/toolbar/toolbar.less(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts(1 hunks)modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml
🔇 Additional comments (5)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (1)
1-223: Component implementation looks solid
The new SizeInputToolbar component is well-structured, utilizes Alloy behaviors effectively, and integrates smoothly with existing components.
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts (2)
6-6: Import statement correctly updated to 'SizeInputToolbar'
The import has been updated to reflect the new SizeInputToolbar component, ensuring the correct module is used.
Line range hint 9-17: Simplified parameters enhance code clarity
Removing unused parameters (inDialog, context, name) from renderSizeInput streamlines the function call and improves readability.
modules/oxide/src/less/theme/components/toolbar/toolbar.less (1)
159-177:
Verify browser support for the ':has' pseudo-class
The use of the :has pseudo-class may not be fully supported in all target browsers, which could affect the focus outline styling.
Please confirm browser compatibility for :has or consider an alternative approach, such as adding a class via JavaScript when the input is focused.
modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts (1)
84-86: Defining selectors improves test maintainability
Introducing constants for selectors enhances readability and makes the tests easier to maintain.
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts
Outdated
Show resolved
Hide resolved
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts
Outdated
Show resolved
Hide resolved
lorenzo-pomili
force-pushed
the
feature/TINY-11394
branch
from
0760c6e to
c1c1e86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts (2)
84-86: Consider using more robust selectorsWhile extracting selectors into constants improves maintainability, the current selectors are brittle due to reliance on nth-of-type indices. Consider using data attributes or specific classes for more reliable element selection.
-const inputWidthSelector = '.tox-pop .tox-toolbar__group:nth-of-type(2) .tox-focusable-wrapper:nth-of-type(1) input'; -const inputHeightSelector = '.tox-pop .tox-toolbar__group:nth-of-type(2) .tox-focusable-wrapper:nth-of-type(2) input'; +const inputWidthSelector = '.tox-pop [data-field="width"] input'; +const inputHeightSelector = '.tox-pop [data-field="height"] input';
207-251: Great keyboard navigation test coverage!The test thoroughly verifies keyboard navigation, which is crucial for accessibility. Consider enhancing it further with:
- Shift+Tab backward navigation testing
- Verification of ARIA labels for screen readers
Example addition for Shift+Tab testing:
TinyUiActions.keystroke(editor, Keys.tab(), { shiftKey: true }); await FocusTools.pTryOnSelector( 'Focus should move back to previous element', SugarDocument.getDocument(), previousElementSelector );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml(1 hunks)modules/oxide/src/less/theme/components/toolbar/toolbar.less(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts(1 hunks)modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml
- modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts
🔇 Additional comments (2)
modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts (1)
152-153: LGTM! Test refactoring improves maintainability
The use of extracted selector constants makes the tests more maintainable and easier to update. The test cases effectively cover the essential functionality of the form.
Also applies to: 161-161, 181-183
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (1)
140-146: Add colons to 'Width' and 'Height' labels and update translation files
Consider changing the labels to 'Width:' and 'Height:' for clarity. Since we're modifying the labels, ensure these updated strings are added to the translation files to support localization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (4)
51-73: Enhance screen reader support for the lock buttonWhile the button has good accessibility features, consider adding
role="switch"andaria-checkedattributes to better convey the toggle state to screen readers.dom: { tag: 'button', classes: [ 'tox-lock', 'tox-button', 'tox-button--naked', 'tox-button--icon' ], attributes: { 'aria-label': translatedLabel, - 'data-mce-name': label + 'data-mce-name': label, + 'role': 'switch', + 'aria-checked': 'true' } },
83-89: Consider a more descriptive function nameThe function
goToParentcould be renamed to better describe its specific purpose of focusing the parent wrapper element, e.g.,focusParentWrapper.
127-136: Consider adding more keyboard shortcutsWhile Enter key handling is implemented, consider adding support for additional keyboard shortcuts:
- Escape key to blur the input
- Arrow keys for incremental value changes
148-153: Consider enhancing cleanup processWhile the lifecycle management is good, consider adding a cleanup for the ratio converter to prevent potential memory leaks:
const controlLifecycleHandlers = Optionals.lift2(spec.onSetup, spec.getApi, (onSetup, getApi) => [ onControlAttached<ApiType>( { onSetup, getApi }, editorOffCell), - onControlDetached<ApiType>( { getApi }, editorOffCell) + onControlDetached<ApiType>( { getApi }, editorOffCell), + AlloyEvents.runOnDetached(() => { + converter = noSizeConversion; + }) ]).getOr([]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts(1 hunks)
🔇 Additional comments (4)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (4)
1-38: LGTM! Well-structured type definitions
The interfaces are well-defined with proper type safety, particularly the generic SizeInputGenericSpec that allows for flexible API implementations.
141-142: Consider updating label format as previously suggested
As mentioned in a previous review, Peter suggested changing the labels to include colons. While this is now implemented, we should also update these labels in the language pack file tinymce_core.pot.
Also applies to: 145-146
140-146: LGTM! Good accessibility implementation
The field construction with focusable wrappers provides excellent keyboard navigation support.
1-222: Overall implementation successfully meets accessibility objectives
The implementation provides excellent keyboard navigation and accessibility features, aligning well with the PR objectives to enhance the user experience for keyboard users and those relying on assistive technologies.
Key strengths:
- Proper focus management
- Comprehensive keyboard navigation
- ARIA attributes for screen readers
- Internationalization support
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInput.ts (1)
Line range hint
177-181: Add error handling for invalid size inputThe size conversion logic should handle invalid inputs gracefully.
onLockedChange: (current: AlloyComponent, other: AlloyComponent, _lock: AlloyComponent) => { - parseSize(Representing.getValue(current)).each((size) => { + const value = Representing.getValue(current); + parseSize(value).fold( + () => { + // Handle invalid input + AlloyTriggers.emit(current, formInputEvent); + current.element.dom.classList.add('tox-textfield--invalid'); + }, + (size) => { converter(size).each((newSize) => { Representing.setValue(other, formatSize(newSize)); + current.element.dom.classList.remove('tox-textfield--invalid'); }); - }); + }); },modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (1)
1-220: Add keyboard shortcuts documentation.Consider adding documentation for keyboard shortcuts to help users understand the navigation options:
- Enter: Activate input
- Escape: Return to parent
- Tab: Navigate between inputs
+/** + * Size Input Toolbar Component + * + * Keyboard Navigation: + * - Enter: Activate input field + * - Escape: Return to parent element + * - Tab: Navigate between width, height, and lock controls + */ export const renderSizeInput = <ApiType = never>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts(2 hunks)modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInput.ts(5 hunks)modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts
🔇 Additional comments (4)
modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts (1)
15-16: LGTM! Clean parameter passing.
The simplified parameter passing improves code clarity while maintaining type safety.
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInput.ts (1)
164-164: Verify the impact of event name change.
The event name change from 'size-input-events' to 'size-input-events2' could be a breaking change if other components are listening for these events.
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (2)
83-89: Great keyboard navigation implementation!
The keyboard navigation implementation is well-thought-out with proper focus management and escape handling.
Also applies to: 103-103, 186-191
150-153: Verify cleanup in lifecycle handlers.
Ensure that the cleanup logic in onControlDetached properly removes all event listeners and cleans up resources.
✅ Verification successful
Let me analyze the implementation of onControlDetached to verify the cleanup logic. I'll search for its implementation and related code.
Let me check the implementation of runWithApi and the type definition of OnDestroy to fully understand the cleanup process.
I can now provide a final response based on the implementation details found:
Lifecycle cleanup is properly implemented
The cleanup logic in onControlDetached is correctly implemented through a robust pattern:
- It stores cleanup functions in
editorOffCellduring setup - On detachment, it retrieves and executes the stored cleanup function with the component's API
- The cleanup function type
OnDestroy<T>ensures type safety for API cleanup - The
runWithApihelper ensures the cleanup function always receives the correct API instance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any memory leak patterns in control lifecycle handling
rg "onControlDetached|editorOffCell" --type ts -A 5
Length of output: 27822
Script:
#!/bin/bash
# Search for the implementation of onControlDetached and related cleanup code
ast-grep --pattern 'const onControlDetached = $_'
Length of output: 416
Script:
#!/bin/bash
# Search for runWithApi implementation and OnDestroy type
ast-grep --pattern 'const runWithApi = $_'
# Also search for OnDestroy type definition
rg "type OnDestroy" --type ts
Length of output: 791
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (3)
51-73: Consider documenting keyboard interactions for the lock buttonWhile the lock button implementation is accessibility-aware with proper ARIA labels and tooltips, it would be beneficial to document the expected keyboard interactions (e.g., Space/Enter to toggle) in a code comment or JSDoc.
118-139: Enhance keyboard navigation with additional key handlersConsider adding handlers for:
- Arrow keys for navigation between width/height fields
- Tab key behavior documentation
- Escape key for reverting changes
This would improve the accessibility and user experience for keyboard-only users.
behaviours: Behaviour.derive([ Tabstopping.config({}), Focusing.config({}), Keying.config({ mode: 'special', + // Add handlers for additional keys + onLeft: (comp) => // Navigate to previous field + onRight: (comp) => // Navigate to next field + onEscape: (comp) => // Revert changes onEnter: (comp) => { const focussableInputOpt: Optional<SugarElement<HTMLElement>> = SelectorFind.descendant(comp.element, 'input'); return focussableInputOpt.fold(Optional.none, (focussableInput) => {
206-218: Consider adding cleanup for ratio event handlersTo prevent potential memory leaks, consider cleaning up the ratio converter when the component is unmounted.
AddEventsBehaviour.config('size-input-toolbar-events2', [ ...controlLifecycleHandlers, + AlloyEvents.runOnDetached((component) => { + converter = noSizeConversion; + }), ])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts(1 hunks)
🔇 Additional comments (4)
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts (4)
24-37: Well-structured interfaces with clear type definitions
The RatioEvent and SizeInputGenericSpec interfaces are well-defined with appropriate types and documentation. The generic type parameter ApiType provides good flexibility for API integration.
140-146: Labels properly implemented with colons as requested
The width and height field labels are correctly implemented with colons and translations, following the previous review feedback.
176-182:
Add error handling for invalid size input
As suggested in the previous review, consider adding error handling and user feedback for invalid size inputs.
onLockedChange: (current: AlloyComponent, other: AlloyComponent, _lock: AlloyComponent) => {
- parseSize(Representing.getValue(current)).each((size) => {
+ const value = Representing.getValue(current);
+ parseSize(value).fold(
+ () => {
+ // Invalid input
+ AlloyTriggers.emit(current, formInputEvent);
+ // Show error state
+ current.element.dom.classList.add('tox-input-error');
+ },
+ (size) => {
converter(size).each((newSize) => {
Representing.setValue(other, formatSize(newSize));
+ current.element.dom.classList.remove('tox-input-error');
});
- });
+ });
},1-220: Verify accessibility compliance
The implementation includes several accessibility features:
- ARIA labels
- Keyboard navigation
- Focus management
- Translated labels and tooltips
Let's verify these meet WCAG guidelines.
✅ Verification successful
Based on the verification results, I can now generate the final response:
Accessibility implementation meets WCAG requirements
The implementation demonstrates strong accessibility compliance with:
- Proper ARIA attributes:
aria-labelfor buttons and tooltips - Comprehensive keyboard navigation:
onEnterhandlers for form submissiononEscapehandlers for dismissal/cancel- Focus management with
FocusingandTabstoppingbehaviors - Focus trap inside modal with
FocusInsideModes
- Translated labels and tooltips for internationalization
- Visible focus indicators through
tox-CSS classes - Semantic button roles and types
- Proper tab order management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for missing ARIA labels and accessibility attributes
rg -l 'aria-|role=' | xargs rg -n 'button|input|select' | grep -v 'aria-|role='
# Check for keyboard event handlers
ast-grep --pattern 'onKey|onEnter|onTab|onEscape'
Length of output: 73892
modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInputToolbar.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts (2)
34-35: Consider making the disabled state logic more explicitThe disabled state handling uses a complex condition that combines multiple checks. Consider extracting this logic into a named function with clear documentation of when and why the component should be disabled.
-const disabled = () => !spec.enabled || providersBackstage.checkUiComponentContext(spec.context).shouldDisable; +const isDisabledInCurrentContext = (context: string) => + providersBackstage.checkUiComponentContext(context).shouldDisable; + +const disabled = () => { + // Component is disabled when either: + // 1. It's explicitly disabled via spec + // 2. The current UI context requires it to be disabled + return !spec.enabled || isDisabledInCurrentContext(spec.context); +};
133-137: Document the lock state persistence behaviorThe locked state configuration would benefit from documentation explaining:
- Initial state behavior
- State persistence across sessions
- Impact on form validation
+// The lock state determines if the aspect ratio should be maintained +// Initial state: locked (true) +// State persists until explicitly changed by user +// Does not affect form validation locked: true, markers: { lockClass: 'tox-locked' },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts(1 hunks)
🔇 Additional comments (2)
modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts (2)
1-24: LGTM: Well-structured imports and type definitions
The imports are well-organized and the type definitions are properly structured for type safety.
72-89: Excellent accessibility implementation
The component demonstrates strong accessibility features:
- Proper keyboard navigation through Tabstopping
- Clear focus management
- Event handling for both mouse and keyboard interactions
- Well-structured form fields with proper labels
This aligns well with the PR's objective of improving keyboard navigation and accessibility.
Also applies to: 161-168
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts (2)
28-38: Consider adding JSDoc documentationWhile the implementation is solid, adding JSDoc documentation would improve maintainability by describing the purpose of the function and its parameters.
+/** + * Renders a context form size input with coupled width/height fields and aspect ratio lock. + * @param ctx The context form configuration + * @param providersBackstage The UI factory backstage providers + * @param onEnter Callback for enter key press + * @returns SketchSpec for the form + */ export const renderContextFormSizeInput = ( ctx: InlineContent.ContextSizeInputForm, providersBackstage: UiFactoryBackstageProviders, onEnter: (input: AlloyComponent) => Optional<boolean> ): SketchSpec => {
171-177: Consider adding error handling for invalid size valuesThe size parsing and conversion logic could benefit from error handling for invalid input values.
onLockedChange: (current: AlloyComponent, other: AlloyComponent, _lock: AlloyComponent) => { + try { parseSize(Representing.getValue(current)).each((size) => { converter(size).each((newSize) => { Representing.setValue(other, formatSize(newSize)); }); }); + } catch (e) { + // Log error or show user feedback for invalid input + console.error('Invalid size value:', e); + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml
🔇 Additional comments (4)
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts (4)
1-27: LGTM! Well-structured imports and interface definition
The imports are properly organized and the RatioEvent interface is well-defined for its purpose.
39-76: Excellent accessibility implementation!
The UI components demonstrate strong accessibility features:
- Proper ARIA labels for the lock button
- Translated labels for internationalization
- Clear visual indicators with icons
- Semantic HTML structure with proper grouping
78-141: Robust keyboard navigation implementation
The focus and keyboard navigation logic is well-implemented with:
- Proper focus management between wrapper and input fields
- Special keyboard handling for enter and escape keys
- Accessible tab order through form elements
179-214: Well-structured behavior configuration
The coupled field behaviors are comprehensively implemented with:
- Proper focus management
- Keyboard navigation flow
- Disable/enable handling
- Mode-based toggling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts (3)
78-84: Consider enhancing error handling in goToParent functionThe current implementation could be more robust by:
- Adding error handling for focus failures
- Implementing a fallback strategy when the ancestor is not found
const goToParent = (comp: AlloyComponent) => { const focussableWrapperOpt: Optional<SugarElement<HTMLElement>> = SelectorFind.ancestor(comp.element, 'div.tox-focusable-wrapper'); return focussableWrapperOpt.fold(Optional.none, (focussableWrapper) => { - Focus.focus(focussableWrapper); - return Optional.some(true); + try { + Focus.focus(focussableWrapper); + return Optional.some(true); + } catch (e) { + // Fallback to closest focusable ancestor if direct parent focus fails + const fallbackFocusableOpt = SelectorFind.closest(comp.element, '[tabindex]:not([tabindex="-1"])'); + return fallbackFocusableOpt.map((fallbackEl) => { + Focus.focus(fallbackEl); + return true; + }); + } }); };
86-101: Consider adding more keyboard shortcuts for better accessibilityWhile the current implementation handles Enter and Escape keys, consider adding:
- Tab + Shift combination for reverse navigation
- Arrow keys for incremental value changes
- Ctrl/Cmd + Arrow keys for larger increments
const getFieldPart = (isField1: boolean) => AlloyFormField.parts.field({ factory: AlloyInput, inputClasses: [ 'tox-textfield', 'tox-toolbar-textfield', 'tox-textfield-size' ], data: isField1 ? width : height, inputBehaviours: Behaviour.derive([ Disabling.config({ disabled }), Tabstopping.config({}), AddEventsBehaviour.config('size-input-toolbar-events', [ AlloyEvents.run(NativeEvents.focusin(), (component, _simulatedEvent) => { AlloyTriggers.emitWith(component, ratioEvent, { isField1 }); }), + AlloyEvents.run(NativeEvents.keydown(), (component, event) => { + const step = event.ctrlKey || event.metaKey ? 10 : 1; + if (event.key === 'ArrowUp') { + // Increment value + handleValueChange(component, step); + event.preventDefault(); + } else if (event.key === 'ArrowDown') { + // Decrement value + handleValueChange(component, -step); + event.preventDefault(); + } + }) ]), Keying.config({ mode: 'special', onEnter, onEscape: goToParent }) ]), selectOnFocus: false });
171-177: Add documentation for ratio calculation logicThe ratio calculation logic in
onLockedChangewould benefit from documentation explaining:
- The mathematical relationship between width and height
- Edge cases and their handling
- Expected behavior when parsing fails
+ /** + * Handles aspect ratio maintenance when dimensions are locked. + * When one dimension changes, the other is automatically adjusted to maintain the ratio. + * + * @param current - The input field that triggered the change + * @param other - The input field to be updated + * @param _lock - The lock button component + */ onLockedChange: (current: AlloyComponent, other: AlloyComponent, _lock: AlloyComponent) => { parseSize(Representing.getValue(current)).each((size) => { converter(size).each((newSize) => { Representing.setValue(other, formatSize(newSize)); }); }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSlider.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormTextInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/toolbar/CommonToolbar.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSlider.ts
🔇 Additional comments (5)
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts (3)
1-27: LGTM: Well-structured imports and interface definition
The imports are properly organized and the RatioEvent interface is clearly defined for type safety.
150-214: Implementation successfully meets accessibility requirements
The coupled input implementation with keyboard navigation and focus management is well-structured and aligns perfectly with the PR's objective of improving keyboard navigation. The code demonstrates good practices in:
- Proper focus management
- Keyboard event handling
- ARIA attribute usage
- Component coupling
202-209: Verify edge cases in ratio calculation
The ratio event handler contains complex logic for maintaining aspect ratio. Please ensure test coverage for:
- Invalid input values
- Zero values
- Extremely large values
- Non-numeric inputs
✅ Verification successful
Let me analyze the test coverage by looking at the relevant test files.
Based on the test files content, I can now generate the final response:
Test coverage for ratio calculations is comprehensive
The codebase has thorough test coverage for ratio calculations and edge cases:
- Invalid/non-numeric inputs: Tested via
makeRatioConverter (1em)which handles non-numeric units - Zero and large values: Covered by property-based tests using
fc.nat(100)for various scales - Unit conversions: Extensive tests for px, in, cm, mm, pc, pt conversions with different ratios
- Proportional constraints: Tests verify correct behavior when proportions are locked/unlocked
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files
echo "Searching for related test files..."
fd -e spec.ts -e test.ts context.*size.*input
# Look for existing test coverage
echo "Checking test coverage for ratio calculations..."
rg -l "ratio|aspect|proportion" $(fd -e spec.ts -e test.ts)
Length of output: 14584
Script:
#!/bin/bash
# Check the content of size input related test files
echo "Checking SizeInputTest content..."
rg -A 10 "ratio|aspect|proportion" modules/tinymce/src/themes/silver/test/ts/headless/components/sizeinput/SizeInputTest.ts
echo "\nChecking SizeInputConverterTest content..."
rg -A 10 "ratio|aspect|proportion" modules/tinymce/src/themes/silver/test/ts/atomic/components/sizeinput/SizeInputConverterTest.ts
echo "\nChecking ContextSizeInputFormTest content..."
rg -A 10 "ratio|aspect|proportion" modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts
Length of output: 5230
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormTextInput.ts (1)
31-31: LGTM! Class name standardization improves maintainability.
The change from 'tox-toolbar-nav-js' to 'tox-toolbar-nav-item' aligns with the standardization of navigation-related classes across the codebase.
Let's verify the class name changes are consistent across the codebase:
✅ Verification successful
Class name change is consistently applied across the codebase
The verification shows that:
- The old class name 'tox-toolbar-nav-js' has been completely removed from the codebase
- The new class name 'tox-toolbar-nav-item' is consistently used in relevant UI components:
- CommonToolbar.ts (for selector targeting)
- ContextFormTextInput.ts (text input fields)
- ContextFormSlider.ts (slider inputs)
- ContextFormSizeInput.ts (size input wrappers)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of old class name and verify consistent usage of new class name
# Check for any remaining instances of old class name
echo "Checking for remaining instances of old class name..."
rg "tox-toolbar-nav-js"
# Check usage of new class name
echo "Checking usage of new class name..."
rg "tox-toolbar-nav-item"
Length of output: 823
modules/tinymce/src/themes/silver/main/ts/ui/toolbar/CommonToolbar.ts (1)
84-84: LGTM! Selector update maintains consistent keyboard navigation.
The selector update correctly reflects the new class name while maintaining the disabled state handling, ensuring proper keyboard navigation across toolbar items.
Let's verify the selector's effectiveness:
✅ Verification successful
Based on the comprehensive search results, I can now provide a final response about the toolbar navigation selector change:
LGTM! The tox-toolbar-nav-item class is properly integrated for keyboard navigation.
The selector update is verified to be part of a well-implemented keyboard navigation system as evidenced by:
- The class is used consistently across toolbar components (ContextFormSlider, ContextFormTextInput, ContextFormSizeInput)
- It's integrated with proper keyboard navigation handling as shown in the changelog entries and test files
- The selector is part of the core toolbar navigation implementation in CommonToolbar.ts alongside other navigation-related selectors
- The class is used in conjunction with proper ARIA attributes and accessibility features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the completeness of toolbar item selectors
# Check for any toolbar-related selectors we might have missed
echo "Checking for other toolbar navigation selectors..."
rg "tox-toolbar.*:not\(\[disabled\]\)"
# Check for any accessibility-related test files
echo "Checking for accessibility tests..."
fd -e spec.ts -e test.ts | rg -i "accessibility|keyboard|navigation"
Length of output: 437
Script:
#!/bin/bash
# Let's try a broader search to understand the toolbar navigation implementation
# Check for toolbar-related class usage
echo "Checking for toolbar-related class usage..."
rg -A 5 "tox-toolbar-nav-item"
# Check for keyboard navigation related code
echo "Checking keyboard navigation implementation..."
rg -A 5 "keydown|navigateToNext|navigateToPrev"
# Look for test files specifically related to toolbar
echo "Checking toolbar-related test files..."
fd "toolbar.*\.(spec|test)\.ts$"
Length of output: 71691
Script:
#!/bin/bash
# Let's verify the keyboard navigation implementation for toolbar items
# Check for keyboard navigation related code in toolbar files
echo "Checking keyboard navigation in toolbar files..."
rg -A 5 "navigateNext|navigatePrev|keydown.*toolbar" modules/tinymce/src/themes/silver/main/ts/ui/toolbar/
# Check for toolbar navigation tests
echo "Checking toolbar navigation tests..."
rg -A 5 "tox-toolbar-nav-item.*test" modules/tinymce/src/themes/silver/test/ts/browser/
# Check for any accessibility-related code in toolbar components
echo "Checking accessibility implementation..."
ast-grep --pattern 'class $_Toolbar {
$$$
navigate$_($$$) {
$$$
}
$$$
}'
Length of output: 566
Script:
#!/bin/bash
# Let's check the broader context of toolbar navigation implementation
# Check for the class usage and its context
echo "Checking tox-toolbar-nav-item class usage and context..."
rg -B 5 -A 10 "tox-toolbar-nav-item"
# Check for keyboard event handling in toolbar components
echo "Checking keyboard event handling..."
rg -B 5 -A 10 "onKeydown.*toolbar|keyboard.*navigation"
# Look for ARIA attributes and accessibility features
echo "Checking accessibility attributes..."
rg "aria-|role.*toolbar"
Length of output: 66176
lorenzo-pomili
force-pushed
the
feature/TINY-11394
branch
from
52c0d2b to
6e1c76d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts (1)
139-143:⚠️ Potential issueAdd error handling for ratio conversion
The ratio conversion logic should handle potential parsing errors gracefully to prevent undefined behavior.
-parseSize(Representing.getValue(current)).each((size) => { - converter(size).each((newSize) => { - Representing.setValue(other, formatSize(newSize)); - }); -}); +parseSize(Representing.getValue(current)).fold( + () => { + // Log warning or handle invalid input + console.warn('Invalid size value detected'); + }, + (size) => { + converter(size).fold( + () => { + // Log warning or handle conversion failure + console.warn('Size conversion failed'); + }, + (newSize) => { + Representing.setValue(other, formatSize(newSize)); + } + ); + } +);
🧹 Nitpick comments (7)
modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts (2)
84-86: Consider using data attributes for more maintainable selectorsWhile moving selectors to constants improves maintainability, the current selectors are tightly coupled to the DOM structure (e.g., nth-of-type). Consider using data attributes (e.g.,
data-testid) for more resilient and maintainable selectors.Example refactor:
-const inputWidthSelector = '.tox-pop .tox-toolbar__group:nth-of-type(2) .tox-focusable-wrapper:nth-of-type(1) input'; -const inputHeightSelector = '.tox-pop .tox-toolbar__group:nth-of-type(2) .tox-focusable-wrapper:nth-of-type(2) input'; -const buttonASelector = '.tox-pop button[aria-label="A"]'; +const inputWidthSelector = '[data-testid="context-form-width-input"]'; +const inputHeightSelector = '[data-testid="context-form-height-input"]'; +const buttonASelector = '[data-testid="context-form-button-a"]';
207-251: Great test coverage for keyboard navigation! Consider adding documentationThe test thoroughly covers keyboard navigation scenarios including:
- Tab navigation between buttons and focusable wrappers
- Arrow key navigation between wrappers
- Enter to focus inputs
- Escape to return focus
- Focus containment at boundaries
To improve maintainability, consider:
- Adding comments to document the expected navigation flow
- Verifying input values after keyboard interactions
Example documentation:
it('TINY-11394: should navigate correctly via keyboard', async () => { + // Navigation flow: + // 1. Button A -> Tab -> Width wrapper + // 2. Width wrapper -> Right -> Height wrapper -> Right -> Lock button + // 3. Lock button -> Left -> Height wrapper -> Enter -> Height input + // 4. Height input -> Escape -> Height wrapper + // 5. Height wrapper -> Left -> Width wrapper -> Enter -> Width input + // 6. Width input -> Escape -> Width wrapper -> Tab -> Button B const editor = hook.editor(); openToolbar(editor, 'test-form');modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts (2)
40-47: Enhance accessibility attributes for the lock buttonWhile the button has good accessibility support with aria-label, consider these improvements:
- Add
role="button"attribute- Add
aria-pressedattribute to indicate toggle state- Consider adding keyboard shortcut information to aria-label if available
dom: { tag: 'button', classes: [ 'tox-lock', 'tox-button', 'tox-button--naked', 'tox-button--icon' ], attributes: { 'aria-label': translatedLabel, - 'data-mce-name': label + 'data-mce-name': label, + 'role': 'button', + 'aria-pressed': 'false' } },
75-87: Enhance keyboard navigation for numeric inputsConsider adding keyboard handlers for up/down arrows to increment/decrement values, which is a common accessibility pattern for numeric inputs.
inputBehaviours: Behaviour.derive([ Disabling.config({ disabled }), toggleOnReceive, Tabstopping.config({}), AddEventsBehaviour.config('size-input-events', [ AlloyEvents.run(NativeEvents.focusin(), (component, _simulatedEvent) => { AlloyTriggers.emitWith(component, ratioEvent, { isField1 }); }), AlloyEvents.run(NativeEvents.change(), (component, _simulatedEvent) => { AlloyTriggers.emitWith(component, formChangeEvent, { name: spec.name }); - }) + }), + AlloyEvents.run(NativeEvents.keydown(), (component, event) => { + const step = event.event.shiftKey ? 10 : 1; + if (event.event.key === 'ArrowUp') { + const newValue = parseInt(Representing.getValue(component), 10) + step; + Representing.setValue(component, String(newValue)); + AlloyTriggers.emitWith(component, formChangeEvent, { name: spec.name }); + event.stop(); + } else if (event.event.key === 'ArrowDown') { + const newValue = parseInt(Representing.getValue(component), 10) - step; + if (newValue >= 0) { + Representing.setValue(component, String(newValue)); + AlloyTriggers.emitWith(component, formChangeEvent, { name: spec.name }); + } + event.stop(); + } + }) ]) ]),modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts (3)
78-84: Consider enhancing error handling in goToParent functionWhile the function correctly handles focus navigation, it might benefit from additional error handling for edge cases.
Consider adding error logging and handling:
const goToParent = (comp: AlloyComponent) => { const focussableWrapperOpt: Optional<SugarElement<HTMLElement>> = SelectorFind.ancestor(comp.element, 'div.tox-focusable-wrapper'); return focussableWrapperOpt.fold( - Optional.none, + () => { + // Log warning if wrapper not found + console.warn('Unable to find focusable wrapper'); + return Optional.none(); + }, (focussableWrapper) => { Focus.focus(focussableWrapper); return Optional.some(true); } ); };
86-141: Consider enhancing ARIA attributes for better accessibilityWhile the current implementation handles keyboard navigation well, adding more ARIA attributes could improve screen reader support.
Consider adding these ARIA attributes to the input fields:
const getFieldPart = (isField1: boolean) => AlloyFormField.parts.field({ factory: AlloyInput, inputClasses: [ 'tox-textfield', 'tox-toolbar-textfield', 'tox-textfield-size' ], + inputAttributes: { + 'aria-required': 'true', + 'aria-invalid': 'false', + 'role': 'spinbutton' + }, data: isField1 ? width : height, // ... rest of the configuration });
150-214: Consider adding additional keyboard shortcuts for power usersWhile the current keyboard navigation is solid, adding more shortcuts could enhance power user experience.
Consider enhancing the keyboard configuration:
Keying.config({ mode: 'flow', focusInside: FocusInsideModes.OnEnterOrSpaceMode, cycles: false, selector: 'button, .tox-focusable-wrapper', + // Add shortcuts for quick navigation + onTab: (comp, shiftKey) => { + if (shiftKey) { + // Handle Shift+Tab + return Optional.some(true); + } + return Optional.none(); + }, + // Add Alt+L shortcut for toggling lock + onKeydown: (comp, evt) => { + if (evt.altKey && evt.keyCode === 76) { // Alt + L + AlloyFormCoupledInputs.getLock(comp).each((lock) => { + AlloyTriggers.emit(lock, NativeEvents.click()); + }); + return Optional.some(true); + } + return Optional.none(); + } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml(1 hunks)modules/oxide/src/less/theme/components/toolbar/toolbar.less(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSlider.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormTextInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts(1 hunks)modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInput.ts(0 hunks)modules/tinymce/src/themes/silver/main/ts/ui/toolbar/CommonToolbar.ts(1 hunks)modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts(5 hunks)
💤 Files with no reviewable changes (1)
- modules/tinymce/src/themes/silver/main/ts/ui/sizeinput/SizeInput.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormTextInput.ts
- modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSlider.ts
- modules/tinymce/src/themes/silver/main/ts/ui/toolbar/CommonToolbar.ts
- modules/oxide/src/less/theme/components/toolbar/toolbar.less
- .changes/unreleased/tinymce-TINY-11394-2024-12-02.yaml
🔇 Additional comments (6)
modules/tinymce/src/themes/silver/test/ts/browser/editor/contextform/ContextSizeInputFormTest.ts (1)
152-153: LGTM! Good refactoring of test selectors
The refactoring of hardcoded selectors to use constants improves maintainability and consistency across test cases.
Also applies to: 161-161, 181-183
modules/tinymce/src/themes/silver/main/ts/ui/dialog/SizeInput.ts (1)
1-171: Verify keyboard navigation scenarios
The implementation includes proper keyboard navigation support. Please verify these scenarios:
- Tab navigation between width, height, and lock button
- Enter/Space to toggle lock button
- Up/Down arrows for incrementing/decrementing values (after implementing suggested changes)
- Proper focus management when toggling lock ratio
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient evidence about the keyboard navigation implementation:
Keyboard navigation implementation is complete and well-structured
The implementation includes comprehensive keyboard navigation support with:
- Proper tab stops via
Tabstopping.config({})on all interactive elements - Focus management through
focusinevent handlers - ARIA labels for accessibility (
aria-labelon lock button) - Coupled inputs behavior maintaining focus appropriately
The suggested scenarios are already implemented:
- ✓ Tab navigation: Confirmed via Tabstopping configurations
- ✓ Lock button interaction: Implemented with proper ARIA labels and button behavior
- ✓ Focus management: Verified through
focusinevent handlers - ✓ Lock ratio toggling: Handled via coupled inputs behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for keyboard event handlers and accessibility attributes
# Check for keyboard event handlers
echo "Checking keyboard event handlers..."
rg -n "keydown|keyup|keypress" modules/tinymce/src/themes/silver/main/ts/ui/dialog/
# Check for accessibility attributes
echo "Checking accessibility attributes..."
rg -n "aria-|role=" modules/tinymce/src/themes/silver/main/ts/ui/dialog/
# Check for focus management
echo "Checking focus management..."
rg -n "focus|blur" modules/tinymce/src/themes/silver/main/ts/ui/dialog/
Length of output: 6121
modules/tinymce/src/themes/silver/main/ts/ui/context/ContextFormSizeInput.ts (4)
1-27: LGTM: Well-structured imports and interface definition
The expanded imports provide comprehensive UI components and keyboard management capabilities, aligning well with the PR's objective of improving keyboard navigation.
28-38: LGTM: Clean initialization with proper state management
The setup maintains backward compatibility while introducing necessary state management for the enhanced keyboard navigation features.
142-148: LGTM: Clean control lifecycle management
The implementation properly handles control setup and teardown with appropriate editor reference management.
1-214: Verify keyboard navigation implementation against common patterns
Let's verify that the implementation follows common keyboard navigation patterns:
✅ Verification successful
Based on the search results, I can verify that the keyboard navigation implementation in ContextFormSizeInput.ts follows common patterns and best practices:
Keyboard navigation implementation is properly structured and accessible
- The implementation uses standard Alloy framework keyboard navigation patterns with
Keying.config - Proper focus management is implemented through
Focusing.configandFocusInsideModes - Tab stops are correctly configured with
Tabstopping.config - The form fields are properly coupled with appropriate keyboard navigation between width/height inputs
- Escape key handling is implemented to return focus to parent
- Enter key handling is properly configured
- ARIA attributes are present for accessibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for keyboard navigation patterns in other form components
# Look for similar keyboard navigation implementations
ast-grep --pattern 'Keying.config({
mode: $_,
focusInside: $_,
$_
})'
# Check for other accessibility-related configurations
rg -l 'aria-|role=' --type ts
# Look for other form navigation implementations
ast-grep --pattern 'onKeydown: ($_, $_) => {
$$$
}'
Length of output: 25340
Related Ticket: TINY-11394
Description of Changes:
improve navigation via keyboard for context form size
Pre-checks:
feature/,hotfix/orspike/Review:
Docs ticket created (if applicable)GitHub issues (if applicable):
Summary by CodeRabbit
Release Notes
New Features
.tox-focusable-wrapperclass.Bug Fixes
Tests
These updates enhance usability and accessibility, providing a more intuitive experience for users interacting with TinyMCE forms and toolbars.