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

Fix placeholder contrast. #9562

Merged
merged 2 commits into from Sep 14, 2018

Conversation

@jasmussen
Contributor

jasmussen commented Sep 3, 2018

This fixes #9502 and fixes #9532.

Screenshots:

3-1 title 4 5-1 prompt

Title is large text and needs 3:1 contrast. Placeholder is small text and needs 4.5:1.

screen shot 2018-09-03 at 10 10 46

screen shot 2018-09-03 at 10 12 09

screen shot 2018-09-03 at 10 14 17

screen shot 2018-09-03 at 10 14 35

@jasmussen jasmussen added this to the 3.8 milestone Sep 3, 2018

@jasmussen jasmussen self-assigned this Sep 3, 2018

@jasmussen jasmussen requested review from afercia and WordPress/gutenberg-core Sep 3, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 3, 2018

Contributor

I'm well aware of the a11y guidelines but for me, it's too dark. There's no distinguishable difference between a placeholder and a regular text. (Try adding multiple empty paragraphs).

Differentiating between placeholders and text should also be an a11y guideline IMO.

Contributor

youknowriad commented Sep 3, 2018

I'm well aware of the a11y guidelines but for me, it's too dark. There's no distinguishable difference between a placeholder and a regular text. (Try adding multiple empty paragraphs).

Differentiating between placeholders and text should also be an a11y guideline IMO.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 3, 2018

Contributor

I'm well aware of the a11y guidelines but for me, it's too dark. There's no distinguishable difference between a placeholder and a regular text. (Try adding multiple empty paragraphs).

Fully agree here. The contrast between the text and placeholders in this PR is not significant enough to to differentiate between the two.

If this is indeed the minimum Gray we want to use for placeholders, we should try pushing non-placeholder text to be darker instead. Changing it to 100% black helps just a little bit:

screen shot 2018-09-03 at 11 06 19 am

... but if I'm being honest, I'm not sure the difference there is noticeable enough either.

Contributor

kjellr commented Sep 3, 2018

I'm well aware of the a11y guidelines but for me, it's too dark. There's no distinguishable difference between a placeholder and a regular text. (Try adding multiple empty paragraphs).

Fully agree here. The contrast between the text and placeholders in this PR is not significant enough to to differentiate between the two.

If this is indeed the minimum Gray we want to use for placeholders, we should try pushing non-placeholder text to be darker instead. Changing it to 100% black helps just a little bit:

screen shot 2018-09-03 at 11 06 19 am

... but if I'm being honest, I'm not sure the difference there is noticeable enough either.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 4, 2018

Contributor

There's no distinguishable difference between a placeholder and a regular text

This is one of the many reasons why placeholders are so debated, and one more reason why they should be used sparingly. Unfortunately, design trends tend to use them as replacement for labels, which is completely wrong.

Contributor

afercia commented Sep 4, 2018

There's no distinguishable difference between a placeholder and a regular text

This is one of the many reasons why placeholders are so debated, and one more reason why they should be used sparingly. Unfortunately, design trends tend to use them as replacement for labels, which is completely wrong.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 4, 2018

Contributor

On a side note: now that there's the "Spotlight Mode", from an a11y perspective I wouldn't have objections if you really feel to lighten the placeholder color when Spotlight Mode is on. At one condition though: as said during the Spotlight Mode review, it shouldn't be on by default.

Contributor

afercia commented Sep 4, 2018

On a side note: now that there's the "Spotlight Mode", from an a11y perspective I wouldn't have objections if you really feel to lighten the placeholder color when Spotlight Mode is on. At one condition though: as said during the Spotlight Mode review, it shouldn't be on by default.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 5, 2018

Contributor

Also fixes #8738

Contributor

jasmussen commented Sep 5, 2018

Also fixes #8738

@youknowriad youknowriad modified the milestones: 3.8, 3.9 Sep 5, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 7, 2018

Contributor

Just noticed the File block placeholder would need a darker blue 🙂

screen shot 2018-09-07 at 16 50 15

Contributor

afercia commented Sep 7, 2018

Just noticed the File block placeholder would need a darker blue 🙂

screen shot 2018-09-07 at 16 50 15

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 10, 2018

Contributor

Pushed fix:

screen shot 2018-09-10 at 09 24 01

I decided it was a bit too creative to show blue underlined text as the placeholder, because that implies it's clickable. So now it's placeholder text like any other. It becomes blue once filled out.

Contributor

jasmussen commented Sep 10, 2018

Pushed fix:

screen shot 2018-09-10 at 09 24 01

I decided it was a bit too creative to show blue underlined text as the placeholder, because that implies it's clickable. So now it's placeholder text like any other. It becomes blue once filled out.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 10, 2018

Contributor

Can we get a final review on this one (perhaps approval), so we can merge it in for 3.9?

Contributor

jasmussen commented Sep 10, 2018

Can we get a final review on this one (perhaps approval), so we can merge it in for 3.9?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 10, 2018

Contributor

Rebased, squashed.

Also pushed a fix to make the text darker:

screen shot 2018-09-10 at 15 43 17

This is to ensure some contrast between basic text and placeholder text. It's still not ideal in situations like this:

screen shot 2018-09-10 at 15 43 34

But I feel like that can possibly be addressed separately. For example perhaps we could revisit the decision to show this placeholder text when the block isn't selected. Or put differently, maybe it's okay if an empty paragraph shows up as an empty paragraph until the caret is in it?

Contributor

jasmussen commented Sep 10, 2018

Rebased, squashed.

Also pushed a fix to make the text darker:

screen shot 2018-09-10 at 15 43 17

This is to ensure some contrast between basic text and placeholder text. It's still not ideal in situations like this:

