-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Writing Flow: Improve emulated caret positioning #5808
Conversation
// offset is too close to the edge. It's unclear how to precisely calculate | ||
// this threshold; it may be the padded area of some combination of line | ||
// height, caret height, and font size. The buffer offset is effectively | ||
// equivalent to a point at half the height of a line of text. | ||
const buffer = rect.height / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I should have better commented this.
Unrelated to the issue this PR specifically fixes, I find this behaviour a bit odd compared to what we currently do for the areas in between paragraphs. At the moment, when you click in between blocks, a new paragraph is created, rather than placing the caret at the closest possible position. It seems to me that clicking under the last block/paragraph should do the same? Why do something different? |
Thank you for working on improving the writing flow. It really matters. This PR definitely fixes an issue where long paragraphs wouldn't place the careat at the bottom edge of the container: However in looking at this, I feel like we may be overthinking it. When you click at the bottom, below the text, should we really set the horizontal position of the caret as well as the vertical? This is not what traditional text editors do. Here's textedit: As you can see, any click below the paragraph sets the caret at the end of the text. In my head right now this feels like it makes for a better writing flow. |
@jasmussen The original behavior was adapted from Google Docs. I assumed other word processors behaved the same. I'm seeing the more common behavior is as you show in your GIF. I also think this helps workflows where a user may want to add a new paragraph at the end of a post, as otherwise they need to manually move caret to the end of the last paragraph. And, of course, it makes the logic simpler 😄 I'll update to this behavior shortly. |
Noting that as you move the caret upwards through multiple paragraphs, the behavior we have in |
e61b3e7 updates the behavior to set focus to the end of the last tabbable field. |
It was originally implemented to emulate other text editors to make it more intuitive to just start writing. It was assumed that to this end, clicking below is not necessarily an intent to add content, just to focus within. Edit: There is some inconsistency in that it adds content if the default appender is present. I think this is nice in combination with the behavior to delete the "provisional" block (#5539) when starting a new post, but for something like when the last block is an image, I'm not entirely sure we should be adding a new block vs. just selecting the image block when clicking below. |
This is feeling super duper solid to me, predictable, natural and right. Very very cool work.
As you pointed out, this is the same behavior as Google Docs. As you seem to imply above, it seems the big difference between Docs and other text editors, is that clicking below the last block is virtually the same as clicking inside the last block, whereas in other text editor clicking inside the last block sets the caret where you click, but clicking below seems to assume you want to get to the end. The latter, imo, feels much more right for us.
I agree that the current behavior feels right (actually, it feels super good), even if it may not have been intended, and it is definitely in line with the original purpose of the appender: _a click at the bottom means you want to continue writing or adding content. Selecting a block should be an explicit action. Stellar work, this feels good to go. |
/** | ||
* Module Constants | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remains form previous work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it intentionally, only because it's meant as a section marker, not a JSDoc for the variables being assigned. Definitely not related to the pull request, but I thought I could sneak it past. Apparently not without being noticed 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh!
Isn't it common to do this then?
/*
* Module Constants
*/
const { UP, DOWN, LEFT, RIGHT } = keycodes;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which we could also use for the "dependencies" comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, yes (though in the specific example, "Module constants" is prone to laziness and ought to at least be accompanied by variable-specific JSDoc).
I was going to make a big argument about how I disagree with the double-vs-single asterisk, but I honestly don't care too much†. There is quite a bit of existing code which would need to be refactored.
Where I might disagree and champion for discussion is some implication from the standards that multi-line comments where each line is prefixed with //
are not valid for inline code. Aside from being far more verbose, I find it hard to disambiguate between a comment and a JSDoc, where only the latter associates a relationship to the immediate following line (not lines, as in a comment describing a sequence of logic), tying back to good code commenting habits of commenting the "what" (JSDoc) from the "why"/"how" (logic flow comments).
† ...Aside from my own personal-workflow incompatibility for me in that Docblock completion via DocBlockr extension only automates creation of intermediate-line asterisks for the double asterisk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I don't care that much either. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was just wondering.
Related: #5541
This pull request seeks to improve the behavior introduced in #5541. For paragraphs longer than few lines, clicking below the editor will redirect to the wrong offset within the block, usually approximately half-way through the content of the block. This is due to a buffer behavior of the
placeCaretAtVerticalEdge
implementation, needed to accommodate incorrect results which can occur by retrieving the range at an offset too close to the edge of the container. Previously we would pass the height of the entire container, when therect
expected is that of the caret (a single line of text). Thus, for paragraphs more than just a couple lines of text, the buffer was much larger than expected, and the caret placed at the wrong position.Testing instructions:
Repeat testing instructions from #5541, for both long and short paragraphs.