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

Fix: When adding a button block, Button receives focus first #15859 #15951

Merged
merged 4 commits into from Jun 6, 2019

Conversation

@senadir
Copy link
Contributor

commented Jun 1, 2019

Description

This pull request fixes issue #15859
When adding a button block, the URL Input field receives focus first, even though it's the second editable text field displayed to users.
this pull fixes it by adding autoFocus={ false } to URLInput component inside the Button/edit.js

How has this been tested?

Testing by creating a button in the editor, have all test pass on local

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@youknowriad
Copy link
Contributor

left a comment

This makes sense to me.

Ideally, we'd explore adding an e2e test and maybe change the default value in URLInput. I can't think of any reason why it should be autofocused by default.

@senadir senadir requested review from aduth, nerrad and ntwb as code owners Jun 3, 2019

@senadir

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I added a simple e2e cases to test basic usage, someone might want to expand on it later, as for changing URLInput default value, I think a discussion should be made about it, ideally here or on the slack channel

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

This failing test is concerning:

https://travis-ci.com/WordPress/gutenberg/jobs/204942638#L892

AIL  packages/e2e-tests/specs/blocks/button.test.js (6.302s)
  ● Button › can jump focus back & forth
    expect(received).toMatchSnapshot()
    Snapshot name: `Button can jump focus back & forth 1`
    - Snapshot
    + Received
      "<!-- wp:button -->
    - <div class="wp-block-button"><a class="wp-block-button__link" href="https://wordpress.org">WordPress.org</a></div>
    + <div class="wp-block-button"><a class="wp-block-button__link" href="https://wordpress.org">.orgWordPress</a></div>
      <!-- /wp:button -->"
      30 | 		await page.keyboard.type( '.org' );
      31 | 
    > 32 | 		expect( await getEditedPostContent() ).toMatchSnapshot();
         | 		                                       ^
      33 | 	} );
      34 | } );
      35 | 
@senadir

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

it seems to be a problem with travis timing since it starts typing before doing await pressKeyWithModifier( 'alt', 'ArrowRight' );
running the test locally produced the correct result, so I might delete the last three lines from the test

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@gziolo I think it's probably the same issue we have in other tests (related to RichText timing issues). @senadir maybe we can avoid the focus back and forth for now to avoid unstable tests.

@youknowriad youknowriad merged commit dfed30a into WordPress:master Jun 6, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Congrats for the first contribution @senadir

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.