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

create a screen-reader-text label for search block (fix #17351) #17983

Merged
merged 1 commit into from Nov 18, 2019

Conversation

@skorasaurus
Copy link
Contributor

skorasaurus commented Oct 16, 2019

Description

The search block should always have a label in it even if it's not visible.

When a user removes the label from the search block when the user is editing content; there should be still be a label present for the corresponding input field, so a user who is consuming the site with a screen reader or other assistive technology has a contextual clue of what the empty input field is :)

What this addition does is, when the label is deleted, a label named "search" is created but is not visually displayed on the screen because the screen-reader-text class is applied to the label element.

I've also manually tested this when you insert multiple search blocks into the same post; a unique label property is created for each one.

WordPress' Accessibility Labeling guidelines also state that all future code should have a label as well.

How has this been tested?

As mentioned at #17351 ; I've tested this in JAWS 2018 with default settings and chrome; and it functioned as I expected.

If you'd like to personally test yourself:
( pull in my changes )

  1. Create or edit new post
  2. Add a search block
  3. delete the label text above the search input field;
  4. publish post.

(Note: I haven't ran automatic tests on this yet, note to self, I'll do that https://github.com/WordPress/gutenberg/blob/master/docs/contributors/testing-overview.md#php-testing )

It's also my first feature/bug PR on Gutenberg. :)

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.
Copy link
Member

jorgefilipecosta left a comment

Hi @skorasaurus, thank you for your contribution 👍 The PR seems to be in the right direction. There is only one thing I think we should address before the merge, currently, we are using a static label "Search" and I think this label needs to be translatable using __(...)

@skorasaurus

This comment has been minimized.

Copy link
Contributor Author

skorasaurus commented Oct 21, 2019

would that look like the following:?


	} else {
			$label_markup = sprintf(
				'<label for="%s" class="wp-block-search__label screen-reader-text">%s</label>',
				$input_id,
				__(  'Search'  )
			);
	}

@skorasaurus

This comment has been minimized.

Copy link
Contributor Author

skorasaurus commented Nov 6, 2019

note to self to rebase and rename class to VisuallyHidden - ref #18167

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 6, 2019

It looks like this PR will fix #17535
It needs a rebase so I can't tell what it's inside the code but based on comments I can tell that there is a new component introduced VisuallyHidden which you probably should consider if changes are on the block editor's side.

@skorasaurus

This comment has been minimized.

Copy link
Contributor Author

skorasaurus commented Nov 6, 2019

It looks like this PR will fix #17535
It needs a rebase so I can't tell what it's inside the code but based on comments I can tell that there is a new component introduced VisuallyHidden which you probably should consider if changes are on the block editor's side.

@gziolo It is intended to fix #17351 .

I have already rebased this morning; did I not do it correctly? I haven't changed the screen-reader-text to the VisuallyHidden component just yet; as the introduction of the component is a little confusing for me (I'm also very new with React) and I'm trying to figure out how to correctly apply this....

In my naive opinion, since this block seems to me written more in php rather than in JS (compared to other blocks), should I just swap out the with screen-reader-text class with the components-visually-hidden class or should I do something else?

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 6, 2019

I have already rebased this morning; did I not do it correctly?

You should see only your commits on this branch. There are over 130 commits at the moment from various contributors. You can also try to recreate this branch from scratch if that makes it easier.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 6, 2019

I have already rebased this morning; did I not do it correctly?

You should see only your commits on this branch. There are over 130 commits at the moment from various contributors. You can also try to recreate this branch from scratch if that makes it easier.

In my naive opinion, since this block seems to me written more in php rather than in JS (compared to other blocks), should I just swap out the with screen-reader-text class with the components-visually-hidden class or should I do something else?

I can give you a correct answer once this branch is cleaned up, otherwise, I will have to guess. In PHP you can use screen-reader-text which is a well-established one in WordPress core.

@skorasaurus skorasaurus force-pushed the skorasaurus:add/search-block-label branch 2 times, most recently from 4599683 to 7ce65ce Nov 12, 2019
@skorasaurus

This comment has been minimized.

Copy link
Contributor Author

skorasaurus commented Nov 12, 2019

@gziolo, I rebased it and only my commit is now showing in the PR although I am unsure if I did properly (I followed directions from https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request#perform-a-rebase).

I'm still using the screen-reader-text class since I haven't figured out how to replace it with the VisuallyHidden component.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 13, 2019

I'm still using the screen-reader-text class since I haven't figured out how to replace it with the VisuallyHidden component.

You can't replace it in PHP code. It's fine as is.

There are 3 issues reported by PHP linter which needs to be addressed before this PR can be landed:

FILE: ...nt/plugins/gutenberg/packages/block-library/src/search/index.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 31 | ERROR | [x] Whitespace found at end of line
    |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EndLine)
 48 | ERROR | [x] Spaces must be used for mid-line alignment; tabs
    |       |     are not allowed
    |       |     (WordPress.WhiteSpace.DisallowInlineTabs.NonIndentTabsUsed)
 48 | ERROR | [x] Whitespace found at end of line
    |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EndLine)

It needs to be tested. The code itself looks good. Good job! 💯

… by default

fix php spacing in block file
@skorasaurus skorasaurus force-pushed the skorasaurus:add/search-block-label branch from 7ce65ce to c415213 Nov 13, 2019
@skorasaurus

This comment has been minimized.

Copy link
Contributor Author

skorasaurus commented Nov 15, 2019

I'm running ubuntu 18.04 and have been doing my development with vvv.

I ran phpcs --standard=WordPress packages/block-library/src/search/index.php and am using PHP_CodeSniffer version 3.5.2 (stable) by Squiz (http://www.squiz.net)
and a clone version of wpcs from earlier this week.

I fixed the spacing issues and tab issues that you mention in a squashed commit .

But there is one more error that remains:
18 | ERROR | Increment and decrement operators must be bracketed when used in string concatenati

Although I did not touch that, does that need to be fixed?

Lastly,
The php test instructions are written for docker users; are there additional tests that I need to run besides what I did above? If so, where can I find out more information about it with my setup?

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 18, 2019

Travis is green so it looks like you were using more strict rules for linting PHP code. This is what is executed for PHP code:

- npm run test-php && npm run test-unit-php-multisite

In other words, this PR seems to be good to go.

@gziolo
gziolo approved these changes Nov 18, 2019
Copy link
Member

gziolo left a comment

Nice work, I can confirm it works as expected when no label is provided 👍

@gziolo gziolo merged commit a661b27 into WordPress:master Nov 18, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@skorasaurus

This comment has been minimized.

Copy link
Contributor Author

skorasaurus commented Nov 18, 2019

Thank you for the assistance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.