Enter editing mode via Enter or Spacebar #58795
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
jeryj
requested review from
kevin940726,
Mamaduka and
ellatrix
as code owners
|
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 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. |
jeryj
added
[Type] Bug
|
Size Change: -1.54 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
scruffian
reviewed
Feb 9, 2024
| await admin.visitSiteEditor(); | ||
|
|
||
| // Navigate to the iframe | ||
| await pageUtils.pressKeys( 'Tab', { times: 3 } ); |
There was a problem hiding this comment.
This feels quite fragile. Is there a way to focus the iframe directly without relying on the number of tab presses?
There was a problem hiding this comment.
I think this is correct because we should be aware we introduced a new tab stop?
scruffian
reviewed
Feb 9, 2024
| */ | ||
| const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' ); | ||
|
|
||
| test.describe( 'Site editor navigation', () => { |
There was a problem hiding this comment.
This test feels quite big and I worry that it's going to need a lot of work to maintain it as the editor continues to evolve. I think it would be better if we could find a way to just test the spacebar press on its own.
draganescu
reviewed
Feb 9, 2024
| await requestUtils.activateTheme( 'twentytwentyone' ); | ||
| } ); | ||
|
|
||
| test( 'Can use keyboard to navigate the site editor', async ( { |
There was a problem hiding this comment.
'Can use keyboard to navigate the site editor sounds like the whole battery of tests instead of a single test - and looking at the many expectations it's kinda true.
| await admin.visitSiteEditor(); | ||
|
|
||
| // Navigate to the iframe | ||
| await pageUtils.pressKeys( 'Tab', { times: 3 } ); |
There was a problem hiding this comment.
I think this is correct because we should be aware we introduced a new tab stop?
|
It’s important to know we can use the keyboard to navigate between modes, so if we split this into several tests, it’s going to repeat a lot of the same work across multiple tests, not be as robust, and take way longer to run. For e2e tests, one larger test is preferable than multiple shorter ones.
That said, I really don’t like the 9 shift tabs to go back. I should replace that with a region shortcut. On the other hand, maybe it’s a UX issue that we require 9 tab stops to return to the previous view.
…On Fri, Feb 09, 2024 at 6:55 AM Andrei Draganescu < Andrei Draganescu ( Andrei Draganescu ***@***.***> ) > wrote:
***@***.**** commented on this pull request.
This is great, I think we should split the test into a few smaller ones
because the big one ... is too big 😄
In test/e2e/specs/site-editor/navigation.spec.js (
#58795 (comment) )
:
> @@ -0,0 +1,80 @@ +/** + * WordPress dependencies + */ +const { test,
expect } = require( ***@***.***/e2e-test-utils-playwright' ); +
+test.describe( 'Site editor navigation', () => { + test.beforeAll( async
( { requestUtils } ) => { + await requestUtils.activateTheme( 'emptytheme'
); + } ); + + test.afterAll( async ( { requestUtils } ) => { + await
requestUtils.activateTheme( 'twentytwentyone' ); + } ); + + test( 'Can use
keyboard to navigate the site editor', async ( {
' Can use keyboard to navigate the site editor sounds like the whole
battery of tests instead of a single test - and looking at the many
expectations it's kinda true.
In test/e2e/specs/site-editor/navigation.spec.js (
#58795 (comment) )
:
> + await requestUtils.activateTheme( 'emptytheme' ); + } ); + +
test.afterAll( async ( { requestUtils } ) => { + await
requestUtils.activateTheme( 'twentytwentyone' ); + } ); + + test( 'Can use
keyboard to navigate the site editor', async ( { + admin, + page, +
pageUtils, + } ) => { + await admin.visitSiteEditor(); + + // Navigate to
the iframe + await pageUtils.pressKeys( 'Tab', { times: 3 } );
I think this is correct because we should be aware we introduced a new tab
stop?
—
Reply to this email directly, view it on GitHub (
#58795 (review)
) , or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AAHMHOCUYBXPBFHDJKFHCU3YSYME3AVCNFSM6AAAAABC6SDOPGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZSGMYDQOJVG4
).
You are receiving this because you were assigned. Message ID: <WordPress/gutenberg/pull/58795/review/1872308957
@ github. com>
|
Yeah I feel like maybe we need a keyboard shortcut that switches between views? |
|
What do you think about limiting the key that triggers edit mode to only the Enter key? In common browsers, the spacebar is used to scroll pages. If you have a mouse, you can operate the mouse wheel on the canvas to see the entire canvas without entering edit mode. If we use the spacebar as a trigger key, users who only use the keyboard won't be able to do the same thing. |
If the element trigger is a button, enter/space keys should activate it. Thanks. EDIT: At mention wrong person. |
This iframe has |
There was a regression in being able to enter editing mode via keydown on the site editor due to the onKeydown event being blocked. This allows an onKeydown prop to be fired if passed to the iframe. I'm not positive this is the best way. I also added a test to check for this as well as general keyboard navigation of the site editor in view mode.
|
This is a pretty big regression, so we should sort it out soon. l'm looking at the failing tests. Does anyone have thoughts on the fix here? |
jeryj
force-pushed
the
fix/keydown-on-site-iframe
branch
from
bf1b6fe to
030b6ea
Compare
|
Tests are passing locally after rebasing, so I'm force pushing to see if the tests will pass. |
t-hamano
reviewed
Feb 13, 2024
There was a problem hiding this comment.
The changes regarding onKeydown seem to make sense to me, but the e2e tests seem a bit redundant to me as they try to cover many scenarios in one test.
For example, how about dividing the test by scenario and clarifying what to expect, as shown below.
test.beforeEach( async ( { admin } ) => {
await admin.visitSiteEditor();
} );
test.describe( 'Site editor keyboard navigation', () => {
test( 'should open command palette via button press', async ( {} ) => {} );
// test( 'should open command palette via keyboard shortcut', async ( {} ) => {} );
// test( 'should not lose focus when switching screens', async ( {} ) => {} );
test( 'should enter into editing mode via button press', async ( {} ) => {} );
test( 'should not lose focus when returning to view mode', async ( {} ) => {} );
} );However, I think the following scenario is unnecessary.
should open command palette via keyboard shortcut
What we're hoping for this time is not whether the keyboard shortcuts will work correctly, but whether the tab stops will work as expected.
should not lose focus when switching screens
I believe this is covered in the Navigator component's unit test.
|
My philosophy on e2e tests is that they should be a more realistic test of how the app is used. Also, having one large test with multiple steps is faster due to not needing to run the admin page setup each time (5s vs 10s). It is also more likely to catch odd edge cases this way. For example, you might be able to click the pages navigation item, have focus on the back button, then press it and return to the page. But, does everything work in tandem? Can you do multiple steps and not lose focus along the way? Sometimes things have unintended side effects, and I think e2e tests are great for catching this. So, while this is a big test, everything here should work. My main concern is that the test will give false negatives if the amount of tab stops changes. I don't want to test the amount of tab stops between the items. I want to test that you can navigate the site editor with the keyboard without losing focus, and splitting this longer test into more tests will give me less confidence that I can do that. I will update the e2e test though to be clearer about what is being tested in each step, but leave them within one larger test. This is a pretty common pattern in our e2e testing, such as in the navigation block frontend interactivity tests.
I think that test is a more isolated example, not testing within the site editor. |
|
I made several changes to the e2e tests based on feedback:
|
|
Flaky tests detected in fea790d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7917697844
|
t-hamano
reviewed
Feb 14, 2024
There was a problem hiding this comment.
Does anyone have thoughts on the fix here?
This fix should make sense. Originally, Iframe components received most props as is, including onKeyDown.
However, #54565 appears to have overridden the onKeyDown prop.
cc @ellatrix
Co-authored-by: Aki Hamano <[email protected]>
Co-authored-by: Aki Hamano <[email protected]>
t-hamano
approved these changes
Feb 15, 2024
Co-authored-by: Aki Hamano <[email protected]>
Co-authored-by: Aki Hamano <[email protected]>
Co-authored-by: Aki Hamano <[email protected]>
|
Thanks @t-hamano! I updated based on your excellent feedback. |
jeryj
added
the
Backport to WP 6.7 Beta/RC
|
Friendly reminder about the props bot ;) |
aristath
added a commit
that referenced
this pull request
Feb 16, 2024
* trunk: (78 commits) Components: Use `Element.scrollIntoView()` instead of `dom-scroll-into-view` (#59085) DataViews: Correctly display featured image that don't have image sizes (#59111) Elements: Fix block instance element styles for links applying to buttons (#59114) Allow editing of image block alt and title attributes in content only mode (#58998) Add toggle for grid types and stabilise Grid block variation. (#59051) Global Styles: fix console error in block preview (#59112) Navigation: Avoid using embedded records from fallback API (#59076) Refactor responsive logic for grid column spans. (#59057) Editor: Unify Mode Switcher component between post and site editor (#59100) Move StopEditingAsBlocksOnOutsideSelect to Root (#58412) Fix logic error in #58951 (#59101) Block-editor: Auto-register block commands (#59079) Fix small typo in rich text reference guide (#59089) Components: Add lint rules for theme color CSS var usage (#59022) Enter editing mode via Enter or Spacebar (#58795) Updating Storybook to v7.6.15 (latest) (#59074) CustomSelectControl (v1 & v2): Fix errors in unit test setup (#59038) Add stylelint rule to prevent theme CSS vars outside of wp-components (#59020) ColorPicker: Style without accessing InputControl internals (#59069) Pattern block: batch replacing actions (#59075) ...
getdave
pushed a commit
that referenced
this pull request
Feb 20, 2024
There was a regression in being able to enter editing mode via keydown on the site editor due to the onKeydown event being blocked. This allows an onKeydown prop to be fired if passed to the iframe. Also added a test to check for this as well as general keyboard navigation of the site editor in view mode. --------- Co-authored-by: jeryj <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: alexstine <[email protected]>
|
I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: 6e5a64f |
getdave
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
There was a regression in being able to enter editing mode via keydown on the site editor due to the onKeydown event being blocked. This allows an onKeydown prop to be fired if passed to the iframe. Also added a test to check for this as well as general keyboard navigation of the site editor in view mode. --------- Co-authored-by: jeryj <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: alexstine <[email protected]>
ellatrix
reviewed
Feb 29, 2024
| // If the event originates from inside the iframe, it means | ||
| // it bubbled through the portal, but only with React | ||
| // events. We need to to bubble native events as well, | ||
| // though by doing so we also trigger another React event, | ||
| // so we need to stop the propagation of this event to avoid | ||
| // duplication. | ||
| if ( | ||
| else if ( |
There was a problem hiding this comment.
This doesn't make any sense. When you have multiple keydown handlers, all of them should execute
There was a problem hiding this comment.
The only time the if statement above it runs is when a keydown listener is passed to the iframe, which is only in the view mode of the site editor. The shortcuts shouldn't be enabled at that point since we're not within the editor.
There was a problem hiding this comment.
Ok, but that's not clear from reading this. Also there might be other iframe instances that pass onKeydown and do want shortcuts disabled. Using onKeydown to disable/enable shortcuts seems a bit strange, imo that should be done through a separate prop. Maybe reuse isPreviewMode?
There was a problem hiding this comment.
I'm not sure how all of that should be structured. The only thing important to me in this PR was fixing a bug where you couldn't use the keyboard to enter into Edit mode from View mode. This onKeydown was not allowed to run.
Very open to fixing this another way. I wasn't sold on this solution.
There was a problem hiding this comment.
No, this is fine, all I'm saying is that the else shouldn't have been added?
There was a problem hiding this comment.
Oh - That's fine with me: #59474
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?
Allow entering into edit mode with keyboard enter/spacebar press on the site editor frame in view mode.
Why?
There was a regression in being able to enter editing mode via keydown on the site editor due to the onKeydown event being blocked.
How?
This allows an onKeydown prop to be fired if passed to the iframe. I'm not positive this is the best way.
I also added a test to check for this as well as general keyboard navigation of the site editor in view mode.
Testing Instructions