Basic cypress tests for focusing blocks in navigation and edit modes #3399
Conversation
|
So are we happy with how these tests will enforce keyboard navigation? @jasmussen @afercia
The selectors being used by the tests were just based on current implementation details in #3195 --- they're not set in stone. These tests aren't prescribing exactly what gets focus. They are really just enforcing that there should be an outer wrapper, and an inner wrapper of some kind, and that |
ephox-mogran
changed the title
ephox-mogran
changed the title
This sounds like a good way to start this, yes. If we can get this in, and test, then we can probably do improvements from there. Nice work. CC: @aduth @youknowriad mind taking a look? Thanks. |
| @@ -0,0 +1,34 @@ | |||
| Cypress.Commands.add( 'auditBlockFocus', ( blockType ) => { | |||
There was a problem hiding this comment.
Do you think this is a global command or would it be better if we just loop over an array of blocks and do this instead in the test directly? My thinking is that this is very specific to this test and no something we want to share across different tests.
There was a problem hiding this comment.
It was originally just a forEach in the test. Then, I spit it out into separate tests thinking that I'd use different files for each one. Then I decided to just use one file. Now, it should definitely just be in that one file.
| @@ -0,0 +1,34 @@ | |||
| Cypress.Commands.add( 'auditBlockFocus', ( blockType ) => { | |||
| // Open up the Insert Block Menu | |||
| cy.get( 'button.editor-inserter__toggle:first' ).click() | |||
There was a problem hiding this comment.
We should be consistent with the way we do actions across tests. Use the same selectors: I'm using .editor-visual-editor [aria-label="Insert block"] because I think we should try to target text and labels instead of classnames as much as we can.
I wonder if something like cy.openInserter() would be a good Idea?
There was a problem hiding this comment.
Let me promote using data attributes like data-test="editor-inserter" again. It would make tests much easier to maintain. In case of any change in the HTML code, you would move data attribute and tests should pass without any modification :)
There was a problem hiding this comment.
Sounds good to me.
| // Type into the Search field for blocks | ||
| cy.focused().type( blockType ); | ||
| // Click on the first result that matches. Note, be careful with things like Image vs Cover Image | ||
| cy.get( '.editor-inserter__menu .editor-inserter__block:contains("' + blockType + '"):first' ).click(); |
There was a problem hiding this comment.
Re selectors, Is it a good time to introduce data-test attributes? or at least using constants to avoid repeating the selectors over and over again across tests. For example this could be: FIRST_INSERTER_BLOCK
There was a problem hiding this comment.
I'm glad we are on the same page. I really need to start reading all comments before I comment anything :)
This is probably not the right PR to ask this question but why do we need an outer/inner wrapper to switch between navigation/edit mode? I don't think we should enforce this on blocks authors (provide an inner focusable elemenet) but just rely on a state value saying that we're on navigation or edit mode to change the behavior of the tabs/arrows. |
I've also been wondering about whether a mode state would help here when interpreting arrow keys. However, the inner and outer wrapper aren't just required for handling the differences in navigation and edit mode. I think the idea is that an inner and an outer wrapper are required at least for |
This branch is mostly just an experiment to see how to start making more substantial UI tests. It's the first time I've used cypress as well, so it's not polished. All of the tests here are skipped, so they can be merged at any time. The plan would be to remove each skip when a developer starts working on the focus for a particular block. We could use this test to see how many blocks still require focus management fixes. If that's not how your preferred approach for using tests though, I understand. |
This seems to me the most important thing, so 👍
I think the most important thing is to standardize the behavior. Editable and non-editable blocks are different in this and Gutenberg should provide some sort of standardization out of the box, without relying on what blocks authors do. About the technical details I don't have a strong opinion. I just think authors shouldn't worry about the navigation/edit mode interaction and Gutenberg should take care of that. |
Exactly, that's why I was saying we should force block authors to add inner wrappers. The outer wrapper is "already" standardized and It's totally understandable for "Text" blocks to have an "inner" focusable. Granted, right now we do some "magic" in some of these blocks to focus this inner wrapper when we're in "edit" mode and this logic should be extracted out of the block if possible to make it consistent across all text blocks. One option here is to find "tabbables" in the block content and if the first one is an input/textarea or contenteditable (Maybe we can move the focus there if we find any "tabbable" element, not sure about this though) than we could move focus to this first tabbable element. |
Maybe an edge case, but what if a custom block doesn't have focusable elements at all? For example, it could have just settings in the sidebar. And what about the latest posts widget and similar blocks? Where focus should land when entering edit mode? An inner wrapper provided by Gutenberg would be a standardized, predictable, place to move focus to. |
It's not possible because for text blocks the inner wrapper is provided by the blocks, it's the editable or input or whatever. For Blocks without any focusable, we can stay on the outer Wrapper and we have our state variable saying that we're on edit (or navigation) mode. |
|
The advantage of testing like this is as soon as decide what we want to do, we can enforce that it works for every block. For example, if we go with what @youknowriad is proposing, we could change these tests to:
Because we aren't being overly prescriptive about where the focus should go (just properties that should hold for the focused element), these types of tests can be relatively flexible. They are approaching the types of property tests made famous by libraries like quickcheck, without the random generation. What having these types of tests do is define the behaviour that we are expecting. Instead of it being a discussion which can get misinterpreted, there is a single place where every assumption is stated. That's why I'm advocating having tests like this. IMO, it makes it much easier to discuss what we actually want to happen, and this particular test is not going to be hard to change. |
ntwb
added
the
[Type] Automated Testing
gziolo
added
the
[Status] Stale
|
@youknowriad @ephox-mogran, what is the status of this one? Is it something that we still get into master or does it need more work? It hasn't been updated for some long time so it might not work with all the changes applied recently. |
|
Let's close as this staled for a long time now, fine to reopen if it gets any more activity |
Description
Introducing cypress tests for the proposed navigation being discussed in #3195. Essentially, what they test are:
For every block that can be found in the
Blockstab (so perhaps not embed)escapeenterprevious-focus)The goal is to ensure that switching between navigation and edit mode works for all blocks, and that keyboard focus is initially given to each block once created. At the moment, these tests fail due to:
How Has This Been Tested?
It is tests. They are all skipped though, because the navigation has not been implemented yet.
Screenshots (jpeg or gifs if applicable):
Types of changes
Adds to the number of tests.
Checklist: