PostPublishPanel: return focus to element that opened the panel #11543

oandregal · 2018-11-06T13:35:21Z

Addresses the last bit of #4187 (comment)
~~Depends on #11584~~
Closes #4187

Description

This PR manages where focus goes after the PostPublishPanel is closed. At the moment, it's lost and just goes to the body element.

Changes this PR introduces:

Uses the withReturnFocus HOC to return focus to the element that had it before opening the publish sidebar.
Changes the Header settings not to be unmounted when the publish sidebar is opened. This requires us to hide the "Saving draft" button, because it is not completely hidden by the publish sidebar.
Uses a single component as the Publish button, instead of the two we had. This helps in that the DOM node that opens the publish sidebar isn't changed by a new component. It does so by teaching the PostPublishButton how to behave like a button (publish directly) or a toggle (opens the publish sidebar instead).

Questions

Should we deprecate PostPublishToggle in favor of the PostPublishButton component in this PR We'll go with deprecating it and removing it in 5.1.

oandregal · 2018-11-06T13:41:39Z

This approach gotchas:

we need to hide the "Save draft" button as it isn't hidden completely by the PostPublishPanel.
we need to refactor the publish toggle / button components to use only one element, otherwise, focus will be lost after changing from toggle to button (for ex: using the toggle to open the PostPublishPanel, which will get unmounted once the post is published in favor of the button).

oandregal · 2018-11-08T15:13:09Z

So, against my original concerns, this PR has ended up being a fine approach. It is ready for review. The only thing I wonder is whether we should deprecate the toggle component. Normally, I'd do it, but given the point we're at in the release cycle I'd want to hear more opinions.

tofumatt

I'm unclear on whether #11584 needs to be merged before this will work as intended—is that the case?

Right now after closing the panel focus seems lost and is not returned to the element that opened it (the "Publish" button).

tofumatt · 2018-11-09T01:21:12Z

For what it's worth: if we aren't using the PostPublishToggle component anywhere internally it seems the kind of thing we could deprecate as I'm unsure where else it would be being used…

But then we're past API freeze and should respect that, maybe we deprecate it as normal and remove it in a few versions, recognising that that'll be after WordPress 5.0.

- Do not unmount Header settings components. - Hide the "Save draft" button because it's not hidden by the PostPublishPanel slide-in sidebar.

tofumatt

I'll try this again a bit later today after I'm back from squash, but for now I have a few change requests regarding the deprecations.

Any chance you could record a screencast of this working on your machine? It would help to see what you're seeing 😄

packages/edit-post/src/components/header/index.js

packages/edit-post/src/components/header/post-publish-button-or-toggle.js

tofumatt · 2018-11-09T12:06:15Z

packages/editor/src/components/post-publish-button/index.js

-				<PublishButtonLabel forceIsSaving={ forceIsSaving } />
+				{ componentChildren }
+				<DotTip tipId="core/editor.publish">
+					{ __( 'Finished writing? That’s great, let’s get this published right now. Just click “Publish” and you’re good to go.' ) }


I find "Just" to be a weasel word and would like to remove it, but I know that's a bit out-of-scope for the PR. Just saying' 😆

packages/editor/src/components/post-publish-panel/test/toggle.js

packages/editor/src/components/post-publish-panel/toggle.js

Co-Authored-By: nosolosw <nosolosw@users.noreply.github.com>

oandregal · 2018-11-09T12:36:30Z

@tofumatt Added a GHIF in the description. This is what I've done (the image is not great):

Click the "Publish" button.
Tab and Intro to close the pre-publish panel.
Intro to open the publish panel again.
Intro to publish the post.
Shift+Tab and Intro to close the post-publish panel.

At this point, the Publish button has the focus although it's disabled. Then, I've done a few Tabs and Shift+Tab to move focus among the Header settings components forward and backward to demonstrate it.

tofumatt

I tested this locally and now it's working perfectly. Thanks so much!

I left a few suggestions but once they're addressed 🚢

Thanks so much for this! Once it's merged, all the Gutenberg merge accessibility issues are covered! 🎉

tofumatt · 2018-11-09T15:49:18Z

docs/reference/deprecated.md

@@ -1,5 +1,9 @@
 Gutenberg's deprecation policy is intended to support backwards-compatibility for releases, when possible. The current deprecations are listed below and are grouped by _the version at which they will be removed completely_. If your plugin depends on these behaviors, you must update to the recommended alternative before the noted version.

+## 4.6


