Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create the demo post content using the keyboard only #10682

Merged
merged 6 commits into from
Oct 17, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Oct 17, 2018

Everything is on the title. I'm almost there:

  • Testing the result is hard because of the ids and the temporary names.
  • The media library is difficult to use with keyboard and is also throwing JS errors sometimes (thus the commented blocks, we can uncomment once the JS errors are fixed)
  • Alt + F10 move to the toolbar only if it's visible, we should fix that.

@tofumatt tofumatt added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 17, 2018
@tofumatt
Copy link
Member

billy-d_approves

@mtias mtias added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Oct 17, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool. 🎉

Just a few comments; I know this isn't 100% finished yet. Ace, though.

//getEditedPostContent,
} from '../support/utils';

async function uploadImageInTheMediaLibrary( assetFileName ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a handy utility in general! 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but this one is very keyboard specific. I mean it could be considered generic but we need to include the whole block and not just assume the media library is open. I think it's fine to keep this one internal to this test for now.

await page.keyboard.up( META_KEY );
}

jest.setTimeout( 1000000 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is for the upload, but worth adding a comment to explain 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it's because this test take too long since it's a very long post :P (I think our current limit is 10 seconds)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah! Right, that makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis doesn't like it:

waiting for selector ".media-modal input[type=file]" failed: timeout 30000ms exceeded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a different error @gziolo Intermittent, I saw it once locally but I don't know the real issue :(

test/e2e/specs/demo-content-with-keyboard.test.js Outdated Show resolved Hide resolved
await pressTimes( 'Tab', 4 ); // We should have a way to focus the block content
await page.keyboard.press( 'Enter' );
await uploadImageInTheMediaLibrary( 'cover-1.jpg' );
await page.click( '.media-modal button.media-button-select' ); // validating the media gallery is not easy with keyboard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easy or impossible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible but you need to count "tabs" which is not great and break easily. There's probably a way to do it which is "tab + test if we reached the correct button"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the tabbing breaks we should know about it, so I'd really prefer if we used a (fragile) keyboard solution here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inserting media was a bit painful in my manual testing, I had to go through all the fields before I could insert.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would:

Suggested change
await page.click( '.media-modal button.media-button-select' ); // validating the media gallery is not easy with keyboard
await tabUntilElementIsActive( '.media-modal button.media-button-select' );

work here? If so it'd be great, but if not: okay. 🤷‍♂️ A follow-up issue to track it would be good though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would work yet but I consider tabUntilElementIsActive not an ideal solution. In general we should have some direct ways to access these buttons (like aria-regions in the medial library for instance).

But I'll update as it's slightly better.

// Create the aligned right paragraph
await page.keyboard.press( 'Enter' );
await page.keyboard.type( '... like this one, which is right aligned.' );
await page.mouse.move( 200, 300, { steps: 10 } ); // This shouldn't be necessary to show the toolbar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, mentioned in the description. We need to fix that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you select the text you can get around it for now as it will show the toolbar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even just command + A for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hot take: using (and thus highlighting) awkward interactions required by keyboard-only usage is better than using the mouse. 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the blocks don't have "text" so "command + A" won't always work. (Embed block for instance).

@mtias
Copy link
Member

mtias commented Oct 17, 2018

This is really cool, thanks.

@gziolo
Copy link
Member

gziolo commented Oct 17, 2018

This is great 💯

@aduth
Copy link
Member

aduth commented Oct 17, 2018

Alt + F10 move to the toolbar only if it's visible, we should fix that.

Raised also in Slack (link requires registration):

https://wordpress.slack.com/archives/C02QB2JS7/p1539202163000100

There, the focus of the discussion was on how to ensure the toolbar is visible so that Alt+F10, but your phrasing here sparked in my mind we should ensure the keybind does its intended operation regardless of the toolbar visibility.

Related ideation in #10529. I'm interested in exploring this further.

@youknowriad youknowriad force-pushed the add/create-demo-post-using-keyboard-only branch from 17f25ab to a9eac33 Compare October 17, 2018 14:30
@youknowriad
Copy link
Contributor Author

Youhou the test is passing on Travis 🎉 . The e2e tests take 1m30s longer but I think it's worth it.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start and we should merge it.

There are a few things we should file like where we skip keyboard nav because it's annoying or buggy? Either we should check if they're filed and link to the issues in the code/comments or file follow-up bugs to perfect this… but still this is such a great first, giant step. 🂡

getEditedPostContent,
} from '../support/utils';

async function tabUntilElementIsActive( text ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COOOOOOOOL. We'll probably want to move that to the support utils eventually. 👍

test/e2e/specs/demo-content-with-keyboard.test.js Outdated Show resolved Hide resolved
await pressTimes( 'Tab', 4 ); // We should have a way to focus the block content
await page.keyboard.press( 'Enter' );
await uploadImageInTheMediaLibrary( 'cover-1.jpg' );
await page.click( '.media-modal button.media-button-select' ); // validating the media gallery is not easy with keyboard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would:

Suggested change
await page.click( '.media-modal button.media-button-select' ); // validating the media gallery is not easy with keyboard
await tabUntilElementIsActive( '.media-modal button.media-button-select' );

work here? If so it'd be great, but if not: okay. 🤷‍♂️ A follow-up issue to track it would be good though.

@youknowriad youknowriad force-pushed the add/create-demo-post-using-keyboard-only branch from 8c7fb4f to 66ad03d Compare October 17, 2018 15:55
@youknowriad youknowriad merged commit 237d6ed into master Oct 17, 2018
@youknowriad youknowriad deleted the add/create-demo-post-using-keyboard-only branch October 17, 2018 16:31
@aduth
Copy link
Member

aduth commented Oct 17, 2018

Failure noted in Slack:

https://wordpress.slack.com/archives/C02QB2JS7/p1539801336000100

 FAIL  test/e2e/specs/demo-content-with-keyboard.test.js (79.755s)
  ● Demo content post › should be created with keyboard
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 403 (Forbidden)"]
      32 |     function assertExpectedCalls() {
      33 |         if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 |             expect( console ).not[ matcherName ]();
         |             ^
      35 |         }
      36 |     }
      37 | 
      at Object.assertExpectedCalls (packages/jest-console/src/index.js:34:4)

I'm seeing one as well locally, though different:

 FAIL  test/e2e/specs/demo-content-with-keyboard.test.js (56.141s)
  ● Demo content post › should be created with keyboard

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 503 (Backend.max_conn reached)"]

      32 | 	function assertExpectedCalls() {
      33 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 | 			expect( console ).not[ matcherName ]();
         | 			^
      35 | 		}
      36 | 	}
      37 | 

      at Object.assertExpectedCalls (packages/jest-console/src/index.js:34:4)

We might need to do more / generalized request stubbing (at least to third-party resources) as in #9924.

await page.keyboard.type( 'https://vimeo.com/22439234' );
await page.keyboard.press( 'Enter' );

await moveMouse(); // This shouldn't be necessary to show the toolbar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #10699

@mtias mtias added this to the 4.1 - UI freeze milestone Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants