Skip to content
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

new Web rich text composer #2557

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

mary-ext
Copy link
Contributor

@mary-ext mary-ext commented Jan 17, 2024

this is an experimental pull request that introduces a new RichText input for web, with the goal of aligning with what's currently on native. This has the benefit of side-stepping any headaches that currently comes with TipTap and other WYSIWYG editors like Lexical, plus, makes a decent dent on reducing bundle size.

this RichText input works pretty similarly to how it is on native, it does this by making the input's text color transparent (and fixing up any adjacent styling like caret, placeholder and selection text), and overlaying our own rendered text below it (such that your cursor will always land on the input)

  • Basic text input functionality
  • Text highlighting
  • Autocomplete suggestions
    • This is entirely new code, should be able to align it with native, but native doesn't handle selection changes
  • Emoji picker (broke range positioning, should be able to make it work again)
  • Paste to add images
  • Keyboard shortcuts (Ctrl/Meta+Enter)

I know that Paul has said that there might be plans to move away from being entirely driven by syntax, but this was just a fun challenge I came up on a whim, feel free to close this PR if necessary :)

TODO

  • Don't pull in ReactDOM for portals, I didn't realize we already have a portal solution going
  • Figure out the performance issue, see below comments
  • Check if ligatures could pose a problem, is turning them off an option?
  • Test non-Latin usage

@mary-ext
Copy link
Contributor Author

before/after
image

@mary-ext mary-ext marked this pull request as ready for review January 18, 2024 11:18
@mary-ext
Copy link
Contributor Author

mary-ext commented Jan 18, 2024

the performance is somewhat awful, at least on Firefox. I'm thinking there's two things going on here:

  • RichText parsing is expensive (detecting facets via regex + retrieving UTF-8 indices for it)
    • we could skip indices calculation if we parse it into an intermediary facet, before resolving it into an actual facet when submitting the post
    • intermediary facet should make it possible to introduce editor-specific functionalities like escaping which doesn't quite make sense as an actual facet
    • plugging this in as reference
  • React diffing is expensive, we can probably test how it goes with innerHTML
  • Post language detection is kicking in too often (will post a PR that puts in an actual throttle mechanism)

@mary-ext
Copy link
Contributor Author

mary-ext commented Jan 18, 2024

patch for skipping React diffing by constructing HTML strings, it seems to be making some difference but I can't really tell how well it's actually working, need a proper bench setup.

diff --git a/src/view/com/composer/text-input/TextInput.web.tsx b/src/view/com/composer/text-input/TextInput.web.tsx
index c164938..b7b24bc 100644
--- a/src/view/com/composer/text-input/TextInput.web.tsx
+++ b/src/view/com/composer/text-input/TextInput.web.tsx
@@ -137,30 +137,24 @@ export const TextInput = React.forwardRef<TextInputRef, TextInputProps>(
     }, [richtext, cursor, overlayRef, setSuggestion])
 
     const textOverlay = React.useMemo(() => {
+      let html = ''
+
+      for (const segment of richtext.segments()) {
+        const isLink = segment.facet
+          ? !AppBskyRichtextFacet.isTag(segment.facet.features[0])
+          : false
+
+        const klass =
+          `rt-segment ` + (!isLink ? `rt-segment-text` : `rt-segment-link`)
+        const text = escape(segment.text, false) || '&#x200B;'
+
+        html += `<span class="${klass}">${text}</span>`
+      }
+
       return (
-        <div className="rt-overlay-inner">
-          {Array.from(richtext.segments(), (segment, index) => {
-            const isLink = segment.facet
-              ? !AppBskyRichtextFacet.isTag(segment.facet.features[0])
-              : false
-
-            return (
-              <span
-                key={index}
-                className={
-                  `rt-segment ` +
-                  (!isLink ? `rt-segment-text` : `rt-segment-link`)
-                }>
-                {
-                  // We need React to commit a text node to DOM so we can select
-                  // it for `getCursorPosition` above, without it, we can't open
-                  // the emoji picker on an empty input.
-                  segment.text || '\u200b'
-                }
-              </span>
-            )
-          })}
-        </div>
+        <div
+          dangerouslySetInnerHTML={{__html: html}}
+          className="rt-overlay-inner"></div>
       )
     }, [richtext])
 
@@ -352,3 +346,19 @@ const findNodePosition = (
 
   return
 }
+
+const escape = (str: string, attr: boolean) => {
+  let escaped = ''
+  let last = 0
+
+  for (let idx = 0, len = str.length; idx < len; idx++) {
+    const char = str.charCodeAt(idx)
+
+    if (char === 38 || (attr ? char === 34 : char === 60)) {
+      escaped += str.substring(last, idx) + ('&#' + char + ';')
+      last = idx + 1
+    }
+  }
+
+  return escaped + str.substring(last)
+}

@pfrazee
Copy link
Collaborator

pfrazee commented Jan 19, 2024

Intriguing. I'll definitely want to play with this at some point.

@pfrazee pfrazee added the x:discussing We've seen the request and we're talking about it! label Jan 24, 2024
@mary-ext mary-ext force-pushed the feat/new-richtext-composer branch from 234ec79 to e2a9ae3 Compare January 30, 2024 02:20
@ocodista
Copy link

ocodista commented Sep 2, 2024

Could you add some screenshots or videos of what it would look like?

@mary-ext
Copy link
Contributor Author

mary-ext commented Sep 2, 2024

It's intended to look exactly the same as before, so there is no screenshot or video.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:discussing We've seen the request and we're talking about it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants