fix(caret): misalignment issue when switching between carets (@byseif21)
Description
fixes a caret misalignment issue that occurred when switching between caret styles (e.g., from block/outline/underline to line caret).
Bug:
When switching caret styles, the line caret could retain the width from the previous style (e.g., block), causing it to appear visually incorrect until the next input.
Root Cause:
- The caret's inline
widthwas not reset when changing styles, so the new style inherited the old dimensions. updatePosition()was called before the style was fully applied, leading to incorrect positioning.
Fix
-
Added a
subscribelistener incaret.tsfor the"caretStyle"event. -
When the caret style changes:
-
The caret's inline width is reset:
caret.style.width = ""; -
Its position is immediately recalculated with:
updatePosition(true);
-
- We could also use
$(caret).css("width", "");inupdatePosition();to always clear caret width to prevent line caret width from inheriting but i had some doubt about it being always called while we will only need it acutely once when switching between the carets so lmkwdyt
- We could also use
$(caret).css("width", "");inupdatePosition();to always clear caret width to prevent line caret width from inheriting but i had some doubt about it being always called while we will only need it acutely once when switching between the carets so lmkwdyt
How about we extract the width modifying parts of updatePosition into a separate function, and call that in the show function, before updatePosition?
How about we extract the width modifying parts of
updatePositioninto a separate function,
- width modifying parts of
updatePosition()are
if (newWidth !== "") {
animation.width = newWidth;
} else {
jqcaret.css("width", "");
}
I'm guessing you're not referring to the width animation, so it's just the else {jqcaret.css("width", "");} part.
This part can be removed completely from updatePosition() if we do @byseif21's solution, (we only need to remove inline width style once, when changing from full-width caret to non-full-width caret).
That way updatePosition() does not change caret width (except in animation), and we're still using the cheapest solution imo (updating inline styles only when needed).
and call that in the
showfunction, beforeupdatePosition?
- I think it is better to remove inline styles on Config.caretStyle change rather than on
show(), becauseshow()will be called a lot more than we need to remove inline styles (which is only when changing from full-width to non-full-width caret).
How about we extract the width modifying parts of
updatePositioninto a separate function, and call that in theshowfunction, beforeupdatePosition?
I completely agree with @NadAlaba's analysis here. The config-based approach seems the way to go.
Calling width reset in show() might also cuz hundreds of executions per typing session. The show() function gets triggered on every focus event (which happens constantly), but the actual width inheritance issue only occurs during the rare moments when users change caret styles.
So I think yah the current approach is targeted with minimal performance impact. lmk your final thoughts
We could do both - extract anything that is not position related out of updatePosition and keep the newly added Config listener.
Then we only modify the size styling when needed, and benefit from cleaner code.
I meant all the size affecting code, not just resetting the width.
Sorry for the confusing comments, turns out i misunderstood my own code - I forgot we need to always update the width of the caret in case we are using a block caret + non monospace font.
Anyway, I made some slight changes to clean things up.