Skip to content

fix: chrome should not wrap mid-line #1423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Feb 11, 2025
Merged

fix: chrome should not wrap mid-line #1423

merged 10 commits into from
Feb 11, 2025

Conversation

nperez0111
Copy link
Contributor

Partially reverts commit "e011cf4863e06b6eb07985f379f7750d76b091ee"

This addresses a bug introduced by #1422 which caused a regression where chrome in some instances would break text wrapping because of the pseudo element's presence in the document.

The pseduo element is now removed, moving back to the original border color approach with an additional fix specifically applied to firefox that will cause the element not to wrap when whitespace is following text

partially reverts commit "e011cf4863e06b6eb07985f379f7750d76b091ee"
Copy link

vercel bot commented Feb 10, 2025

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

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Feb 11, 2025 9:15am
blocknote-website ✅ Ready (Inspect) Visit Preview Feb 11, 2025 9:15am

partially reverts commit "e011cf4863e06b6eb07985f379f7750d76b091ee"
@nperez0111
Copy link
Contributor Author

I went back on this approach again. I found that the pseudo element can use a border so that it does not take space within the content, but still have a width to allow mouse events to work on the element. Seems to be working well in firefox & chrome in the various scenarios we have identified.

Will want to look into this in the morning though....

@@ -77,9 +77,7 @@ export const createCollaborationExtensions = (collaboration: {
labelElement.setAttribute("style", `background-color: ${user.color}`);
labelElement.insertBefore(document.createTextNode(user.name), null);

cursorElement.insertBefore(document.createTextNode("\u2060"), null); // Non-breaking space
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now let CSS insert these  

@nperez0111
Copy link
Contributor Author

Screen.Recording.2025-02-11.at.09.20.27.mp4

Left Firefox, Middle Chrome, Right Safari

I think I've got all of the edge cases figured out for the major browsers here. They each needed their own hacks. I went with the most chrome native implementation (using the pseudo element with a border) and then added the hacks for firefox & safari behavior on top with comments on why.

Hopefully this will resolve it

@nperez0111
Copy link
Contributor Author

Works on iOS too
image

@nperez0111 nperez0111 requested a review from YousefED February 11, 2025 08:35
Copy link
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome. Worth the effort 🙌

  • There's a little bit of whitespace now again right?
  • shall we comment the scenarios as to why this was complicated? (I hope we're not touching this code for a while, but maybe good to explain that changes need to be tested on all browsers and both the long lines + whitespace end of line scenarios?)

/* Add nbsp; to each side of the caret */
.collaboration-cursor__caret::after,
.collaboration-cursor__caret::before {
content: "⁠";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one is here? Github doesn't show it
nbsp = https://www.compart.com/en/unicode/U+00A0
word joiner = https://www.compart.com/en/unicode/U+2060

the previous code had "word joiner"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

word joiner is there. I'll update the comment with the unicode to make it clear

@nperez0111
Copy link
Contributor Author

There's a little bit of whitespace now again right?

There is but the negative margin slurps it so it becomes effectively 0px wide (though Safari seems to not agree with that unless position: relative is set).

shall we comment the scenarios as to why this was complicated? (I hope we're not touching this code for a while, but maybe good to explain that changes need to be tested on all browsers and both the long lines + whitespace end of line scenarios?)

Yea, I'll add a here be dragons comment

@nperez0111 nperez0111 merged commit 1ade05c into main Feb 11, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants