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

Block Editor: Change default URLInput autoFocus to false #19973

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 30, 2020

Related #18061
Closes #22440
Blocked by: #19462

This pull request seeks to change the default behavior of URLInput to avoid assigning auto-focus to its rendered input. Auto-focus is largely discouraged (we even enforce a rule) and it should not be the default behavior for such a basic component, as it limits (complicates) reusability and makes unnecessary assumptions about its rendering context. It's assumed that this was the default largely based on the assumption that the input is typically rendered within a Popover, which is not something we should be assuming.

Note: Technically this can be considered a breaking change. That being said, if the component is used within a Popover (as was often assumed), the behavior should be unimpacted, since the default behavior of Popover's focusOnMount prop is to focus the first focusable element, which would be the URLInput if one is present.

Status: This pull request is considered to be blocked by #19462, mostly because the interactions involved with Cmd+K are such that Popover's default focusOnMount isn't strictly enough to work with the specific rendering behavior of the current inline link dialog.

Testing Instructions:

Verify that URL inputs are auto-focused as you would expect them to be in popovers:

  1. Navigate to Posts > Add New
  2. Insert a paragraph block
  3. Add some text
  4. (Optional) Highlight some text
  5. Click "Add Link" in the block toolbar
  6. Note that link popover is shown and the input is focused

Follow-up tasks:

Should URLInputButton be deprecated? Or at least undocumented. I don't believe that it's used in any core blocks, and I question whether we should want to encourage it as a pattern.

@aduth aduth added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor labels Jan 30, 2020
@aduth aduth added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jan 30, 2020
@aduth aduth force-pushed the update/url-input-no-default-autofocus branch from ce1c776 to e69aed5 Compare January 31, 2020 19:38
@aduth aduth removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Apr 3, 2020
@aduth aduth force-pushed the update/url-input-no-default-autofocus branch from e69aed5 to 0936dcf Compare April 3, 2020 16:16
@aduth
Copy link
Member Author

aduth commented Apr 3, 2020

Rebased to resolve conflicts. Removed "In Progress" label. This is awaiting review.

@github-actions
Copy link

github-actions bot commented Apr 3, 2020

Size Change: +24 B (0%)

Total Size: 904 kB

Filename Size Change
build/block-editor/index.js 104 kB +24 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.43 kB 0 B
build/block-library/style.css 7.44 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.1 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 93.5 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.4 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.53 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.6 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.29 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aduth
Copy link
Member Author

aduth commented Apr 3, 2020

There are (probably legitimate) test errors now. I'll take a look on Monday.

@aduth
Copy link
Member Author

aduth commented Apr 8, 2020

I gave a quick manual test to this branch. I see that the issue which is likely causing the test failures is related to the fact that when clicking "Edit" when previewing a link, the input field isn't automatically focused.

It's been some time since I originally put the changes together here, but based on the changes applied to URLInputButton in toggling a button to trigger focus to shift into the input, I expect the same treatment could/should be applied to the LinkControl toggle as well.

@aduth aduth force-pushed the update/url-input-no-default-autofocus branch from 0936dcf to 9045b67 Compare April 9, 2020 18:53
@aduth
Copy link
Member Author

aduth commented Apr 9, 2020

I gave a quick manual test to this branch. I see that the issue which is likely causing the test failures is related to the fact that when clicking "Edit" when previewing a link, the input field isn't automatically focused.

This should hopefully be fixed now as of the changes in 9045b67 . This was able to lean on some of the component's own prior art, where focus is retained when completing the selection of a link. The revised implementation essentially applies this behavior for any toggle, either the start or completion of an edit state.

Note that this required a few extra considerations:

  1. Retaining focus in response to clicking the "Edit" button to this very commonly-encountered browser inconsistency of button clicks and focus. This was worked around by creating separate handling specific to changes in state from button clicks, where it is assumed that it should be treated as effectively the same as if focus was within the element.
  2. @wordpress/dom focusable utilities do not work correctly in our JSDOM unit test environment, because they rely on layout dimensions of an element to determine if an element is a focus candidate. These dimensions are not implemented by JSDOM, and it's quite likely they never will be. This has come up on a few occasions in the past. Ideally we solve this globally with some global mock or revised implementation of the utilities. In the meantime, I've resolved this by overriding the Element prototype within the test to simulate as if the element would have some non-zero dimensions.

@aduth aduth added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 9, 2020
@aduth
Copy link
Member Author

aduth commented Apr 9, 2020

I added the "Needs Dev Note" label since this is technically a breaking change for existing usage of the URLInput component. The included CHANGELOG.md entry could serve well as the note text:

The default URLInput autoFocus prop value has changed from true to false. If you relied on the auto-focus behavior of the input, you must explicitly assign an autoFocus={ true } prop. Note that (in accordance with the purpose of this change) it is usually inadvisable to auto-focus an input. Refer to the component README for more information.

@aduth
Copy link
Member Author

aduth commented Apr 9, 2020

The stubs appear to be taking effect in other tests unintentionally. I'll have to take another look. It might just need to be complemented by an afterAll tear-down, but given how the setup is mutating the prototype directly, not sure how feasible this is without other changes.

Or it might just be worth looking at something on the @wordpress/dom side of things. I had some luck getting a global mock earlier, but it wasn't working completely. Might be easier / better to try to sort out those remaining issues instead.

@ellatrix ellatrix mentioned this pull request Jul 3, 2020
12 tasks
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jul 29, 2020
@youknowriad
Copy link
Contributor

This has been implemented in a separate PR. Thanks Andrew

@youknowriad youknowriad deleted the update/url-input-no-default-autofocus branch December 24, 2020 22:24
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). [Package] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Editor: URLInput: Remove autoFocus as a default prop of URLInput
2 participants