Best to keep this consistent with the rest of the file:

Suggested change

## 4.6

## 4.6.0

tofumatt · 2018-11-09T15:55:21Z

packages/edit-post/src/components/header/index.js

-				<div className="edit-post-header__settings">
+			<div className="edit-post-header__settings">
+				{ ! isPublishSidebarOpened && (
+					// This button isn't completely hidden by the publish sidebar.


👍

This is an awesome, super-helpful comment. Ace!

tofumatt · 2018-11-09T15:57:06Z

packages/editor/src/components/post-publish-button/index.js

+		const toggleProps = {
+			'aria-disabled': isToggleDisabled,
+			'aria-expanded': isOpen,
+			className: 'editor-post-publish-panel__toggle',


editor-post-publish-panel__toggle seems like the right name.

EDIT: oops, I meant editor-post-publish-button__toggle

tofumatt

Damn, looks like we shouldn't be deprecating stuff here:

#11679 (comment)

Might be best to remove that for now. Really sorry, I thought deprecations would continue as normal, but seems like the mechanism will be different going forward, so for now maybe we shouldn't.

aduth · 2018-11-09T16:53:07Z

Damn, looks like we shouldn't be deprecating stuff here:

#11679 (comment)

The changes of #11679 are not really relevant to speaking to whether or not a thing can be deprecated still. The changes there are merely a reflection of the fact that the dependency becomes unused once the current deprecations are removed.

tofumatt · 2018-11-09T17:01:26Z

Right, but I thought @youknowriad was saying deprecations after 4.3 we don't know how we'll handle them.

Maybe I'm confused and these deprecations are fine? I'm confused 😅

youknowriad · 2018-11-09T17:05:08Z

Right, but I thought @youknowriad was saying deprecations after 4.3 we don't know how we'll handle them.

Since we said we're in API Freeze, yes I did say that and I think we should be more strict now to keep our engagements. We shouldn't deprecate anything impactful.

I think PostPublishPanelToggle is not going to impact any plugin though because it's something npm consumers will likely use but less WP plugins.

tofumatt · 2018-11-09T17:06:03Z

Fair enough then; changed back to approved 😄

tofumatt · 2018-11-09T17:18:42Z

We're actually going to merge this into 4.3; I'll take it over from here and tweak the deprecations. Thanks again for working on this 👍

aduth · 2019-01-04T16:19:33Z

packages/editor/src/components/post-publish-button/index.js

-				isPrimary
-				isLarge
-				onClick={ onClick }
-				disabled={ isButtonDisabled }


For what reason did we need to stop passing the disabled prop? While this and many other buttons in the header appear as disabled, they don't act as disabled. You can immediately press "Publish" on a new post and the publish panel is (wrongly?) shown.

As it relates to focus return, it's not clear to me how it's relevant, since the panel would have presumably never been opened if it were disabled.

Good catch. #13194 should fix it.

The disabled property didn't play well with the withFocusReturn HOC, IIRC. I can dig up the exact details if necessary.

oandregal self-assigned this Nov 6, 2018

oandregal added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 6, 2018

oandregal mentioned this pull request Nov 7, 2018

Publishing Flow accessibility #4187

Closed

oandregal added the [Status] In Progress Tracking issues with work in progress label Nov 7, 2018

oandregal force-pushed the update/focus-after-post-publish branch 3 times, most recently from ce47e64 to 5067de3 Compare November 8, 2018 13:33

oandregal changed the base branch from master to fix/disable-publish November 8, 2018 13:34

oandregal force-pushed the fix/disable-publish branch from 0c91e4b to 4075b57 Compare November 8, 2018 13:35

oandregal force-pushed the update/focus-after-post-publish branch from 5067de3 to 0f99f9d Compare November 8, 2018 13:35

oandregal added this to the 4.4 milestone Nov 8, 2018

oandregal force-pushed the update/focus-after-post-publish branch from 0f99f9d to 4f9a323 Compare November 8, 2018 15:10

oandregal removed the [Status] In Progress Tracking issues with work in progress label Nov 8, 2018

oandregal requested review from a team and tofumatt November 8, 2018 15:13

tofumatt reviewed Nov 9, 2018

View reviewed changes

oandregal added 11 commits November 9, 2018 11:01

Toggle disabled if post is published

3044063

Toggle should be disabled if post is published

6cca773

Return focus to Publish... button

4bfb697

- Do not unmount Header settings components. - Hide the "Save draft" button because it's not hidden by the PostPublishPanel slide-in sidebar.

Sort dependencies

0fac6ef

Extract props

3155c59

Add toggleProps to button

9228b68

Inline text for toggle within the button component

557eca0

Always render the button component

4bbcac4

Use aria-disabled instead of disabled to avoid focus loss

22a421f

Improve comments

35de337

Update snapshots

c2b1a9f

tofumatt requested changes Nov 9, 2018

View reviewed changes

Update version

48439c0

oandregal commented Nov 9, 2018

View reviewed changes

packages/editor/src/components/post-publish-panel/toggle.js Outdated Show resolved Hide resolved

nosolosw and others added 5 commits November 9, 2018 13:09

Update packages/editor/src/components/post-publish-panel/toggle.js

9341979

Place component in a separate line

0ef0206

Co-Authored-By: nosolosw <nosolosw@users.noreply.github.com>

Update version number

94b251f

Add comment

8f50b19

Update comment

ca93215

Tweak comment

99433b4

tofumatt approved these changes Nov 9, 2018

View reviewed changes

tofumatt mentioned this pull request Nov 9, 2018

Remove 4.3 deprecated APIs #11679

Merged

tofumatt requested changes Nov 9, 2018

View reviewed changes

tofumatt approved these changes Nov 9, 2018

View reviewed changes

youknowriad modified the milestones: 4.4, 4.3 Nov 9, 2018

chore: Move deps to 4.5

8b0d6bb

youknowriad merged commit fc03492 into master Nov 9, 2018

youknowriad deleted the update/focus-after-post-publish branch November 9, 2018 18:00

adamsilverstein mentioned this pull request Dec 14, 2018

Exit early from onClick handler when isButtonDisabled #12885

Merged

5 tasks

Copons mentioned this pull request Dec 19, 2018

Gutenlypso: Disable the Publish button if the email is unverified Automattic/wp-calypso#29594

Merged

mmtr mentioned this pull request Dec 19, 2018

Disabled Publish button is clickable #13014

Closed

aduth reviewed Jan 4, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PostPublishPanel: return focus to element that opened the panel #11543

PostPublishPanel: return focus to element that opened the panel #11543

oandregal commented Nov 6, 2018 •

edited by tofumatt

Loading

oandregal commented Nov 6, 2018

oandregal commented Nov 8, 2018

tofumatt left a comment

tofumatt commented Nov 9, 2018

tofumatt left a comment

tofumatt Nov 9, 2018

oandregal commented Nov 9, 2018 •

edited

Loading

tofumatt left a comment

tofumatt Nov 9, 2018

tofumatt Nov 9, 2018

tofumatt Nov 9, 2018 •

edited

Loading

tofumatt left a comment

aduth commented Nov 9, 2018

tofumatt commented Nov 9, 2018

youknowriad commented Nov 9, 2018

tofumatt commented Nov 9, 2018

tofumatt commented Nov 9, 2018

aduth Jan 4, 2019

oandregal Jan 4, 2019

		@@ -1,5 +1,9 @@
		Gutenberg's deprecation policy is intended to support backwards-compatibility for releases, when possible. The current deprecations are listed below and are grouped by _the version at which they will be removed completely_. If your plugin depends on these behaviors, you must update to the recommended alternative before the noted version.

		## 4.6

PostPublishPanel: return focus to element that opened the panel #11543

PostPublishPanel: return focus to element that opened the panel #11543

Conversation

oandregal commented Nov 6, 2018 • edited by tofumatt Loading

Description

Questions

oandregal commented Nov 6, 2018

oandregal commented Nov 8, 2018

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt commented Nov 9, 2018

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Nov 9, 2018

Choose a reason for hiding this comment

oandregal commented Nov 9, 2018 • edited Loading

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Nov 9, 2018

Choose a reason for hiding this comment

tofumatt Nov 9, 2018

Choose a reason for hiding this comment

tofumatt Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

aduth commented Nov 9, 2018

tofumatt commented Nov 9, 2018

youknowriad commented Nov 9, 2018

tofumatt commented Nov 9, 2018

tofumatt commented Nov 9, 2018

aduth Jan 4, 2019

Choose a reason for hiding this comment

oandregal Jan 4, 2019

Choose a reason for hiding this comment

oandregal commented Nov 6, 2018 •

edited by tofumatt

Loading

oandregal commented Nov 9, 2018 •

edited

Loading

tofumatt Nov 9, 2018 •

edited

Loading