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

Focus preview button after opening preview. #20570

Merged
merged 4 commits into from Mar 6, 2020

Conversation

@tellthemachines
Copy link
Contributor

tellthemachines commented Mar 2, 2020

Description

Fixes #20404 .

When opening preview externally and returning to the editor tab, the dropdown loses focus, so it doesn't close when clicked outside. This change sets focus back on the preview button after opening the external preview.

How has this been tested?

Tested in browser; ran unit tests and updated selectors for e2e tests.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 2, 2020

Size Change: -428 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-editor/index.js 105 kB +131 B (0%)
build/block-editor/style-rtl.css 10.5 kB -24 B (0%)
build/block-editor/style.css 10.5 kB -23 B (0%)
build/block-library/editor-rtl.css 7.36 kB -304 B (4%)
build/block-library/editor.css 7.36 kB -305 B (4%)
build/block-library/index.js 116 kB +17 B (0%)
build/block-library/style-rtl.css 7.5 kB +14 B (0%)
build/block-library/style.css 7.51 kB +14 B (0%)
build/components/style-rtl.css 15.6 kB +36 B (0%)
build/components/style.css 15.5 kB +38 B (0%)
build/edit-post/index.js 90.9 kB +4 B (0%)
build/edit-post/style-rtl.css 8.53 kB -13 B (0%)
build/edit-post/style.css 8.53 kB -12 B (0%)
build/editor/index.js 44.7 kB +17 B (0%)
build/editor/style-rtl.css 3.98 kB -25 B (0%)
build/editor/style.css 3.98 kB -25 B (0%)
build/rich-text/index.js 14.3 kB +32 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 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.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 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 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 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 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 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.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@@ -144,6 +146,7 @@ describe( 'Preview', () => {
// Pressing preview without changes should bring same preview window to
// front and reload, but should not show interstitial.
await editorPage.bringToFront();
await editorPage.click( '.editor-post-preview__button-toggle' );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 2, 2020

Contributor

Something I don't understand here.

We click on "editor-post-preview__button-toggle" here and then when calling waitForPreviewNavigation. Do we need to click the button twice?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 3, 2020

Author Contributor

When navigating back from the preview to the editor, the preview options dropdown is still open, so when waitForPreviewNavigation clicks on it it closes instead of opening. The alternative to doing it this way would be not opening the dropdown inside waitForPreviewNavigation and instead opening it beforehand if it's not already open. Perhaps it will be less confusing that way.

This comment has been minimized.

Copy link
@talldan

talldan Mar 3, 2020

Contributor

waitForPreviewNavigation could also check if the dropdown is already open and only click the toggle when it needs to.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 3, 2020

Author Contributor

I thought about that at first, but is it good practice to have conditionals in tests? It always feels a bit wrong to me 😅 and it would probably make the test slower because we'd have to wait to see if the selector was found.
Happy to do it if everyone agrees though!

This comment has been minimized.

Copy link
@talldan

talldan Mar 3, 2020

Contributor

I suppose it makes that part of the test no longer deterministic. Maybe the easiest thing would be to break waitForPreviewNavigation into two functions in that case.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 3, 2020

Contributor

I don't mind either conditions or breaking into two functions. Other than that, the changes here look good to me.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 4, 2020

Author Contributor

Done!

@talldan
talldan approved these changes Mar 6, 2020
Copy link
Contributor

talldan left a comment

This definitely seems to solve the problem and it's great that there are tests too to catch any potential further issues. 🎉

As an aside, I just noticed this issue too - #20677, but it's in master as well so shouldn't hold up this PR.

@talldan talldan merged commit aecf3a5 into master Mar 6, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@talldan talldan deleted the fix/external-preview-focus-return branch Mar 6, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.