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

Tab support #4436

Merged
merged 15 commits into from May 6, 2023
Merged

Tab support #4436

merged 15 commits into from May 6, 2023

Conversation

zurfyx
Copy link
Member

@zurfyx zurfyx commented May 1, 2023

Tab

This PR implements Tab (\t) support.

intro2.mov

The camouflaged broken experience

Turns out that this basic feature was never implemented. My hunch is that we thought it would work out of the box on a contenteditable but the reality is very different. Particularly, for Chrome.

Previous behavior (all it takes is to paste a Tab or type one in a code block (that can bubble up to the parent)):

Screen.Recording.2023-04-30.at.8.13.33.pm.mov

It wasn't until very recently, that @abelsj60 took a stab at it (#3770) to fix a pain point with CodeNodes (gated via Node props), but after the internal report (#4399) and the fact that we don't necessarily want to override the default CodeNode, it revealed a major issue.

It is a non-obvious one because of the current indent behavior (everything's an indent) (similar to Quip), that forces you to indent the content wherever you are, preventing you from tabbing the content.

I believe the ideal behavior is somewhere closer to Google Docs or MS Word, where you can have a mix of both (and support for customizability). You can indent when the selection spans across multiple blocks or at the beginning but otherwise you'll tab (this is a simplication, see unit tests).

Chrome

Interestingly, this only applies to Chrome, but the fact that everyone uses Chrome makes it a no-brainer to write a fix for them. As it turns out, Chrome will break your span when it contains a \t. The DOM Mutations will return a valid state but it is far from a Lexical valid state and with what we have now, Lexical will just drop it.

Screen.Recording.2023-04-30.at.6.47.47.pm.mov

You can repro this here. https://codepen.io/zurfyx/pen/bGmrrpX

Failed attempts

First attempt (#4434)

Expand the Rich Text plugin approach to make tabs also work with controlled mode. This way, we can dodge the complicated Mutations and still perform quite optimally. Won't quite work as Chrome will still destroy your span later even if you're not anywhere close to the tab. F

Second attempt (#4435)

Try reconstruct a valid state from the Mutations. This won't quite work for IME because as you soon as you touch selection you kill it. F

The solution that works (this PR)

Turns out that Chrome will not destroy anything but the contents of a span. I kinda realized that via Prosemirror that doesn't use span wrapper and they don't have this issue on their basic example editor. You can combine \t just fine on a p without any special logic.

Unfortunately, Lexical relies heavily on span so we can't just drop them. However, we can isolate it. We can do this via the newly introduced LexicalTabNode, a span that renders just \t.

This also works well with IME as it leaves the rest of the editor untouched!

Then, most of the PR goes around trying to make this Node on various cases.

The fastest approach would have been to have a registerNodeTransform, but it comes at the cost of performance, repeated indexOf and splits, so instead this PR handles every single case manually:

  • Insert text via selection
  • Clipboard
  • Code (which deserves it's own section)

Code

code.mov

So there's two type of tabs UX-wise. The nicely expansible tabs and the hardcoded-width tabs that are the commonly used in code.

For this reason, we need to introduce yet another tab node: CodeTabNode.

This tab will just simply render a space but the size is variable, just like in every editor.

Two particularly interesting points behind the implementation:

  1. We intentionally only render a single space to make sure that selection can jump within one hop.
  2. letter-spacing is the only one that works across browsers and mobile. word-spacing only works properly on Chrome and Firefox and white-space: pre-line behaves badly when tab is at end.

The rest of the PR just makes this work around indent/outdent and line-shifting.

Copy-paste

Most of the failing tests are about copy-paste. I intentionally didn't fix them yet because I don't understand the GDoc fixes in #2969. By doing this we broke a valid copy-paste case, demostrated in https://codepen.io/zurfyx/pen/KKGvvNV

Will need your help @fantactuka / @acywatson

Peers

Editor Notes
GDoc
MS
Draft.js ❌ (crashes)
Prosemirror ? (not sure)
Slate ❌ (crashes)
Quill ? (not sure)

References

Fixes #4429
Fixes #4399
Fixes #4433

TODO

  • Flow types
  • Revise copy-paste tests

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 1, 2023
@vercel
Copy link

vercel bot commented May 1, 2023

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

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 4, 2023 9:32pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 4, 2023 9:32pm

@github-actions
Copy link

github-actions bot commented May 1, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 27.17 KB (+0.71% 🔺) 544 ms (+0.71% 🔺) 211 ms (+22.38% 🔺) 754 ms
packages/lexical-rich-text/dist/LexicalRichText.js 38.09 KB (+0.67% 🔺) 762 ms (+0.67% 🔺) 171 ms (-23.86% 🔽) 933 ms
packages/lexical-plain-text/dist/LexicalPlainText.js 38.06 KB (+0.67% 🔺) 762 ms (+0.67% 🔺) 309 ms (+100.82% 🔺) 1.1 s

@abelsj60
Copy link
Contributor

abelsj60 commented May 1, 2023

@zurfyx I just want to tell you, this is amazing.

  1. I have been over some of this ground and felt insane at the time. I'm delighted to hear I'm not, at least not today.
  2. I did think the best solution to tabs would be a TabNode, however, the merging algorithm killed me. Modifying that was too much for me.
    • I did the best I could with Better tab handling #3770, and while it did the job to my eye, I totally get that tabs are more than meet the eye.
    • You touched on tab spacing. You'd have to check with RawLasagna in Discord, but I think he was translating MS Word tabs into Lexical, then converting that spacing back to Word on export or save.
    • Can tab spacing be "assigned," via your node somehow? I seem to recall this being the one special feature of tabs that could use special handling. Maybe? It's been awhile.

Just a couple thoughts.

Again, really impressed that you dug through this. It's not an easy area, especially as navigation keeps stealing the spotlight from tabs. Confronting this in Lexical has turned me into an unabashed tab defender. lol.

-j

@zurfyx
Copy link
Member Author

zurfyx commented May 1, 2023

@abelsj60 Thank you, appreciate the warm comment! Your PR was great, not only it fixed the immediate issue but also helped me find the root cause. It is part of the process. I also understand that this one does require a big block of time, as it's still not yet ready to merge.

You touched on tab spacing. You'd have to check with RawLasagna in Discord, but I think he was translating MS Word tabs into Lexical, then converting that spacing back to Word on export or save.

cc'ed him on Discord, thanks for the heads up! I believe this PR covers it but otherwise we can discuss further.

Can tab spacing be "assigned," via your node somehow? I seem to recall this being the one special feature of tabs that could use special handling. Maybe? It's been awhile.

Yes, via theming. There's a TODO I need to address since it's currently hardcoded for CodeTabNode but otherwise it should be customizable. Let me know if you recall this special case, I might have missed something.

@abelsj60
Copy link
Contributor

abelsj60 commented May 1, 2023

@zurfyx I took a quick look. I guess spacing is less a special case than a manifestation of the results people expect from a tab character. Their idea is bound to be custom. If it's helpful, I'll toss out my questions from the code:

  • TabNode

    • How would I set a size value (1ch, 2ch, .5rem) for these nodes? I notice you pass the hidden "/t" to supra, then let TextNode take care of createDOM, which I understand. It's all just text.
      • Am I then able to set its spacing by targeting whatever DOM node holds the "/t" via a class name?
      • Is passing a class name via "base" enough? Do you plan to add a new entry to TextNodeThemeClasses?
        • In other words, what's the mechanism to target the TabNode's DOM node? (I may have missed it.)
  • CodeTabNode

    • I see: "// TODO pass through theme"
    • Your idea here is to allow devs to assign one tab value for the main editor and another for CodeNodes?
      • That seems right to me. People are finicky about tab spacing in code blocks/full code editors.
    • The css property that should be used to set tab spacing is letter-spacing (see also above)?
      • Is that right? I never tried that, so I don't know. Or is that just temporary for your development?
      • Similar to above, will you be adding an entry to EditorThemeClasses for CodeTabNode?
  • Finally, a couple edge cases that gave me pause back in the day:

    • What happens if I hit tab in the middle of a line? The TabNode is inserted there? Probs OK.
    • And if I put the cursor before a tab and type, then what? The tab moves? Probs OK, too.

I know you have it all in hand, this is just what pops to my mind.

Love the use of the Override API. Full customization...

-j

@zurfyx
Copy link
Member Author

zurfyx commented May 1, 2023

@abelsj60 - Appreciate the in-depth thinking process!

Am I then able to set its spacing by targeting whatever DOM node holds the "/t" via a class name?

Easier than that, you can just add tab-size to your contenteditable!

Your idea here is to allow devs to assign one tab value for the main editor and another for CodeNodes?

Yes, because they behave differently, the TabNode resizes as you type (unless it's size 1), while CodeTabNode is fixed size. Also, while you would usually type JS with 2 spaces, Java is often 4 so that's a valid reason to allow people customize this.

The css property that should be used to set tab spacing is letter-spacing (see also above)?

Yes! Added some more details on the description above. Let me know the above doesn't cover this question.

Similar to above, will you be adding an entry to EditorThemeClasses for CodeTabNode?

Correct

What happens if I hit tab in the middle of a line? The TabNode is inserted there? Probs OK.

Yes

And if I put the cursor before a tab and type, then what? The tab moves? Probs OK, too.

Adds another tab (i.e. GDoc's behavior)

@abelsj60
Copy link
Contributor

abelsj60 commented May 1, 2023

@abelsj60 - Appreciate the in-depth thinking process!

...

This is great, @zurfyx. I come from the school of "speak now or forever hold your peace," so try to get it all out.

Your enhanced description at the top is helpful. I'd be afraid that letter-spacing applies to all letters, not just tabs, but you know what you're doing, so I've no doubt it'll work great.

This is exciting. Tabs without hand-wringing.

Pretty cool...

-j

@zurfyx
Copy link
Member Author

zurfyx commented May 2, 2023

I'd be afraid that letter-spacing applies to all letters

@abelsj60 - Ah, yes, that's correct. I should've clarified that letter-spacing is just for the CodeTab itself so <span style="letter-spacing: 15px;"> </span> whereas tab-size is for the entire editor <div contenteditable="true" style="tab-size: 2">...</div>

@rferreira98
Copy link

rferreira98 commented May 5, 2023

@zurfyx (RawLasagna here) noticed something strange, when I tab at the end of node after writing (this behaviour is not very consistent though). Is this intended?

zurfyx.tab3.mov

@zurfyx
Copy link
Member Author

zurfyx commented May 5, 2023

@zurfyx (RawLasagna here) noticed something strange, when I tab at the end of node after writing (this behaviour is not very consistent though). Is this intended?

@rferreira98 - Thanks for the feedback! I think so, besides we don't do any special rendering of the tab in this case, I can repro the same behavior in GDocs, MS Word and Apple Notes.

@zurfyx zurfyx merged commit fe940b9 into main May 6, 2023
45 checks passed
@zurfyx zurfyx deleted the tab3 branch May 6, 2023 10:06
This was referenced May 19, 2023
@leolorenzoluis
Copy link

@zurfyx The shift tab doesn't work if the contents are not tab in the beginning. Try in playground to tab and shift tab. It doesn't outdent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
6 participants