fix(ip-input): retain cursor position when inserting characters #1349
Conversation
Summary of ChangesHello @spliffone, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the cursor position was not correctly retained in the IPv6 input component after character insertion, particularly when the input value was automatically formatted. The changes introduce a mechanism to calculate and apply a cursor adjustment, significantly improving the user experience by preventing unexpected cursor jumps and ensuring a more natural typing flow. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with cursor positioning in the IPv6 input component. The changes introduce a cursorDelta to adjust the cursor position after formatting. However, the logic for calculating this delta is flawed, as it compares indices from different coordinate systems (raw input vs. cleaned section value). This can lead to incorrect cursor placement when inserting characters in the middle of an IP address segment. Additionally, the new tests do not cover these middle-insertion scenarios, so the bug is not caught. I've provided a detailed explanation of the bug and a suggested fix, along with a new test case that would expose the issue and verify the correction.
| if (current && pos >= charsProcessed) { | ||
| cursorDelta++; | ||
| } |
There was a problem hiding this comment.
The logic to calculate cursorDelta is incorrect because it compares pos (an index in the raw input string) with charsProcessed (an index within a cleaned section value). This can lead to incorrect cursor positioning when inserting characters in the middle of an IP address segment, as pos and charsProcessed are in different coordinate systems.
To fix this, you need to determine the cursor's position relative to the hexadecimal characters within its section.
Here is a suggested approach:
-
In the first
forloop (lines 111-128), determine the cursor's position relative to the hex characters in its section and store it.let cursorRelativePos = -1; for (let i = 0; i <= input.length; i++) { if (pos === i) { sections.at(-1)!.current = true; cursorRelativePos = sections.at(-1)!.value.length; } if (i >= input.length) { continue; } // ... existing logic to build sections ... }
-
Then, use this
cursorRelativePoshere to correctly calculatecursorDelta.if (current && cursorRelativePos >= charsProcessed + 4) { cursorDelta++; }
Additionally, the new tests for cursor positioning in si-ip6-input.directive.spec.ts only cover appending characters. It would be beneficial to add a test case for inserting a character in the middle of a section to verify the fix and prevent future regressions.
| expect(input.value).toBe('2001:DB8::1/64'); | ||
| expect(input.selectionStart).toBe(14); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The current tests for cursor positioning do not cover cases where a section split is triggered by inserting characters in the middle of a string. This scenario is where the cursorDelta calculation is most critical. Please add a new test case to cover this, which will help validate the fix for the logic in address-utils.ts.
it('should retain cursor position when section splits from middle insertion', () => {
// Simulate typing 'a' in '12345678' at position 4 -> '1234a|5678'
input.value = '1234a5678';
input.setSelectionRange(5, 5);
input.dispatchEvent(new InputEvent('input', { data: 'a', inputType: 'insertText' }));
expect(input.value).toBe('1234:A567:8');
expect(input.selectionStart).toBe(6);
});
|
|
|
Documentation. Coverage Reports: |
Uh oh!
There was an error while loading. Please reload this page.