monkeytype icon indicating copy to clipboard operation
monkeytype copied to clipboard

fix(caret): misalignment issue when switching between carets (@byseif21)

Open byseif21 opened this issue 6 months ago • 6 comments

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 width was 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 subscribe listener in caret.ts for 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);
      

byseif21 avatar Jun 18 '25 02:06 byseif21

  • We could also use $(caret).css("width", ""); in updatePosition(); 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

byseif21 avatar Jun 20 '25 15:06 byseif21

  • We could also use $(caret).css("width", ""); in updatePosition(); 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?

Miodec avatar Jun 23 '25 11:06 Miodec

How about we extract the width modifying parts of updatePosition into a separate function,

  1. 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 show function, before updatePosition?

  1. I think it is better to remove inline styles on Config.caretStyle change rather than on show(), because show() 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).

nadalaba avatar Jun 23 '25 15:06 nadalaba

How about we extract the width modifying parts of updatePosition into a separate function, and call that in the show function, before updatePosition?

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

byseif21 avatar Jun 24 '25 16:06 byseif21

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.

Miodec avatar Jun 26 '25 13:06 Miodec

I meant all the size affecting code, not just resetting the width.

Miodec avatar Jun 27 '25 17:06 Miodec

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.

Miodec avatar Jul 01 '25 17:07 Miodec