screen shot 2018-09-10 at 15 43 34

But I feel like that can possibly be addressed separately. For example perhaps we could revisit the decision to show this placeholder text when the block isn't selected. Or put differently, maybe it's okay if an empty paragraph shows up as an empty paragraph until the caret is in it?

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 10, 2018

Contributor

Not sure if this fixes also captions displayed on top of images, see #5838 ?

Contributor

afercia commented Sep 10, 2018

Not sure if this fixes also captions displayed on top of images, see #5838 ?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 10, 2018

Contributor

Not sure it does, but if it doesn't I'd like to look at that one separately.

Contributor

jasmussen commented Sep 10, 2018

Not sure it does, but if it doesn't I'd like to look at that one separately.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 10, 2018

Contributor

Looking good. I think the darker text color helps. 👍

Not sure if this fixes also captions displayed on top of images, see #5838 ?

For all captions, this PR seems to eliminate the difference in color between placeholder and entered text:

Image Block, placeholder:
screen shot 2018-09-10 at 10 35 42 am

Image Block, caption:
screen shot 2018-09-10 at 10 35 53 am

Gallery Block, placeholder:
screen shot 2018-09-10 at 10 36 11 am

Gallery Block, caption:
screen shot 2018-09-10 at 10 36 22 am

This does remove that accessibility issue, but I'm not sure if this was intended or not?

Contributor

kjellr commented Sep 10, 2018

Looking good. I think the darker text color helps. 👍

Not sure if this fixes also captions displayed on top of images, see #5838 ?

For all captions, this PR seems to eliminate the difference in color between placeholder and entered text:

Image Block, placeholder:
screen shot 2018-09-10 at 10 35 42 am

Image Block, caption:
screen shot 2018-09-10 at 10 35 53 am

Gallery Block, placeholder:
screen shot 2018-09-10 at 10 36 11 am

Gallery Block, caption:
screen shot 2018-09-10 at 10 36 22 am

This does remove that accessibility issue, but I'm not sure if this was intended or not?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 11, 2018

Contributor

Okay so f04c24d makes the caption slightly darker.

caption color

There's not much difference, but I think it's okay given that caption only appears when you've selected the image first.

Contributor

jasmussen commented Sep 11, 2018

Okay so f04c24d makes the caption slightly darker.

caption color

There's not much difference, but I think it's okay given that caption only appears when you've selected the image first.

@@ -89,9 +89,18 @@
}
}
& + .editor-rich-text__tinymce {
opacity: 0.5;

This comment has been minimized.

@jasmussen

jasmussen Sep 11, 2018

Contributor

@iseulde I'm realizing I may be causing regressions by introducing this rule. Can you give me a sanity check that removing this rule?

What other cases might there be where this opacity would be used as the only way to indicate placeholder text?

@jasmussen

jasmussen Sep 11, 2018

Contributor

@iseulde I'm realizing I may be causing regressions by introducing this rule. Can you give me a sanity check that removing this rule?

What other cases might there be where this opacity would be used as the only way to indicate placeholder text?

This comment has been minimized.

@iseulde

iseulde Sep 13, 2018

Member

I'm not sure. Only thing I can think of is adding emoji as placeholders, which would not look like placeholder text.

@iseulde

iseulde Sep 13, 2018

Member

I'm not sure. Only thing I can think of is adding emoji as placeholders, which would not look like placeholder text.

This comment has been minimized.

@jasmussen

jasmussen Sep 14, 2018

Contributor

That's an awesome observation 😅

Thanks so much. I'll keep an eye on any regressions that might come of this.

@jasmussen

jasmussen Sep 14, 2018

Contributor

That's an awesome observation 😅

Thanks so much. I'll keep an eye on any regressions that might come of this.

jasmussen added some commits Sep 3, 2018

Fix placeholder contrast.
This fixes #9502 and fixes #9532.
Fix caption placeholder styles.
This fixes the caption styles for images (and embeds and anything else that has external captions), as well as for gallery items which has placeholder text overlaid. The overlay placeholder text has about a 4.8 average contrast ratio with the text on a scrim over a light image. This will vary a bit based on the darkness of the image.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 11, 2018

Contributor

Rebased, squashed.

I could use a sanity check from @iseulde about the removal of this opacity rule: https://github.com/WordPress/gutenberg/pull/9562/files#diff-9bc45fdf28d434e26e152546f25fd08eL93

Do you think removing it and relying on rgba color is going to have adverse effects? There was one effect as Kjell noted with the gallery images, but I fixed that in b9309ea

screen shot 2018-09-11 at 10 41 17

Contributor

jasmussen commented Sep 11, 2018

Rebased, squashed.

I could use a sanity check from @iseulde about the removal of this opacity rule: https://github.com/WordPress/gutenberg/pull/9562/files#diff-9bc45fdf28d434e26e152546f25fd08eL93

Do you think removing it and relying on rgba color is going to have adverse effects? There was one effect as Kjell noted with the gallery images, but I fixed that in b9309ea

screen shot 2018-09-11 at 10 41 17

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 11, 2018

Contributor

Looks great, thanks @jasmussen. 👏

Contributor

kjellr commented Sep 11, 2018

Looks great, thanks @jasmussen. 👏

@aduth

aduth approved these changes Sep 13, 2018

Can't speak to accessibility or design, but code-wise you get my 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 14, 2018

Contributor

Thanks for the reviews, everyone. Let's get this in.

Contributor

jasmussen commented Sep 14, 2018

Thanks for the reviews, everyone. Let's get this in.

@jasmussen jasmussen merged commit f9d3f6e into master Sep 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the fix/placeholder-contrast branch Sep 14, 2018

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