search

facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
17.5k stars 1.45k forks source link

feat: move images by dragging on the editor #2027

Closed LuciNyan closed 1 year ago

LuciNyan commented 2 years ago

Description

Related Issues 📦

1852

https://user-images.githubusercontent.com/22126563/165798807-8b299a54-3a54-4bd2-adda-81ed7ef422d8.mov

vercel[bot] commented 2 years ago

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview May 17, 2022 at 4:44PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview May 17, 2022 at 4:44PM (UTC)
LuciNyan commented 2 years ago

Could you give me a few pointers with flow. I've come across some flow hints that are confusing me, for example:

Cannot call '$isImageNode' with 'node' bound to 'node' because 'LexicalNode' [1] is incompatible with 'LexicalNode' [2]. [incompatible-call]

But the two LexicalNode are actually the same thing...

acywatson commented 2 years ago

Could you give me a few pointers with flow. I've come across some flow hints that are confusing me, for example:

Cannot call '$isImageNode' with 'node' bound to 'node' because 'LexicalNode' [1] is incompatible with 'LexicalNode' [2]. [incompatible-call]

But the two LexicalNode are actually the same thing...

IIRC this usually means you're importing something from the wrong place - usually a relative import that should be absolute.

LuciNyan commented 2 years ago

Thanks for the reminder!

Could you give me a few pointers with flow. I've come across some flow hints that are confusing me, for example: Cannot call '$isImageNode' with 'node' bound to 'node' because 'LexicalNode' [1] is incompatible with 'LexicalNode' [2]. [incompatible-call] But the two LexicalNode are actually the same thing...

IIRC this usually means you're importing something from the wrong place - usually a relative import that should be absolute.

fantactuka commented 2 years ago

Could you add a video showing how it works?

acywatson commented 2 years ago

Thanks for the reminder!

Could you give me a few pointers with flow. I've come across some flow hints that are confusing me, for example: Cannot call '$isImageNode' with 'node' bound to 'node' because 'LexicalNode' [1] is incompatible with 'LexicalNode' [2]. [incompatible-call] But the two LexicalNode are actually the same thing...

IIRC this usually means you're importing something from the wrong place - usually a relative import that should be absolute.

No worries, thanks for this PR!

LuciNyan commented 2 years ago

Could you add a video showing how it works?

Ok! I have added it.

yuzhouu commented 2 years ago

Hi, how about use the original image as drag preview?

LuciNyan commented 2 years ago

Hi, how about use the original image as drag preview?

When the size of the image is large, a large preview image appears when dragging. To the user, this looks like an unnecessary visual distraction.

LuciNyan commented 2 years ago

Hello @acywatson.

I have tested drag and drop on windows and mac with different browsers (including firefox) and everything works fine, but when I run my e2e tests with playwright, some of the tests with firefox don't work, I have tried a lot but still can't solve the problem. For example, I can't get event.rangeParent and event.rangeOffset correctly when testing. Could you tell me what I should do?

Thanks!

acywatson commented 2 years ago

Hello @acywatson.

I have tested drag and drop on windows and mac with different browsers (including firefox) and everything works fine, but when I run my e2e tests with playwright, some of the tests with firefox don't work, I have tried a lot but still can't solve the problem. For example, I can't get event.rangeParent and event.rangeOffset correctly when testing. Could you tell me what I should do?

Thanks!

@LuciNyan So sorry - I missed this comment. Let me take a look a this and see what's going on.

LuciNyan commented 2 years ago

Hello @acywatson. I have tested drag and drop on windows and mac with different browsers (including firefox) and everything works fine, but when I run my e2e tests with playwright, some of the tests with firefox don't work, I have tried a lot but still can't solve the problem. For example, I can't get event.rangeParent and event.rangeOffset correctly when testing. Could you tell me what I should do? Thanks!

@LuciNyan So sorry - I missed this comment. Let me take a look a this and see what's going on.

No worries! and I have resolved the conflict and resubmitted the code. There are a few minor changes.

Thanks for you, and I have to say it's great to be able to use Typescript here. LOL.

LuciNyan commented 2 years ago

Hello @acywatson. I have tested drag and drop on windows and mac with different browsers (including firefox) and everything works fine, but when I run my e2e tests with playwright, some of the tests with firefox don't work, I have tried a lot but still can't solve the problem. For example, I can't get event.rangeParent and event.rangeOffset correctly when testing. Could you tell me what I should do? Thanks!

@LuciNyan So sorry - I missed this comment. Let me take a look a this and see what's going on.

I was wondering if I could not test against firefox for the above reason. I've spent a lot of time trying to solve the problem that firefox behaves differently in practice than when running tests in playwright, but there seems to be no effective solution so far. And there doesn't seem to be a better way to get the range on drag and drop.

Could you give me some advice?

acywatson commented 2 years ago

