Edit Link modal fix

[HarrySIV]

This PR fixes #4064 by searching the selected nodes for a node whose parent is not a auto/link node and sets isLink accordingly.
Edit: I forgot to mention the css needs updating with this fix as the text toolbar still makes space for the link toolbar, but I couldn't figure out how the styling was being added.

[vercel]

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	Jan 29, 2024 6:16pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 29, 2024 6:16pm

[HarrySIV]

I'm not sure how to solve the failed test, essentially it looks like assertSelection is failing, but I'm not exactly sure what the Array is saying.

` - Expected - 2
+ Received + 1

  Array [
    0,
-   0,
-   0,
+   1,
  ]`

I've tested it and the only change I see is when you click the hyperlink icon the focus offset is 0 instead of 6, but the selection is essentially the same otherwise. I'm not sure how the change alters the selection since all it does is add additional checks for determining when isLink should be true.

[zurfyx]

Thank you! I wonder if it would be easier and more performant to look at the selection anchor and focus instead to determine whether the link is in between boundaries.

[zurfyx]

Be careful when using .getParent directly, the immediate parent doesn't necessarily have to be the Link. For example, when using CharacterLimit ?isCharLimit=true

[HarrySIV]

Thank you, I will look into this.

[HarrySIV]

I thought about that, but I figured if you select a lone text node in between two link nodes the modal would still pop up.
I also considered adding an out-of-bounds state that prevented further rendering of the modal if you selected a lone text node and were still selecting, but that turned out to be convoluted and I wasn't sure it would work when using a "select all" command.
Let me know if you still think that's the best course of action.

[zurfyx]

@HarrySIV You're right, what about, we loop all the nodes, find $findMatchingParent(LinkNode) for each of them. If any returns null, we close the loop (there's no link), if there's a link we continue matching as long as they all match to the same link. Exception is LineBreakNode which gets excluded. For the majority of the cases, the cost will be O(d) where d is the depth of the tree, when a link is matched it will be O(n * d)

[HarrySIV]

@zurfyx I'll try that

[HarrySIV]

Oh, @zurfyx , there was the problem that if you select all, you select the top paragraph node which doesn't have a link node ancestor, which is why I made the search for any node that wasn't the parent of a link node, or have a link node as a parent. I'll try out your suggestion and see what I can do to catch the paragraph node.

[zurfyx]

#4064

Good point, I think that what you're referring to is a confusing behavior from the browser. The selection is moved to the previous ElementNode even when it's selected at the very end. What do you think of our own custom getNodes() that looks at leaf nodes only? Otherwise a custom DFS implementation from anchor to focus but seems like an overkill for this case. I can give it another thought tomorrow, at first glance, it seems like we lack the tooling for a use case that shouldn't be too complicated to address on the product side...

[HarrySIV]

I think I fixed the issue.
Basically updateToolbar searches for the first "badNode" it can find, either a non-LinkNode that isn't a LineBreakNode or another LinkNode that isn't the focus node from selection.
For some reason, "select all" only selects the top ParagraphNode or ListItemNode when there are multiple lines, which would select a non-LinkNode or another LinkNode anyway, so the modal doesn't open, which is fine. That could change if the way selection works changes though.
I will say, something that currently bothers me is if you have a space after the link and double click it, the modal will close because it selects the text node. I'm not exactly sure how I would write that out if the proceeding TextNode contains other text, as you'd have to check that the first character is an empty space.

[HarrySIV]

Would it make sense to move the selection and focusNode declarations out of updateToolbar, so that if the focus is not on a LinkNode, we can return out of the useEffect hook so as not to go through the .find method every time selection changes?

[zurfyx]

Would it make sense to move the selection and focusNode declarations out of updateToolbar, so that if the focus is not on a LinkNode, we can return out of the useEffect hook so as not to go through the .find method every time selection changes?

The React re-renderering involved in useEffect might make it slower and harder to synchronize with the rest of the logic.

I still think that this complex problem might help from some utility function to optimize for the best performance without making the product code complicated but happy to merge this PR as is right now. Thank you for working on this!

[HarrySIV]

That's certainly something I could take a crack at, like the custom getNodes() function you were mentioning? Where would it live, in selection, utils, link?

[agriffis]

@HarrySIV I'm looking at the code in this PR, and I'm struggling to understand this condition:

          if (
            !linkNode?.is(focusLinkNode) &&
            !autoLinkNode?.is(focusAutoLinkNode) &&
            !linkNode &&
            !autoLinkNode &&
            !$isLineBreakNode(node)
          ) {
            return node;
          }

First it tests !linkNode?.is(focusLinkNode) and then it tests !linkNode. It seems like the earlier condition can't possibly match if linkNode is falsy?

It seems like this wants to be either

          if (
            !linkNode?.is(focusLinkNode) &&
            !autoLinkNode?.is(focusAutoLinkNode) &&
            !$isLineBreakNode(node)
          ) {
            return node;
          }

or

          if (
            !linkNode &&
            !autoLinkNode &&
            !$isLineBreakNode(node)
          ) {
            return node;
          }

but it's also possibly I'm misunderstanding entirely 🤷‍♂️

[HarrySIV]

Hi @agriffis ,
That looks to be an error on my part, the first replacement you suggested should work.
Cheers!