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

Post visibility better html and a11y #1361

Merged
merged 9 commits into from
Jul 13, 2017
Merged

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jun 22, 2017

This PR tries to improve the Post Visibility "popover" HTML semantic and accessibility:

Not sure why setPrivate() uses onSave(), would need some feedback here since that seems to trigger a re-rendering of the "popover". /cc @aduth

Todo:

  • when closing the panel, focus should be moved back to the toggle button
  • pressing Escape should close the panel (and move focus back)
  • when setting visibility to "Private": interaction with the JS confirm: is it really needed?

These points should probably be addressed in #1204 as @aduth mentioned

Note: Needs to be tested in IE/Edge (I suspect the legend element would need some additional styling for those browsers)

Fixes #1312

@mtias mtias added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 22, 2017
@afercia afercia changed the title Post visibility better html a11y Post visibility better html and a11y Jun 22, 2017
@afercia
Copy link
Contributor Author

afercia commented Jun 22, 2017

OK so I've just noticed a (pre-existing) bug:
in Firefox (and maybe other browsers too?) it is not possible to select a different radio button using the down and up arrow keys, note this is the native behavior with radio buttons.
Looks like the radio buttons need a name attribute to make Firefox understand they belong to the same group.

@afercia
Copy link
Contributor Author

afercia commented Jun 22, 2017

Screenshot: Firefox and NVDA correctly announcing the fieldset legend, the count of radio buttons, and the descriptions:

screenshot 106

@afercia afercia requested a review from aduth June 23, 2017 08:33
@aduth
Copy link
Member

aduth commented Jul 12, 2017

Not sure why setPrivate() uses onSave(), would need some feedback here since that seems to trigger a re-rendering of the "popover".

I think this was inspired by Calypso's implementation, which prompts the user to immediately publish the post. It was a workaround to an issue caused by Calypso's "noisy" autosave, where drafts are saved very frequently without user intervention (i.e. don't need to press "Save Draft"). It's an issue because you can't save a draft with "Privately Published" as the status (since private and draft are mutually exclusive statuses).

In wp-admin this is pretty unexpected as well:

  1. Navigate to Posts > Add New
  2. Enter title or content
  3. Set Visibility to "Privately Published"
  4. Press "Save Draft"

Expected: A draft is saved
Actual: The post is published privately

Depending on the direction we take with #1773, we might need something like this with Gutenberg as well. Or: a way to respect that a user wants the post to be published privately in draft meta. Or: autosave excluding status.

@aduth
Copy link
Member

aduth commented Jul 12, 2017

pressing Escape should close the panel (and move focus back)

Should the panel also close when focus leaves (guessing yes)? Example: Press tab while focused in the password input moves to the next Post Settings field, but the panel remains open.

@@ -56,8 +56,8 @@ class PostVisibility extends Component {
const { onUpdateVisibility, onSave } = this.props;

onUpdateVisibility( 'private' );
this.setState( { hasPassword: false } );
Copy link
Member

Choose a reason for hiding this comment

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

Outside the scope of this pull request, but I don't like how we have to manually manage password state. Seems like it should be inferred by the password prop, or at least consolidated to one of the lifecycle methods (e.g. componentWillUpdate). I might plan to tackle this separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree it should be addressed separately.

<div key={ value } className="editor-post-visibility__choice">
<input
type="radio"
name="editor-post-visibility__setting"
Copy link
Member

Choose a reason for hiding this comment

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

It's not likely we'd have more than one of these components on the page, but given that we're injecting instanceId anyways, maybe we should include it in the input name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try.

@afercia
Copy link
Contributor Author

afercia commented Jul 13, 2017

Should the panel also close when focus leaves (guessing yes)? Example: Press tab while focused in the password input moves to the next Post Settings field, but the panel remains open.

Yeah, I guess it should.

Depending on the direction we take with #1773, we might need something like this with Gutenberg as well. Or: a way to respect that a user wants the post to be published privately in draft meta. Or: autosave excluding status.

I understand. I can only say that from an usability and accessibility perspective the JS confirm is a terrible experience 🙂 I'd really like to try a different mechanism here, but it makes sense to postpone this issue, pending decisions on autosave.

@afercia
Copy link
Contributor Author

afercia commented Jul 13, 2017

Added instanceId to the radio buttons name attribute. Will split closing the panel on Escape/onBlur and moving back the focus in a separate issue (I'd need some feedback and don't want to block this PR).

@afercia afercia merged commit af63d4e into master Jul 13, 2017
@afercia afercia deleted the update/post-visibility-html-a11y branch July 13, 2017 16:23
youknowriad pushed a commit that referenced this pull request Jan 17, 2020
…oad-progress

Move MediaUploadPorgress to its own component folder
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).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PostVisibility component: invalid HTML and better accessibility
3 participants