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

feat(IText): Draggable text #7802

Merged
merged 103 commits into from Jul 28, 2022
Merged

feat(IText): Draggable text #7802

merged 103 commits into from Jul 28, 2022

Conversation

ShaMan123
Copy link
Member

@ShaMan123 ShaMan123 commented Mar 11, 2022

General

Adds support for native like text dragging! Out of the box!
This PR aims to support text dragging, making fabric's text objects feel/behave like a HTML textarea.
It is an expected interaction from the user's point of view.

closes #7800

Changes

  • drag/drop event logic on canvas/text:
    • firing DnD events with more data for the dev
    • addressed an issue with firing order on drag over targets and the selected target
    • onDragStart and canDrop are public methods for overriding that control drag and drop respectively
    • dragStart logic cycle - imitates spec, invokes target's onDragStart method, if result is true drag starts. Behaves like onSelect.
    • creating a drag image from onDragStart
    • dragEnd calls _onMouseUp synthetically - the browser doesn't do it and fabric needs it to finalize interaction logic
  • cursor/selection rendering on text - in order to render text selection on the drag source while dragging and the cursor on the drop target while dragging over it (same as what the browser does)

Thoughts

I think we ought to change the selectWord method to align with the behavior of html textarea which selects the word and all the trailing whitespace (it's important for drag and drop).

TO DO

ezgif com-gif-maker (11)
ezgif com-gif-maker (12)

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.09 |    76.49 |   86.27 |    82.8 |                                               
 fabric.js |   83.09 |    76.49 |   86.27 |    82.8 | ...,29917,30042,30122-30187,30310,30409,30626 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.08 |    76.45 |   86.27 |   82.79 |                                               
 fabric.js |   83.08 |    76.45 |   86.27 |   82.79 | ...,29917,30042,30122-30187,30310,30409,30626 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.95 |     76.3 |   86.15 |   82.67 |                                               
 fabric.js |   82.95 |     76.3 |   86.15 |   82.67 | ...,29955,30080,30160-30225,30348,30447,30664 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.01 |    76.38 |   86.15 |   82.72 |                                               
 fabric.js |   83.01 |    76.38 |   86.15 |   82.72 | ...,29956,30081,30161-30226,30349,30448,30665 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.86 |    76.32 |   86.04 |   82.57 |                                               
 fabric.js |   82.86 |    76.32 |   86.04 |   82.57 | ...,29987,30112,30192-30257,30380,30479,30696 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.72 |    76.17 |   85.98 |   82.43 |                                               
 fabric.js |   82.72 |    76.17 |   85.98 |   82.43 | ...,30019,30144,30224-30289,30412,30511,30728 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.73 |    76.21 |   85.98 |   82.44 |                                               
 fabric.js |   82.73 |    76.21 |   85.98 |   82.44 | ...,30019,30144,30224-30289,30412,30511,30728 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.73 |     76.2 |   85.98 |   82.44 |                                               
 fabric.js |   82.73 |     76.2 |   85.98 |   82.44 | ...,30020,30145,30225-30290,30413,30512,30729 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.74 |    76.22 |   85.98 |   82.45 |                                               
 fabric.js |   82.74 |    76.22 |   85.98 |   82.45 | ...,30025,30150,30230-30295,30418,30517,30734 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.73 |    76.17 |   85.93 |   82.43 |                                               
 fabric.js |   82.73 |    76.17 |   85.93 |   82.43 | ...,30047,30172,30252-30317,30440,30539,30756 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.74 |    76.12 |   85.93 |   82.44 |                                               
 fabric.js |   82.74 |    76.12 |   85.93 |   82.44 | ...,30054,30179,30259-30324,30447,30546,30763 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.73 |    76.09 |   85.93 |   82.43 |                                               
 fabric.js |   82.73 |    76.09 |   85.93 |   82.43 | ...,30055,30180,30260-30325,30448,30547,30764 
-----------|---------|----------|---------|---------|-----------------------------------------------

@melchiar
Copy link
Member

melchiar commented Jul 6, 2022

Yes, wrong bounding box, then after selecting a different object and then trying to select the first one again you get no bounding box at all though it's still in text editing mode.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   81.95 |    74.85 |      85 |   81.61 |                                               
 fabric.js |   81.95 |    74.85 |      85 |   81.61 | ...,31607,31681,31692-31757,31880,31979,32215 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123
Copy link
Member Author

Tried to reproduce on windows Chrome without success.
Could you post the dataTransfer data so I could have a look?

@melchiar
Copy link
Member

melchiar commented Jul 7, 2022

I've simplified my test and the tiny bounding box was actually an app issue. The issue of the object being stuck in edit mode with no bounding box remains though.

Here's my dataTransfer data:

image

Also, firefox seems render the hover image in the wrong position/size on highdpi displays (my pixel ratio is 1.5)

draggable-text-firefox

@ShaMan123
Copy link
Member Author

could you upload the test into a sandbox?
I don't have firefox...

@ShaMan123
Copy link
Member Author

The positioning issue is strange.
I have 1.25 dpr on chrome and is well positioned

@ShaMan123
Copy link
Member Author

ShaMan123 commented Jul 7, 2022

@melchiar test on http://fabricjs.com/test/misc/itext.html (with the local server)

@melchiar
Copy link
Member

melchiar commented Jul 8, 2022

same issues running on itext.html. Drag an image file into a unselected textbox and it gets stuck in edit mode with no control box. Only way to get it back is by dragging text into it from another box.

The hover image position/size issue only occurs in firefox, chrome is fine.

draggable-text-firefox

border: 'none'
});
fabric.document.body.appendChild(dragImage);
e.dataTransfer.setDragImage(dragImage, offset.x, offset.y);
Copy link
Member Author

Choose a reason for hiding this comment

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

@melchiar try manipulating the offset value
Perhaps firefox accounts for retina scaling?

@ShaMan123
Copy link
Member Author

ShaMan123 commented Jul 8, 2022

Here's my dataTransfer data:

image

This doesn't seem like the data transfer from the drop event, seems like from a drag event.

@ShaMan123
Copy link
Member Author

ShaMan123 commented Jul 8, 2022

can you make sure all drag/drop events are fired in the correct order?
start
enter
over
exit
drop
end

It might be that the mouse up handler isn't called correctly at the drag end operation

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    81.9 |    74.75 |   84.95 |   81.57 |                                               
 fabric.js |    81.9 |    74.75 |   84.95 |   81.57 | ...,31633,31707,31718-31783,31906,32005,32241 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Jul 9, 2022

There are a bunch of things i want to experiment with before merging.
In particular i don't like:

  • the use of enter/exit editing and update textarea to draw the destination cursor for the drop
  • the particular meaning we attribute to dropEffect ( we read it on the event, we never assign it, meaning that someone else has assigned it and there is a contract on the meaning ).
  • _onDragStart using activeObject rather than finding a target from the event

I want to try and test the code and see if i have alternatives for those problems, i don't have direct comment to lines that i can ask to change, is more a generic thing.

But again, all those informations shouldn't be digged in code review, for a large pr and features Adds support for native like text dragging! Out of the box! is not an adequate description.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 10, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   81.91 |    74.78 |   84.95 |   81.58 |                                               
 fabric.js |   81.91 |    74.78 |   84.95 |   81.58 | ...,31633,31707,31718-31783,31906,32005,32241 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123
Copy link
Member Author

ShaMan123 commented Jul 10, 2022

  1. Sorry about the PR description. UPDATED
  2. The effectAllowed is set on dragStart - the system populates dropEffect, the docs saw you can change it but experimenting with it showed differently if I recall correctly, see MDN dropEffect effectAllowed. There is a contract, I aimed to follow MDN and web standard.
  3. _onDragStart using activeObject rather than finding a target from the event - GOOD POINT - however it must be the active object or else the text object won't be in editing mode

@@ -299,6 +299,17 @@
*/
onSelect: function() {
// implemented by sub-classes, as needed.
}
},

Copy link
Member Author

@ShaMan123 ShaMan123 Jul 10, 2022

Choose a reason for hiding this comment

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

I guess we should/could add onDragStart: () => false here, as onDragStart and canDrop are public methods for overriding

Only for better readability

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.03 |    74.95 |   84.77 |   81.69 |                                               
 fabric.js |   82.03 |    74.95 |   84.77 |   81.69 | ...,31610,31684,31695-31760,31883,31982,32218 
-----------|---------|----------|---------|---------|-----------------------------------------------

@@ -512,6 +506,7 @@
e.dataTransfer.effectAllowed = 'copyMove';
this.setDragImage(e, data);
}
this.abortCursorAnimation();
Copy link
Member Author

Choose a reason for hiding this comment

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

for future follow up
this line might be causing selection to be cleared on drag start

@asturur asturur deleted the draggable-text branch September 11, 2022 23:03
@ShaMan123 ShaMan123 restored the draggable-text branch December 22, 2022 09:14
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
@ShaMan123 ShaMan123 deleted the draggable-text branch February 5, 2023 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants