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

Improve color contrast of the default block inserter shortcuts. #12928

Merged

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Dec 16, 2018

As mentioned in #10519 (comment) the default appender shortcuts icons and the default block shortcuts icons are styled differently.

In the default appender, the shortcuts do have a sufficient color contrast:

default appender

However, when the default appender is clicked, it becomes the "default block" and the icons don't have a sufficient color contrast:

default block

This small PR seeks to improve the contrast and simplify the CSS:

  • removes specific styling from the default-block-appender
  • uses the colors removed from the default-block-appender directly into the inserter-with-shortcuts, as part of its default styling

Worth noting there are also some CSS rules related to the opacity transition that don't work as expected after #12510. I guess this should be addressed separately together with the refactoring proposed in #10519.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Dec 16, 2018
@jasmussen
Copy link
Contributor

Thanks for the PR and ping. Given that we mean to retire those, is it actually worth making this change? The question here is not one of whether one is better than the other, rather: which WordPress release will this ship with? If it's 5.1 then I would be disappointed if we couldn't fix the problems with this interface at the root, in #10519.

So expanding this for a wider review from the group.

@jasmussen jasmussen requested a review from a team December 17, 2018 09:24
@afercia
Copy link
Contributor Author

afercia commented Dec 17, 2018

Yup, totally agree it depends on when the refactoring will happen. If it's going to happen soon, in the next release, I'd agree to just close this PR. Otherwise, I'd like to temporarily fix the contrast issue. Thank you.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Personally I too would like to see these retired. Leaving them with bad contrast on focus isn't a friendly message to users who have trouble with low contrast, but realistically I don't want them to be visible at all so I'd say they're getting a better deal 😉

I think making them more obvious here isn't a good stopgap because it's a worse visual UX for users who can already make out the icons. I know that's not a great answer but I think it's better to leave them as-is until we remove. If someone made a PR that just removed them though I'd approve it ASAP 😄

@afercia
Copy link
Contributor Author

afercia commented Jan 13, 2019

Looks like #10519 / #11329 won't make it for 5.1. Can we improve this color contrast in the meantime? /Cc @jasmussen @tofumatt

@afercia afercia force-pushed the update/default-block-inserter-shortcuts-color-contrast branch from 1b896cf to 9ea2adf Compare January 13, 2019 12:26
@jasmussen
Copy link
Contributor

Can we improve this color contrast in the meantime?

Yes, I think we should. Because current master isn't great. Here's 5.0.3:

5 0 3

Notice how the contrast is sufficient in the initial placeholder, but it is not sufficient in an "empty paragraph placeholder".

Here's this branch:

this branch

So outside of the contrast changes for the empty paragraph, at least this branch also makes it consistent.

However, the fade effect is missing from the left plus on a new post, and it's also missing from the shortcuts on the right. That's not new to this branch specifically, but seems worth fixing while we're at it:

this branch new post

In other words: can we make it so all the shortcut icons in placeholders fade in, whether in the initial appender, or the empty placeholders? If yes, then I think we can ship this as a good interim improvement.

@afercia
Copy link
Contributor Author

afercia commented Jan 14, 2019

However, the fade effect is missing

@tofumatt see my previous comment:

Worth noting there are also some CSS rules related to the opacity transition that don't work as expected after #12510. I guess this should be addressed separately together with the refactoring proposed in #10519.

Those are now rendered only when hovered. So when rendered, they're already hovered and there's no state transition the CSS transition can understand.

@jasmussen
Copy link
Contributor

Those are now rendered only when hovered. So when rendered, they're already hovered and there's no state transition the CSS transition can understand.

I'm not sure that's true. I believe that animation-fill-mode: forwards; should enable this, and this is also the case for the block mover which doesn't exist in the dom until you move the cursor near them.

But it's not the end of the world to ship this without those improvements, just seems like it might be worth it. Though it's worth noting that the fade effect is missing on the empty paragraph placeholder in this branch, but it is present in master.

@afercia
Copy link
Contributor Author

afercia commented Jan 14, 2019

animation-fill-mode: forwards; applies to keyframe animations. Here, the opacity property uses a CSS transition. We can try to replace it with the keyframe one.

@afercia
Copy link
Contributor Author

afercia commented Jan 14, 2019

Though it's worth noting that the fade effect is missing on the empty paragraph placeholder in this branch, but it is present in master.

Check better 🙂 This PR touches only colors.

@jasmussen
Copy link
Contributor

🤦‍♂️

I blame mondays.

Anyway, no objections to getting this in, as it adds a little consistency. Would be nice if we could restore the fading.

@jasmussen
Copy link
Contributor

CC: @youknowriad

@afercia
Copy link
Contributor Author

afercia commented Jan 14, 2019

Yep, I'm looking into it.

@afercia
Copy link
Contributor Author

afercia commented Jan 14, 2019

Even the most simple things... 🙂 turns out the effects can only work consistently if the elements have the same rendering logic.

Basically when an element is always rendered in the DOM, it can get both fade-in and fade-out effects.

Instead, when an element is added to the DOM, it can get the fade-in effect via the keyframe animation but when removed it's just removed... there's no time for transitions or animations. To have a fed-out effect, either the element should always be rendered or we should detect transitionEnd or animationEnd and then remove it. Meh.

In the last commit:

editor-default-block-appender:

  • the "plus" button is always in the DOM: can have fade-in and fade-out with CSS transition
  • the "shortcuts icons" are rendered / removed: can only have fade-in
  • I've added the fade-in animation on the "shortcuts icons", it was missing

side-inserter and empty-block-inserter:

  • they're always rendered and removed when typing / emptying the block; question: @youknowriad any performance concern here?
  • can only have fade-in
  • I've changed the CSS from transition to animation

Going to check if the side-inserter "plus" can be changed to always be rendered in the DOM.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I didn't think it was worth investing in improving these** as my impression was that they're be removed. I don't think they're a good UX regardless of their accessibility.

I'd rather they just be removed.

That said, seems the inserter UX is changing in #10519 so I'm fine with this as a stop-gap.

@afercia
Copy link
Contributor Author

afercia commented Jan 14, 2019

I'd rather they just be removed.

Looking forward for that to happen but, as commented above, looks like #10519 / #11329 won't make it for 5.1.

I'm fine with this as a stop-gap.

I can't express how this kind of self-referential language ("I") isn't helpful.

@afercia afercia merged commit 16b94bb into master Jan 14, 2019
@afercia afercia deleted the update/default-block-inserter-shortcuts-color-contrast branch January 14, 2019 13:24
@youknowriad youknowriad added this to the 4.9 (Gutenberg) milestone Jan 14, 2019
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Improve color contrast of the default block inserter shortcuts.

* Restore fade in effect.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Improve color contrast of the default block inserter shortcuts.

* Restore fade in effect.
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.

4 participants