fix: end-of-line cursor issue in Safari

[sodenn]

Description

This PR addresses a bug in Safari where the cursor cannot be positioned at the end of a line when the line ends with a contenteditable=false element.

Closes #4487

Of course, other text editors like ProseMirror have the same problem, and they work around it by adding an <img> element before the line break. This PR implements a similar approach and adds an <img> element before the __lexicalLineBreak element.

Please note that there is a method called isManagedLineBreak in LexicalMutations.ts that accesses __lexicalLineBreak. It is unclear to me if the new <img> element causes any issues in this context.

❌ Before

✅ After

For further reference, see:

• https://github.com/ProseMirror/prosemirror/issues/1165#issuecomment-865678282
• https://github.com/ProseMirror/prosemirror-view/commit/68cab14b2a92417317735d50f45ee9583ed9140b
• https://github.com/ProseMirror/prosemirror-view/commit/aaa50d592074c8063fc2ef77907ab6d0373822fb

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	❌ Failed (Inspect)			Jun 25, 2024 6:20pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 25, 2024 6:20pm

[github-actions[bot]]

size-limit report 📦

Path	Size
lexical - cjs	28.47 KB (0%)
lexical - esm	28.28 KB (0%)
@lexical/rich-text - cjs	36.91 KB (0%)
@lexical/rich-text - esm	28.09 KB (0%)
@lexical/plain-text - cjs	35.54 KB (0%)
@lexical/plain-text - esm	25.3 KB (0%)
@lexical/react - cjs	38.82 KB (0%)
@lexical/react - esm	29.27 KB (0%)

[StyleT]

Hi!

Can you pls submit screen recordings before / after fix and try to update unit / e2e test?

And thank you for your contribution!

[sodenn]

@StyleT I've updated the PR and added screen recordings. However, adding tests is a bit tricky since this is a visual browser bug. Meaning, the getSelection position data is correct - even when the cursor is drawn at the wrong position. Maybe we could add a test that captures the cursor position in a screenshot. But this could lead to a flaky test because of the cursor blinking.

[ccapndave]

Thanks so much for doing this! This bug has been the bane of our lives :)

[StyleT]

Hi! Sorry for a long reply here...

I see that this PR fixes the issue with the end-of-line cursor, however when image (as in your videos) is the first element on the line - now you can't place cursor at the start-of-line.

Am I missing something?

[sodenn]

Hi! Sorry for a long reply here...

I see that this PR fixes the issue with the end-of-line cursor, however when image (as in your videos) is the first element on the line - now you can't place cursor at the start-of-line.

Am I missing something?

It is a different problem 😕. The cursor is at the beginning of the line, but now the image hides the cursor.

To clarify, for other elements than images (div, span, etc.), the cursor is not hidden, but is slightly inside the elements. I see exactly the same behavior in other text editors (probably another Safari bug?).

This is the behavior when a span is used instead of an img:

I still believe in most cases this is an improvement to the Safari users.

[wouterlemcke]

Any progress on this? This is a big issue for our project. Would be great if this get fixed! If there is anything I can do to help, please let me know!

[ccapndave]

It was also a big issue for us, so we've been using the PR branch directly and so far haven't had any issue.

[wouterlemcke]

It was also a big issue for us, so we've been using the PR branch directly and so far haven't had any issue.

The branch from @sodenn fork you mean? I installed it using npm install "https://github.com/sodenn/lexical.git#fix/cursor-placement-safari" --save, but I don't see an update (locally) in the changed file..

Probably I'm doing something stupid, is this the same thing you used?

[ccapndave]

So actually I ended up writing a script to use yarn patch and yarn patch-commit to update the build in-place in node_modules. I've pasted it in here in case its any use to you!

#!/usr/bin/env bash
set -e

if [ $# -ne 2 ]; then
    echo "Given a branch and a repo, this builds the lexical package and records a patch in .yarn/patches."
    echo "Usage: yarn patch:lexical <branch> <repo>"
    exit 1
fi

BRANCH=$1
REPO=$2

# Make a temporary directory
TEMP_DIR=$(mktemp -d)

# Clone the fork
git clone -b $BRANCH --single-branch $REPO $TEMP_DIR

# Change directory to the temporary directory
cd $TEMP_DIR

# Set the required node version
echo "nodejs 20.11.0" > ./.tool-versions 

# Build Lexical
npm install
npm run build-release

# Change back to the original directory
cd -

# Get all the lexical dependencies
DEPS=$(jq -r '.dependencies | keys | map(select(test("lexical")))' package.json)
readarray -t DEP_NAMES <<< "$(echo $DEPS | jq -r '.[]')"

# Find the folder for each dependency
for DEP in "${DEP_NAMES[@]}"; do
    # Loop through all directories in $TEMP_DIR/packages
    for DIR in $TEMP_DIR/packages/*; do
        if [ -d "$DIR" ]; then  # Ensure $dir is a directory
            # Check if package.json exists in the directory
            if [ -f "$DIR/package.json" ]; then
                # Extract the name field from package.json
                PACKAGE_NAME=$(jq -r '.name' "$DIR/package.json")
                # Compare with the current dependency name
                if [[ "$PACKAGE_NAME" == "$DEP" ]]; then
                    echo "Patching $PACKAGE_NAME (in $DIR)"

                    echo "PACKAGE_NAME: $PACKAGE_NAME"
                    echo "DIR: $DIR"
                    echo "TEMP_DIR: $TEMP_DIR"
                    echo "DEP: $DEP"

                    # Build the patch
                    PATCH_PATH=$(yarn patch $PACKAGE_NAME --json | jq -r '.path')
                    cp -R $DIR/dist/* $PATCH_PATH

                    # Determine if there is actually a difference
                    set +e
                    git diff --quiet --no-index $PATCH_PATH ./node_modules/$DEP
                    if [ $? -eq 0 ]; then
                        echo "No changes detected for $PACKAGE_NAME."
                    else
                        echo "Patching..."
                        yarn patch-commit -s $PATCH_PATH
                        echo "Generated patch for $PACKAGE_NAME."
                    fi
                    set -e

                    echo
                fi
            fi
        fi
    done
done

[mnlkrs]

@sodenn Thanks for the fix - do you mind rebasing & @StyleT what needs to happen for this fix to be merged? I'm happy to contribute 😃

[ivailop7]

@sodenn Thanks for the fix - do you mind rebasing & @StyleT what needs to happen for this fix to be merged? I'm happy to contribute 😃

Given this is a 5 month old PR, I doubt this one will get traction. My only concern is the platform-specific logic inside the reconciler, which is something we try to avoid at all cost, since testing and troubleshooting complexity grows exponentially. I'd be much happier if we can have this somehow as a standalone plugin that is enabled on/off at playground-level using the variables for the platform - IS_IOS, etc. If such an approach is feasible, it would be much easier to merge this.

[etrepum]

Right now there isn't really anywhere you can extend the reconciler, but the #6759 stuff would make this a bit easier to manage if we merge it

[ivailop7]

Right now there isn't really anywhere you can extend the reconciler, but the #6759 stuff would make this a bit easier to manage if we merge it

I'm very much in favour of fixing this. It's super easy to reproduce on any Apple device - MacOS, iOS, iPadOS, etc. So to have this behave nicely on Mobile, we definitely need to fix this. I don't know the reconciler code well enough, to have a strong opinion on if the solution is good or not.