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

[Enhancement] Making the focus state of button text in Button block more visible #15058

Merged
merged 5 commits into from Apr 26, 2019

Conversation

@nfmohit-wpmudev
Copy link
Member

commented Apr 18, 2019

Description

This PR tries to close #15051 which reports the focus style of the button text within the button block not being clear.

The placeholder on this text input is essentially an overlay. It has a reduced opacity set so that the cursor shows up. But naturally, it barely does. This PR just reduces the opacity further only when the input is focused so that the cursor, i.e. the focus state, is more visible.

I don't think it is the most ideal approach to this, but at least it's a start. I'd request some feedback and suggestions on how we could improve it. Thanks ❤️

How has this been tested?

This PR has been tested by going through the following steps:

  1. Started a new post using the Gutenberg editor.
  2. Added the "Button" block.
  3. Focused on the button text input.
  4. Made sure the opacity of the placeholder reduces making the cursor more visible.

Screenshots

15051

Types of changes

This PR just simply reduces the opacity of the placeholder on :focus state of the text input.

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.
@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Thank you, @nfmohit-wpmudev! This is a definite improvement. The previous layering of these elements is definitely a bug in retrospect.

Upon trying this out, I wonder if we can go a little bit further with two changes before merging this:

  1. Other placeholder text we have disappears completely on select. I think we should adopt that here:

typing

  1. It's confusing that the cursor is in the center of the block here, since that's not actually where the text starts when you type. Can we move the cursor to the beginning when there's no text?

Here's what that would work out to:

button-typing-new

Thank you for taking this on so quickly.

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Thank you so very much for your feedback @kjellr ❤️ I have pushed some commits that address your suggestions.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Thanks, @nfmohit-wpmudev! This is looking excellent. I found one small bug — not sure if we can fix this or not.

  1. Add a Button block
  2. Select the button text field.
  3. Change the alignment of the button.
  4. Notice that the cursor moves to the center of the button again, and the placeholder text is visible.

button

It'd be great if we could keep adopt the hidden placeholder text + left-aligned cursor in that case, since the cursor is still active.

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Thank you very much for the quick review @kjellr ❤️

Sincere apologies if I'm missing something but the cursor doesn't stay active for me when I change the alignment of the button. I think that maybe because the focus state on the button is lost when I click elsewhere. Here's a screencast:
15058-2

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

There's one other thing I could do - remove the keepPlaceholderOnFocus attribute from the RichText component so that after focus, the placeholder is removed completely. I'm not sure if this is an appropriate behavior but here's how it'd look:

15058-3

Let me know if you like it. We can even try setting a minimum width to the button in this specific scenario so that it doesn't stay shrank like that if you want.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Sincere apologies if I'm missing something but the cursor doesn't stay active for me when I change the alignment of the button. I think that maybe because the focus state on the button is lost when I click elsewhere.

Interesting! This only seems to happen in Safari now that I'm testing more widely. Chrome and Firefox lose focus when I click a new alignment. 🤔 I'll do a little digging tomorrow and see if I can come up with a decent solution.

@kjellr

kjellr approved these changes Apr 25, 2019

Copy link
Contributor

left a comment

This looks good on my end! The placeholder now behaves like I'd expect it to. Here's a GIF:

animation

I'd like to just get one last accessibility review before merge. @afercia, since does this seem like a solid improvement from your end? Thank you!

@afercia

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Fixing the caret is a good start, thank you! However, in the editing context the button is equivalent to an input field: it should have aso an additional focus indicator, for example an outline, like all the input fields.

@kjellr
Copy link
Contributor

left a comment

Thanks, @afercia!

@nfmohit-wpmudev, let's add a border like this so it has a more visible focus state:

focus-state

Something like this should take care of it:

// Add a blue border to indicate focus. 
.block-editor-rich-text__editable:focus {
	box-shadow: 0 0 0 1px $white, 0 0 0 3px $blue-medium-500;

	// Windows High Contrast mode will show this outline, but not the box-shadow.
	outline: 2px solid transparent;
	outline-offset: -2px;
}

For now, we'll also need to move the .block-library-button__inline-link down by 2px in order to make sure this border doesn't overlap the URL field. That'll eventually be unnecessary once #10128 is merged in.

Once that's in place, this should be good to 🚢!

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Thank you for your feedback @afercia and thank you so very much for your suggestions @kjellr ❤️ I have addressed them in this commit.

@kjellr

kjellr approved these changes Apr 25, 2019

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Looks great, thanks @nfmohit-wpmudev. Should be good to merge after the tests pass.

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

Thank you very much @kjellr ❤️

@kjellr kjellr merged commit 20582e5 into WordPress:master Apr 26, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019

@nfmohit-wpmudev nfmohit-wpmudev deleted the nfmohit-wpmudev:update/button-focus-style/15051 branch Jul 18, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

[Enhancement] Making the focus state of button text in Button block m…
…ore visible (WordPress#15058)

* Reduce placeholder opacity on focus to make cursor visible

* Make placeholder disappear on focus to make focus-state visible

* Align cursor to left when text input is focused and placeholder is active

* Left align text input when placeholder is active at all times, not just on focus

* Added focus outline on focus state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.