Allow multi-select on iOS Safari/touch devices #63671
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @iamthomasbishop, @porg. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +2.41 kB (+0.14%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
This was referenced Jul 24, 2024
ellatrix
force-pushed
the
fix/io-multi-select
branch
2 times, most recently
from
8f32a38 to
51a5ce9
Compare
ellatrix
commented
Aug 7, 2024
| if ( ownerDocument.activeElement !== element ) { | ||
| // If it is not, we can stop listening for selection changes. We | ||
| // resume listening when the element is focused. | ||
| ownerDocument.removeEventListener( |
There was a problem hiding this comment.
To do: it would be good to attach one event handler in the first instance, then reuse it for all instances.
ellatrix
force-pushed
the
fix/io-multi-select
branch
from
4b74ad5 to
31cfb86
Compare
ntsekouras
reviewed
Aug 13, 2024
There was a problem hiding this comment.
It's a very specialized PR with selections and events, so I took a first look and left some questions to start understanding better. Sorry if some of them might seem obvious to you.
Does your redirection approach mean that we continuously move focus on every related event or it's just about keeping the new focused node (writing flow wrapper) and redirecting the event to rich text so it behaves as if it had focus?
|
|
||
| const init = {}; | ||
|
|
||
| for ( const key in event ) { |
There was a problem hiding this comment.
Why not just spread the object here?
packages/block-editor/src/components/writing-flow/use-arrow-nav.js
Outdated
Show resolved
Hide resolved
| export default function useEventRedirect() { | ||
| return useRefEffect( ( node ) => { | ||
| function onInput( event ) { | ||
| if ( event.target !== node || event.__isRedirected ) { |
There was a problem hiding this comment.
When this event.__isRedirected is true?
| return; | ||
| } | ||
| } | ||
|
|
||
| if ( event.keyCode === ENTER ) { | ||
| if ( event.shiftKey || __unstableIsFullySelected() ) { |
There was a problem hiding this comment.
Not from here, but curious how Enter and shift are used.. Any chance you remember?
There was a problem hiding this comment.
Yes, this is Enter behaviour for partial selection. So select two paragraphs partially and press Enter, this handles split for them.
| ] ); | ||
|
|
||
| // Ensure that the newly created empty block is focused. | ||
| await expect.poll( editor.getBlocks ).toHaveLength( 3 ); | ||
| await expect.poll( editor.getBlocks ).toHaveLength( 2 ); |
There was a problem hiding this comment.
Why you had to change this?
There was a problem hiding this comment.
Because it's the only thing in the e2e tests that this PR changed in behaviour. Arrow up selects two blocks. Then press Backspace. Instead of leaving an empty block, it no longer leaves an empty block and puts selection at the end of the previous block.
Yes, we keep focus on the wrapper, and instead clone the event and dispatch it onto the content editable element that has the selection. I mention in the PR description that I also tried maintaining focus on the original content editable, but this is pretty fragile as the browser keeps setting it back to writing flow. |
ellatrix
force-pushed
the
fix/io-multi-select
branch
2 times, most recently
from
1b2ea33 to
1cfdedb
Compare
ellatrix
force-pushed
the
fix/io-multi-select
branch
from
8403c2b to
55d09d3
Compare
|
Flaky tests detected in 6674904. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10392006794
|
ellatrix
force-pushed
the
fix/io-multi-select
branch
from
6674904 to
8579290
Compare
ntsekouras
approved these changes
Aug 22, 2024
This was referenced Aug 22, 2024
|
🎉 This stone incidentally got another bird #64617. |
This was referenced Aug 23, 2024
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
2 tasks
2 tasks
jeryj
added a commit
that referenced
this pull request
Sep 25, 2024
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #16206.
Why?
Currently it's not possible to select multiple block on iOS at all.
How?
As soon as there's an uncollapsed selection, prepare for multi-selection with the content editable wrapper.
Note that we already do this for pointer device selection, when the pointer drags out of a block, and also whenever you arrow select out of a block.
The one consequence of doing this is that focus will move from the rich text element in the block to the writing flow wrapper. There's a potential fix by always forcing focus back to the original rich text element, but the browser will continuously put focus back to writing flow whenever an event happens. The other solution, which I have implemented in this PR, is to dispatch a cloned event on the rich text element that does not bubble, so all the usual event handlers are called.
Alternatives considered
I considered a more similar approach to what we currently have for pointer devices. There's no
touchoutevent like there is amouseoutevent, but that can be fixed. On touch devices, you can only change an uncollapsed selection when there already is an uncollapsed selection (to get the handle bars). So we could enable the content editable wrapper "right in time" ontouchstartwhenever there is already an uncollapsed selection. However, I tried this multiple times and iOS seems to lose the selection whenever the content editable wrapper is turned on... The approach above is the only one that I could make work, and all e2e tests pass as well.Testing Instructions
This can more easily be tested with Playground. Open this link in Safari iOS and try to select multiple paragraphs.
Screenshots or screencast