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

Add editor heading for screenreaders #3937

Merged
merged 5 commits into from Dec 22, 2017

Conversation

@jasmussen
Contributor

jasmussen commented Dec 12, 2017

This is round 2 of #3801.

It mitigates/fixes #503 and fixes #2024.

What remains to be done in this branch is:

  • Make sure the text is translatable, right now it's hardcoded to "Edit Post"
  • Make sure the text is contextual (so it shows Edit Page, Edit CPT etc)
  • Consider whether we show "Add New Post" on a fresh post, and change that to "Edit Post" when returning
Add editor heading for screenreaders
This is round 2 of #3801.

It mitigates/fixes #503 and fixes #2024.

What remains to be done in this branch is:

- Make sure the text is translatable, right now it's hardcoded to "Edit Post"
- Make sure the text is contextual (so it shows Edit Page, Edit CPT etc)
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 12, 2017

Contributor

@afercia in gutenberg.php, there's the following:

/**
 * Project.
 *
 * The main entry point for the Gutenberg editor. Renders the editor on the
 * wp-admin page for the plugin.
 *
 * @todo Remove the temporary fix for the NVDA screen reader and use meaningful
 *       content instead. See pull at and issues #467 and #503.
 *
 * @since 0.1.0
 */
function the_gutenberg_project() {
	?>
	<div class="nvda-temp-fix screen-reader-text">&nbsp;</div>
	<div class="gutenberg">
		<h1 class="screen-reader-text">Edit Post</h1>
		<div id="editor" class="gutenberg__editor"></div>
		<div id="metaboxes" style="display: none;">
			<?php the_gutenberg_metaboxes(); ?>
		</div>
	</div>
	<?php
}

Given that there's now a h1 inside the gutenberg div, should the nvda temp div be removed?

Contributor

jasmussen commented Dec 12, 2017

@afercia in gutenberg.php, there's the following:

/**
 * Project.
 *
 * The main entry point for the Gutenberg editor. Renders the editor on the
 * wp-admin page for the plugin.
 *
 * @todo Remove the temporary fix for the NVDA screen reader and use meaningful
 *       content instead. See pull at and issues #467 and #503.
 *
 * @since 0.1.0
 */
function the_gutenberg_project() {
	?>
	<div class="nvda-temp-fix screen-reader-text">&nbsp;</div>
	<div class="gutenberg">
		<h1 class="screen-reader-text">Edit Post</h1>
		<div id="editor" class="gutenberg__editor"></div>
		<div id="metaboxes" style="display: none;">
			<?php the_gutenberg_metaboxes(); ?>
		</div>
	</div>
	<?php
}

Given that there's now a h1 inside the gutenberg div, should the nvda temp div be removed?

Make post title contextual and translatable
This just uses the stock WordPress objects.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 12, 2017

Contributor

@gziolo in e682f1e I adopted/adapted some code to output a translated and contextual label for the heading. It seems to work well, insofar as it outputs "Edit Post" and "Edit Page" when it needs to. I haven't tested with custom post types, but I'm assuming since it taps into the same function, that it should work.

Can you help me with this? I need a sanity check my PHP, it's never been my forte. I'd also love some help testing with custom post types.

Finally, the $post_type_object->labels has entries for both Edit Post as well as Add New Post, but the logic to define when to output one vs. the other escapes me. Any advice here? Thanks a bunch.

Contributor

jasmussen commented Dec 12, 2017

@gziolo in e682f1e I adopted/adapted some code to output a translated and contextual label for the heading. It seems to work well, insofar as it outputs "Edit Post" and "Edit Page" when it needs to. I haven't tested with custom post types, but I'm assuming since it taps into the same function, that it should work.

Can you help me with this? I need a sanity check my PHP, it's never been my forte. I'd also love some help testing with custom post types.

Finally, the $post_type_object->labels has entries for both Edit Post as well as Add New Post, but the logic to define when to output one vs. the other escapes me. Any advice here? Thanks a bunch.

Show outdated Hide outdated gutenberg.php Outdated
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 12, 2017

Member

I tested it with the page and post types. It works as advertised. Let's confirm PHP bits before we merge it.

Member

gziolo commented Dec 12, 2017

I tested it with the page and post types. It works as advertised. Let's confirm PHP bits before we merge it.

@gziolo

gziolo approved these changes Dec 12, 2017

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 12, 2017

Contributor

Given that there's now a h1 inside the gutenberg div, should the nvda temp div be removed?

Yes! We can get rid of that ugly hack 🙂

Contributor

afercia commented Dec 12, 2017

Given that there's now a h1 inside the gutenberg div, should the nvda temp div be removed?

Yes! We can get rid of that ugly hack 🙂

@jasmussen jasmussen requested review from aduth, youknowriad and pento Dec 20, 2017

@pento

One minor change, everything else looks good.

Show outdated Hide outdated gutenberg.php Outdated
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 22, 2017

Contributor

Thanks a bunch for the review! I pushed the change.

If the change was correctly interpreted, and this is good to merge, feel free to do so. I'm heading out for some holiday today! ❤️

Contributor

jasmussen commented Dec 22, 2017

Thanks a bunch for the review! I pushed the change.

If the change was correctly interpreted, and this is good to merge, feel free to do so. I'm heading out for some holiday today! ❤️

Show outdated Hide outdated gutenberg.php Outdated
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 22, 2017

Member

I will give it a spin and merge today :)

Member

gziolo commented Dec 22, 2017

I will give it a spin and merge today :)

@pento

pento approved these changes Dec 22, 2017

👍🏻 after the change @gziolo mentioned.

@gziolo gziolo merged commit f023754 into master Dec 22, 2017

3 checks passed

codecov/project 41.94% (+3.7%) compared to 5648a77
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the try/edit-heading-round-2 branch Dec 22, 2017

@@ -110,7 +110,7 @@ class PostTitle extends Component {
tabIndex={ -1 /* Necessary for Firefox to include relatedTarget in blur event */ }
>
{ isSelected && <PostPermalink /> }
<h1>
<div>

This comment has been minimized.

@mtias

mtias Jan 11, 2018

Contributor

@jasmussen was this div needed?

@mtias

mtias Jan 11, 2018

Contributor

@jasmussen was this div needed?

This comment has been minimized.

@jasmussen

jasmussen Jan 11, 2018

Contributor

It was needed in order to let this "pseudo" block inherit styles that look exactly like our other blocks, without writing completely different CSS.

I did try to apply the on-select border to the textarea itself, or pseudo selectors, but it added a ton of complexity to the CSS, and I don't think I was able to get the responsive behavior to be the same. I decided it wasn't worth a complete rewrite to save this div, and I'm still unconvinced it can be done.

@jasmussen

jasmussen Jan 11, 2018

Contributor

It was needed in order to let this "pseudo" block inherit styles that look exactly like our other blocks, without writing completely different CSS.

I did try to apply the on-select border to the textarea itself, or pseudo selectors, but it added a ton of complexity to the CSS, and I don't think I was able to get the responsive behavior to be the same. I decided it wasn't worth a complete rewrite to save this div, and I'm still unconvinced it can be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment