Fix navigate regions backwards for macOS Firefox and Safari. #45019
Conversation
afercia
added
[Type] Bug
afercia
requested review from
ajitbohra,
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel and
kevin940726
as code owners
|
This has also been an issue for Windows. I just tested it in Windows with Firefox and Google Chrome, the issue is now resolved. I would be in favor of adding an E2E test for this to catch this bug in the future. Besides the relevant test case and the changelog entry, this is ✅ . Thanks. |
|
Not an expert but I'm not sure an E2E test would help. The E2E tests run on Chrome, where the bug can't be reproduced because Chrome returns (incorrectly?) the backtick character while Firefox and Safari return the tilde. |
|
#44631 will introduce the option of running new playwright end to end tests in firefox and webkit (safari-ish). |
|
Testing a bit more in depth: the Gutenberg shortcuts use isKeyboardEvent. It's based on macOS Chrome: Control + Shift + backtick returns the backtick character - works Firefox both platforms: Control + Shift + backtick returns the tilde character - fails Safari: Control + Shift + backtick returns the tilde character - fails Opera both platforms: The new alias will only mitigate the problem. For the future, maybe some refactoring to move away from |
afercia
force-pushed
the
fix/navigate-regions-shift-backtick
branch
from
7be7768 to
464e78b
Compare
|
Size Change: +2.17 kB (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
I can confirm the discrepancy (via this useful visualizer):
I guess the core of the issue is that the "backtick + shift" combination can produce two different values for Just to understand, what is the expected key combination for navigating backwards ? Ctrl+tilde, or ctrl+shift+backtick?
As highlighted above, this may be solved by using |
|
@afercia If I test latest trunk on Windows Google Chrome (latest), this currently does not work. The keyboard shortcut Is there a difference in the way Mac handles this as well? Thanks. |
|
Okay, the related PR was merged. @afercia Do you still have time to rebase and add E2E for this? |
@alexstine as mentioned earlier, Chrome appears to return a different
Adding the tilde alias should fix it, as a temporary workaround. Re: E2E tests, I'm afraid I won't be able to dedicate time to that. |
|
@ciampo Just added a Playwright E2E for this. I know it is not very detailed, but I think it will work for us for the moment until other tests can be migrated over. Thoughts? This is also my first Playwright E2E creation, hopefully it looks okay. Refreshed the branch, fixed conflicts, should be set on this one. Thanks. |
alexstine
added
[Package] Edit Post
|
Thank you for adding the e2e tests! To double-check, I ran them against I had a quick look at the To sum it up: these tests are useful for checking that backtick-related shortcuts allow a user to navigate regions, but they are not able to catch regressions caused by how different browsers interpret the combination of the I'm not a playwright expert and I'm not if there's a different way to simulate a keypress (cc @kevin940726 @talldan in case they have any suggestion?) |
|
Yeah, it seems like it's not possible in Playwright or Puppeteer. They both normalize the key codes. 😞 The best option I can think of is to manually test the inconsistency depending on the browser. if (testInfo.project.name === 'chromium') {
await page.keyboard.press('Control+Shift+`');
} else {
await page.keyboard.press('Control+Shift+~');
}FWIW, any other key combinations with |
|
Thanks for all the testing and feedback,
I'd agree. Can we merge this PR's temporary workaround for now? |
Sure! Do you want to adjust the test as per my suggestion above, or do you think it's sub-optimal and we should revisit it later? |
|
Will look into the test. |
|
Adjusted the test as suggested. Can confirm that, when removing the tilde alias, the test passes with Chromium and fails with Webkit and Firefox, as expected. To run this single test in 'headed' mode: |
| await expect( editorContent ).toBeFocused(); | ||
|
|
||
| // Navigate to previous/first region and check that we made it. | ||
| // Make sure navigating backwards works also witht he tilde character, |
There was a problem hiding this comment.
Nit: typo
| // Make sure navigating backwards works also witht he tilde character, | |
| // Make sure navigating backwards works also with the tilde character, |
| // Navigate to previous/first region and check that we made it. | ||
| // Make sure navigating backwards works also witht he tilde character, | ||
| // as browsers interpret the combination of the crtl+shift+backtick keys | ||
| // and assign it to event.key inconsistently. |
There was a problem hiding this comment.
Nit: Might be worth adding a link to this PR in the comment here. It's optional though as git blame will do the same 😅 .
|
Thank you @kevin940726 for taking the time to have a look and share feedback. |
Noticed while testing #44883
What?
When pressing the keys Shift + Control + backtick to navigate backwards through the editor regions, the returned
event.keyvaries across macOS browsers:~~My environment:
This makes region navigation fail on macOS Firefox and Safari, as the event.key is expected to match the backtick defined in the editor keyboard shortcuts.
Why?
Navigation through the editor regions is broken and needs to be fixed.
How?
Instead of trying to fix browser inconsistencies, seems to me the quickest way to fix this is by defining one more shortcut alias for the
Navigate to the previous part of the editorcommand.Testing Instructions
Test with Firefox or Safari on macOS. Make sure to test with an keyboard with an English layout or set your keyboard layout to English, as the backtick key may not be present on other keyboard layouts.
Before:
After:
Screenshots or screencast