Writing Flow: Fix vertical arrow navigation skips #7877
Conversation
aduth
added
[Type] Bug
|
This tested well for me, thanks! |
aduth
force-pushed
the
fix/6524-vertical-nav
branch
from
f370e8a
to
f009562
Compare
|
This was a bit of a challenge to author end-to-end tests for, due to some unrelated bugginess (#6524 (comment)) and the fact that the arrow skipping wasn't always predictable for me. It seems to be relatively predictable with the implemented test (a preformatted block with at least one blank line). |
packages/dom/src/dom.js
Outdated
| return ( | ||
| // Compare whether anchor node precedes focus node. | ||
| position !== DOCUMENT_POSITION_PRECEDING && | ||
|
|
There was a problem hiding this comment.
This empty newline strikes me as a bit odd. 🤷♂️
There was a problem hiding this comment.
This empty newline strikes me as a bit odd. 🤷♂️
It's probably more symptomatic of it being an issue to need such descriptions in a multi-line return value, which could be achieved in a more verbose / easy-to-understand format by simply splitting out the return statements. I've done so in the rebased 52b8f97.
packages/dom/src/dom.js
Outdated
| * Check whether the selection is horizontally at the edge of the container. | ||
| * | ||
| * @param {Element} container Focusable element. | ||
| * @param {boolean} isReverse Set to true to check left, false for right. |
There was a problem hiding this comment.
This seems like an odd variable name–we aren't setting if something isReverse, we're checking for it. Would we want checkForReverse or maybe to specify the direction? eg. isHorizontalEdge( container, direction='left' )? I know it's a more verbose API but it's also easier to grok. Supplying a binary to this function doesn't provide much context in isolation. (isHorizontalEdge( container, false )–what is false in that bit?)
There was a problem hiding this comment.
I'd probably tend to agree, but it's not within scope of the pull request, and since this is a published module, such a change would qualify as breaking. Since I'm already touching the documentation, I'll at least try to make the boolean-ness more clear.
There was a problem hiding this comment.
Yeah, that's fair. I just figured I'd point it out either way. If I still feel strongly about it I'll file a code quality issue later 😉
There was a problem hiding this comment.
Yes, it's good to point out. I think it's easy to fall into the trap of accepting "simple" values as arguments when authoring the function originally, and not considering that it would be difficult to interpret the intent of the statement isHorizontalEdge( container, false ).
packages/dom/src/dom.js
Outdated
|
|
||
| return true; | ||
| // Edge reached if effective caret position is at expected extreme. |
There was a problem hiding this comment.
"extreme" is maybe an odd word to use here–I guess it's alright but maybe rephrasing this to something like:
// Check if the caret position is at the expected start/end position (depending on the
// value of `isReverse`). If so we consider the horizontal edge to be reached.🤷♂️
| * @param {boolean} isReverse Set to true to check top, false for bottom. | ||
| * @param {boolean} collapseRanges Whether or not to collapse the selection range before the check. | ||
| * @param {Element} container Focusable element. | ||
| * @param {boolean} isReverse Set to true to check top, false for bottom. |
There was a problem hiding this comment.
Same here, I just find this API a bit odd. If we have to do deprecations to change it we could file an issue, but I just find it hard to understand in isolation.
packages/dom/src/dom.js
Outdated
| // `getClientRects` can be empty in some browsers. This can be resolved | ||
| // by adding a temporary text node to the range. | ||
| if ( ! rect ) { | ||
| const padNode = document.createTextNode( '\u200b' ); |
There was a problem hiding this comment.
What is that character and why is it used? It's probably just worth documenting whether it matters for future devs.
There was a problem hiding this comment.
Good call. I'll add some clarification with a link to the resource where I'd found the tip (StackOverflow).
aduth
force-pushed
the
fix/6524-vertical-nav
branch
from
f009562
to
52b8f97
Compare
| // Check if the caret position is at the expected start/end position based | ||
| // on the value of `isReverse`. If so, consider the horizontal edge to be | ||
| // reached. | ||
| const caretOffset = range.toString().length; |
Fixes #7838
Fixes #6524
Salvaged from #6467
This pull request seeks to pull in improvements to the
isHorizontalEdgeandisVerticalEdgeproposed in #6467 to resolve multiple issues around navigating multi-line content within contenteditables.Testing instructions:
Repeat testing instructions from #7838 and #6524, observing expected results.