Hello @acywatson. I have tested drag and drop on windows and mac with different browsers (including firefox) and everything works fine, but when I run my e2e tests with playwright, some of the tests with firefox don't work, I have tried a lot but still can't solve the problem. For example, I can't get event.rangeParent and event.rangeOffset correctly when testing. Could you tell me what I should do? Thanks!

@LuciNyan So sorry - I missed this comment. Let me take a look a this and see what's going on.

I was wondering if I could not test against firefox for the above reason. I've spent a lot of time trying to solve the problem that firefox behaves differently in practice than when running tests in playwright, but there seems to be no effective solution so far. And there doesn't seem to be a better way to get the range on drag and drop.

Could you give me some advice?

It looks like the failure is actually happening only on Firefox for Windows, which is interesting. I don't have immediate access to a Windows machine to try to repro this, but I think @fantactuka might?

Interesting that you tested it on both windows and mac - I wonder if there's a version difference that might explain the different behaviors? I couldn't find anything in the release logs.

Doing some reading on the Drag Event API, it looks like there are some particularly divergent browser behaviors here. One thing. I found interesting is that rangeParent and rangeOffset don't seem to be part of the public API (which is probably why you had to delcare them as global types). One thing I might suggest is trying to use document.caretPositionFromPoint as your fallback when caretRangeFromPoint is not available (instead of event.rangeParent and event.rangeOffset).

https://developer.mozilla.org/en-US/docs/Web/API/Document/caretPositionFromPoint

Can you give that a try and see what happens? If you can provide more details on what's happening in playwright (are rangeParent and rangeOffset undefined? Set to zero?) Perhaps we can file a bug with Playwright and see if we can get more information there.

Thanks for your effort and persistence here!

LuciNyan commented 2 years ago

Hello @acywatson.

I've tried document.caretPositionFromPoint and it works perfectly fine in practice, but in playwright it gets the same wrong value as event.rangeParent and event.rangeOffset. For example, if there is a line with 1234 and I drag the image to the middle of 23, the correct rangeOffset should be 2, but in playwright we get 0. I would like to add that I tested the drag and drop on my windows virtual machine, I did not test it on a real windows machine. It looks like it would be nice to have a windows user to help look at this test.

Thanks for your advice!

fantactuka commented 2 years ago

Hello @acywatson.

I've tried document.caretPositionFromPoint and it works perfectly fine in practice, but in playwright it gets the same wrong value as event.rangeParent and event.rangeOffset. For example, if there is a line with 1234 and I drag the image to the middle of 23, the correct rangeOffset should be 2, but in playwright we get 0. I would like to add that I tested the drag and drop on my windows virtual machine, I did not test it on a real windows machine. It looks like it would be nice to have a windows user to help look at this test.

Thanks for your advice!

Did some testing on windows machine and came to the same results as you did. After some offline discussion with @acywatson we thought it could be outdated playwright-core fork that used firefox 93 vs 99-100 on desktop. But updating to the latest version didn't change anything.

Tried to do dragging in steps (bring image into paragraph end, and then run second drag into the middle); splitting dragging into multiple steps (moving by delta between two points), and each approach had same issue with firefox

LuciNyan commented 2 years ago

Hello @acywatson. I've tried document.caretPositionFromPoint and it works perfectly fine in practice, but in playwright it gets the same wrong value as event.rangeParent and event.rangeOffset. For example, if there is a line with 1234 and I drag the image to the middle of 23, the correct rangeOffset should be 2, but in playwright we get 0. I would like to add that I tested the drag and drop on my windows virtual machine, I did not test it on a real windows machine. It looks like it would be nice to have a windows user to help look at this test. Thanks for your advice!

Did some testing on windows machine and came to the same results as you did. After some offline discussion with @acywatson we thought it could be outdated playwright-core fork that used firefox 93 vs 99-100 on desktop. But updating to the latest version didn't change anything.

Tried to do dragging in steps (bring image into paragraph end, and then run second drag into the middle); splitting dragging into multiple steps (moving by delta between two points), and each approach had same issue with firefox

Yes, I've tried all of these too, including waiting a bit at the end of a drag before dropping it, distributing the movement, etc., and all have the same problem. I'd like to know what we should do next on this feature.

Thank you very much for your help!

fantactuka commented 2 years ago

Tried to debug it on windows a bit more, but no luck with getting proper rangeOffset. Let me report it as a bug into playwright, and we can have e2e fork for now

LuciNyan commented 2 years ago

Hello @acywatson.

There is still a requested change that needs your approval and I would like to know if there is anything else I need to change.

LuciNyan commented 1 year ago

Hello @acywatson.

I noticed that the PR had not been closed, and I wondered if it had forgotten to be merged.

acywatson commented 1 year ago

Hello @acywatson.

I noticed that the PR had not been closed, and I wondered if it had forgotten to be merged.

No, we just leave PRs open after approval for a bit sometimes to give other team members a chance to look at them. In this case, we were also discussing the long-term implications of the FF issue a bit internally. I think this is safe to merge now. Thanks again!