feat(IText): Swap flashing Rate of Editing Cursor for better feedback #9823
Conversation
This commit adjusts the initial flashing speed of the cursor when editing text. At the moment, when you click on text being edited, the cursor will only begin flashing after its delay has occurred. By swaping the "toValues" around, this makes the cursor begin to flash more quickly provided better feedback to the user.
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Funny timing |
|
I do not think there is a way to test this properly apart manual verification. |
|
Deleted |
|
I didn't run the code yet, but basically is going to take longer to fade and quicker to get back to full opacity. |
asturur
changed the title
|
I have been staring at the cursor long enough to say this could make sense. There is an additional forced delay, called cursor delay that is imposed when the cursor is moved between places and is used to do not have the cursor flashing until is steady. It make sense that the first animation is the one that fades out since if we want a dealy we can just add it explicity, but what is changing here is also that we swap also the speed of fading. In my opinion we should also swap the duration of the animation. |
here i made a picture to better explain the 3 changes this code brings. the 3 changes are:
Can you please try to explain which one ( or more than one ) from the 3 is bringing the improved experience in your opinion? |
|
I've just noticed that cursorDelay is only being applied to a keyboard event. If I set cursorDelay to 2000 and press a key, there is a 2 second delay before fadeout. If I move the cursor with the mouse, there's no delay from a mouse up event. Surely it should be the same. Always starting at 1, then slight delay before fading out? As it is right now, on focus, there is a long delay before the cursor begins to fade out. What I think should happen is this:
I'll have a look and see why only keyboard events honor cursorDelay. In essence, there's too much delay before the cursor begins to fade. Swapping the toValues seemed to help but maybe only for mouse events? |
|
i have noticed the same about the delay. But to answer my question your point is that you just want to remove the extra initial 600ms of wait before the cursor start to fade after the alread 1000ms of delay we apply. |
src/shapes/IText/ITextBehavior.ts
Outdated
| @@ -160,7 +160,7 @@ export abstract class ITextBehavior< | |||
| private _onTickComplete() { | |||
| this._currentTickCompleteState?.abort(); | |||
| this._currentTickCompleteState = this._animateCursor({ | |||
| toValue: 0, | |||
| toValue: 1, | |||
| duration: this.cursorDuration / 2, | |||
| delay: 100, | |||
There was a problem hiding this comment.
because of the above comment here eventually we need to remove delay completely and make the duration as the full cursorDuration.
|
@milonic test the current code, if is what you expected we can merge it |
|
Yes, that seems to be working fine. However, I'm confused about the ternary inside initDelayedCursor() that sets the delay to 1 if the restart flag is set, this renders cursorDelay useless if the last event was onMouseUp. My question is, what's the point of this? My experience is that the cursor blinks the same regardless of keyboard or mouse event. Would it be better to
|
|
The value of cursorBlink rate it is what it is and can bet tweaked, there is no reason to change the default value. |
|
if you look at how it works today: Before you get the first blink you get: after you will get only the custom delay or the 100 ms. |
|
Well ok apparently we broke some behaviour |
|
I'm intrigued. What's happening? |
|
Nothing really broke but the tests we have rely on some sort of strict timing and now are failing. |
|
I completed the changes to tests i want to rebase this PR on: I m sorry both of your prs hit some small hiccup but they are both good we will do them as soon as normally possible |
src/shapes/IText/ITextBehavior.ts
Outdated
| delay, | ||
| toValue: 0, | ||
| duration: this.cursorDuration / 2, | ||
| delay: delay || 100, |
There was a problem hiding this comment.
This means that of I pass 0 to the delay it will be 100.
So use ?? instead of better off make it a default value in the method signature.
There was a problem hiding this comment.
yes delay 0 is not allowed on purpose. The cursor has to stay opaque for 100ms at least as it was before non configurable. Is the flat part of the original curve between the swaps, you can only make longer the first one as it is today
There was a problem hiding this comment.
In that case I would change the param from delay to something else and handle the delay value internally.
There was a problem hiding this comment.
i think it should me delay: Math.max(delay, 100) so there is no confusion anymore.
| delay, | ||
| toValue: 0, | ||
| duration: this.cursorDuration / 2, | ||
| delay: delay || 100, | ||
| onComplete: this._onTickComplete, |
There was a problem hiding this comment.
I feel strongly against recursive/looping methods calling each other.
IMO a recursive/looping method should call ONLY itself.
Spreading recursion across multiple methods is confusing and is prone for inifinte loops.
There was a problem hiding this comment.
Therefore, I suggest to make it a single function accepting a boolean flagging the state
There was a problem hiding this comment.
Just open an issue with label enanchement, the first one that can do it will do it if there is a real improvement of some kind, i can't see one that make me jump at it.
If you look it from a different point of view tick and tickComplete are 2 wrappers around animateCursor, and is just animateCursor calling itself with configuration A or B.
The issue with not having 2 methods is that you have to use that boolean to determine the 3 conditions every time you call the single method and you don't have a single place where to determine the first delay ( delay || 100 ) so it will be harder to track which is the start of the animation.
Today that is done implicitly by calling _onTick bound without parameters by onComplete
There was a problem hiding this comment.
I have other questions on this code more than the double methods:
- why do we need to track the 2 animations with 2 separate states ( they are running only in sequence )
- why onTickComplete does a pre abort check while onTick doesnt
but also those are out of scope for the animation timing tweak
|
Ok as i hoped generic unit tests are now passing, and the only failing one are the snapshot of the animation itself where the train of 1 is disappeared |
|
@milonic do you know how to update the snapshot tests in jest? |
|
Never used jest, and as I expected and as always it doesn't work as I think it should:
No tests found, exiting with code 1 Run me through what I need to do |
|
So you should have cloned the repo somewhere and you should also have done npm install already. 'npm run test:jest' should give you a test run and 'npm run test:jest -- -u' should give you an update of the failing snapshots. You should just be in the main fabricJS package folder where package.json is, just to be clear. |
|
So, here's what I'm doing: cd /home/andy; mkdir git2; cd git2; npm install fabric@beta; cd node_modules/fabric; npm install chalk; npm run build; npm run test:jest -- -u; ls -l; As you can see package.json is definitely there, what am I doing wrong? Although my development environment is Windows, the above is done on a Linux server |
|
i think you are doing it wrong. cd /home/andy
git clone git@github.com:milonic/fabric.js.git
// this will create your fabric.js folder of your fork
cd fabric.js
npm install
// create a branch, commit, try, push etc etc.
// from your local branch then you can run
npm run test:jest |
|
in the published package, the one you install with npm fabric there isn't the jest config inside and other things are missing too. That is meant for installing/rebuilding but not for developing over |
|
NOW it makes sense..... my apologies |
|
So, I can see it fail with "IText cursor animation snapshot › initDelayedCursor true - with NO delay" What are the tests looking at, what are we comparing them with? |
those tests are snapshotting the cursor opacity over time. update the snapshot so we can review it |
|
Is this something you can do? I've been working on Github for several hours now and it's just getting worse for me. I literally have no idea what I'm doing with it and there are now files all over the place and commits are showing that I did not want to be uploaded etc etc. I'm running out of patience, so sorry! Here's something I was trying to post: https://jsfiddle.net/milonic/57e8au32/27/ - it helps fix issues with double and triple click on text and also helps fix a text drag drop issue. You are more than welcome to the code but I'm unable to get Github to do anything correctly and I'm just making a mess of things. I may come back to this when I'm feeling a little better but at the moment I'm pretty much done. |
|
since this is busted now i reopened it here #9848 |
|
your commits are still there |
This commit adjusts the initial flashing speed of the cursor when editing text.
At the moment, when you click on text being edited, the cursor will only begin flashing after its delay has occurred. By swaping the "toValues" around, this makes the cursor begin to flash more quickly provided better feedback to the user.