Shortcuts: don't fire keydown handlers outside block editor content area #37326
Conversation
ramonjd
added
[Status] In Progress
|
Size Change: +1.01 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
try/prevent-block-editor-key-down-events-when-not-focussed
branch
from
6155a1b to
5864da5
Compare
ramonjd
changed the title
There was a problem hiding this comment.
Thanks so much for diving into this one @ramonjd! This is testing nicely for me in the editor and looks like a good approach to me. I just looked at the failing e2e test, and it appears to be failing for me locally, too with this PR:
Not sure if the change in behaviour here has unintentionally affected the widget editor? From my manual testing, the undo behaviour is a little flaky in the widget editor on trunk, too, so I wasn't too sure what the real culprit is there 🤔
To run the e2e test locally, I ran the following in one terminal with wp-env start already running (with npm run dev already running in another terminal for a dev build):
PUPPETEER_HEADLESS="false" npm run test-e2e specs/widgets/editing-widgets.test.js
|
Thanks for testing @andrewserong
I saw that failing test and 🤞 that it was another flaky one 🤣 Thanks for looking into it further. I ran out of time to look at it more closely today. I'll investigate tomorrow. |
|
… does not have focus.
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
|
Turns out that checking for the ref element before we use it is a good idea 😆 |
ramonjd
force-pushed
the
try/prevent-block-editor-key-down-events-when-not-focussed
branch
from
9ead25c to
9b0c546
Compare
There was a problem hiding this comment.
@ramonjd, I just gave this another test, and it's still working nicely for me in the post editor, but there's another couple of curious cases of uses of the BlockTools component that I was unsure of. Apologies for not catching this sooner!
In the site editor, it looks like selecting multiple blocks at the same time and pressing backspace/delete doesn't current delete the selected blocks. I could replicate the same issue on trunk, so I don't think it's introduced by this PR, but I also wasn't quite sure why it wasn't working. Possibly due to the iframed editor?
There's another case that this fix doesn't appear to resolve, which is more of an edge case but would still be good to see if we can fix, and that's the template editor. With this PR applied, I did the following:
- Open up a post in the post editor
- Under
Templateselect edit - Within the template, scroll down and select two paragraphs (assuming you already have two paragraphs in the template, otherwise add them in)
- Go to change the color in the sidebar, and pressing backspace in the input field removes the selected paragraphs:
Kapture.2021-12-16.at.12.38.30.mp4
I wonder if that's another iframe related issue?
|
Thanks for catching this behaviour. I think you're right: the iframe doesn't know about any active elements outside of its context. The container passed to This is making me think I'll need to retreat back to the original idea of adding At least until we work out a better way. It's not a fantastic experience the way it is. The might be a neat way of adding sugar to them all. |
…o that the event does not bubble up to the parents in the block editor.
|
@andrewserong It turns out I was misinterpreting the Slot Fill docs.
In 84a3c7c I've removed the active element check and instead am passing down It fixes most things we've discovered, but there might be side effects. I'll keep testing. 😄 |
|
Thanks for pressing on with it @ramonjd! This is working nicely for me in the post editor and template editor now using the One thing for us to consider is that in #37024 the ability to pass down |
|
I'd like to add @ellatrix to this PR as If I remember properly, she worked on very similar goals in the past. |
| @@ -66,7 +66,7 @@ const BlockInspector = ( { showNoBlockSelectedMessage = true } ) => { | |||
| return ( | |||
| <div className="block-editor-block-inspector"> | |||
| <MultiSelectionInspector /> | |||
| <InspectorControls.Slot /> | |||
| <InspectorControls.Slot bubblesVirtually={ false } /> | |||
There was a problem hiding this comment.
I'm not sure this is the right fix, If I remember properly, this breaks context propagation in the fills. For instance if there's a context being provided around the slot like we do sometimes for BlockEditContextProvider, the inspector controls won't receive these values anymore. I remember in the past the impossibility to use RichText inside of Inspector controls because of this.
For these reasons, we've preferred moving all slots to bubblesVirtually, The other implementation might be here just for native usage (where portals are not supported)
There was a problem hiding this comment.
I'm not sure this is the right fix, If I remember properly, this breaks context propagation in the fills.
Yeah, it was more of a 🤞 so thanks for the warning.
The fallback, if we can't come up with a neater way, would be to try to stop propagation on key events for these particular text controls. Maybe even targeting del and backspace specifically. 🤷
There was a problem hiding this comment.
be to try to stop propagation on key events for these particular text controls. Maybe even targeting del and backspace specifically
What the heck. 😄
It's more low level, but might be a backup approach if we can't target the events at a higher level.
|
With the fix proposed in #37595, it looks like we should be able to close out this PR. |
|
Closing in favour of #37595 (thanks for digging into this one, Ramon!) |
youknowriad
deleted the
try/prevent-block-editor-key-down-events-when-not-focussed
branch
Let's see if this PR resolves:
Description
Just an experiment. Don't take it too seriously... unless it works that is
Prevent block editor keyboard shortcuts where the block editor itself does not have focus.
The concept rides on the claim that we shouldn't fire editor-specific callbacks where the editor area, or its children, are not focussed.
How has this been tested?
When writing/editing in the block editor, you should be able to use all the Block Tool keyboard shortcuts:
Check that deleting control values in the Block Inspector does not affect the Block Editor:
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).