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

Polish duotone filter #143

Merged
merged 15 commits into from
Sep 17, 2020
Merged

Polish duotone filter #143

merged 15 commits into from
Sep 17, 2020

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Sep 9, 2020

Followup from #88 with some improvements on the duotone filter to get this ready to ship :shipit:

  • Render out stylesheet to apply the filter to only the media
  • Disable duotone for cover block (until structure is fixed in core Add srcset for cover image WordPress/gutenberg#25171)
  • Is it necessary to sanitize block attributes, or is that already taken care of? EDIT: Looks like it's taken care of in wp_pre_kses_block_attributes called from default-filters.php

duotone image with palette colors

duotone image with custom colors

@ajlende ajlende added the Enhancement New feature or request label Sep 9, 2020
@ajlende ajlende self-assigned this Sep 9, 2020
@jasmussen
Copy link
Member

This is what I see:

duotone

First thought: oh wow this is so excellent. I cannot wait for this to land. I love it so much.

2nd thought — for this to ship as a filter for the image block, I can think of two things we'll probably want to address.

The first one is shift that happens as soon as you apply a duotone filter. This happens, best as I an inspect, because the SVG that contains the filter is inserted before the image. Now in my example above, that has to do with margin collapsing. Here's with no SVG:

Screenshot 2020-09-09 at 11 56 26

You can see the top margin I apply to the figure element is non existant. Now the filter is applied and the SVG inserted:

Screenshot 2020-09-09 at 11 56 20

Suddenly the top-margin on my figure no longer collapses, this is simply due to there being an element before it.

The easiest fix, perhaps, is to output that SVG after the figure element. That seems like it would fix it right?

The other issue is that the duotone effect is only applied if you actually define two colors, and until you choose the 2nd color, nothing happens. What can we do here to improve that flow?

One thing we might do is to set the 2nd for you. I think that's what the pullquote block does:

pullquote

I'm not entirely sure on the best heuristic for that to happen, any thoughts?

Other than that, this is so cool!

@ajlende
Copy link
Contributor Author

ajlende commented Sep 9, 2020

@jasmussen thanks for taking a look!

The first one is shift that happens as soon as you apply a duotone filter. This happens, best as I an inspect, because the SVG that contains the filter is inserted before the image.

That must be a theme thing because I don't see the issue on Twenty Twenty. What theme are you seeing the issue on so I can test it out?

I'm also already writing a stylesheet to apply the filter, so it's not too difficult to just add some extra CSS too. If it is collapsing margins, you'll have a similar problem pushing the block below down if you move the SVG below the block.

The other issue is that the duotone effect is only applied if you actually define two colors, and until you choose the 2nd color, nothing happens. What can we do here to improve that flow?

I like your suggestion for automatically setting the second color.

The pullquote heuristic is optimizing for contrast, so you'd end up with an "inverted color" image if you chose white, for example, for the dark color. Maybe that's fine for the image block, but when this makes it's way to the cover block, I think folks will end up wanting a subtle texture for their background, not high contrast.

I think instead, if I can choose the darkest and lightest colors from the palette, I can use them as the default dark and light colors. Not only will this fix the inverted colors problem, but it will also be trivial to set either the dark or light color and have the other one automatically set. Whereas the pullquote heuristic only works when setting the background color.

Thoughts on that approach?

@jasmussen
Copy link
Member

That must be a theme thing because I don't see the issue on Twenty Twenty. What theme are you seeing the issue on so I can test it out?

It's a theme I'm working on myself. But the key is that the first block in the block list must have a top margin. That top margin usually collapses down with the title above it, but when an element is inserted, it doesn't collapse. It's rather edgecasey, but if the filter SVG can be output after the block it should fix it. Is that hard?

The pullquote heuristic is optimizing for contrast, so you'd end up with an "inverted color" image if you chose white, for example, for the dark color. Maybe that's fine for the image block, but when this makes it's way to the cover block, I think folks will end up wanting a subtle texture for their background, not high contrast.

I don't think we need to optimize for any contrast here, applying a duotone filter would intrinsically be a decorative act. I think the only thing we need to optimize for is that something happens as soon as you choose a color, but also that whatever 2nd color is chosen is not identical to the first one.

I think instead, if I can choose the darkest and lightest colors from the palette, I can use them as the default dark and light colors. Not only will this fix the inverted colors problem, but it will also be trivial to set either the dark or light color and have the other one automatically set. Whereas the pullquote heuristic only works when setting the background color.

I like the sound of "trivial" — because all we need to happen is something, so it's clear that the effect is working. It's of course important to be able to clear the colors after the fact if you decide you don't want duotone after all — I guess that just means we allow you to break the duotone effect if you clear the 2nd color after it's auto-set.

But yes, if you can do any kind of calculation of whether the first color you choose is light or dark, and then choose a highly contrasting color as the 2nd one, that would be great. Honestly it'd even be fine to set the 2nd color to be black or white depending on the lightness of color 1 — while duotone is technically two colors of your choice, it's often used as black + color, like so:

coltrane

And honestly, black works really well as the 2nd color nearly always.

When a theme uses margin-top on the block, the editor styles override the margin-top on the first child. When the SVG is the first child, that style isn't applied to the correct element.
@ajlende
Copy link
Contributor Author

ajlende commented Sep 10, 2020

It's a theme I'm working on myself. But the key is that the first block in the block list must have a top margin. That top margin usually collapses down with the title above it, but when an element is inserted, it doesn't collapse. It's rather edgecasey, but if the filter SVG can be output after the block it should fix it. Is that hard?

Not difficult, I just wanted to be able to test it out myself. After a couple modifications to the gutenberg starter theme I was able to reproduce the issue. The actual problem was that the block editor adds some styles to the first child.

.block-editor-block-list__layout > .wp-block:first-child {
    margin-top: 0;
}

With the SVG being the first child, the margin-top: 0 wasn't applied to the block anymore. Should be fixed now unless your issue is different.

I think the only thing we need to optimize for is that something happens as soon as you choose a color, but also that whatever 2nd color is chosen is not identical to the first one.

This is what I ended up doing:

I grabbed the darkest and lightest color based on luminance. When a dark color was selected and no light color had been selected, the lightest color was auto-selected for the light color. And vice versa, when a light color was selected and no dark color had been selected, the darkest color was auto-selected for the dark color.

I wanted to avoid having inverted looking colors (dark luminance > light luminance), so when a color identical to the opposite default color (i.e. choosing the darkest color for the light color or vice versa), I fall back to pure black or pure white. This still breaks if the darkest color in your palette is pure black and you select it for the light color, but it didn't seem right to me to auto-select a lighter color for the dark color in that case.

It's of course important to be able to clear the colors after the fact if you decide you don't want duotone after all — I guess that just means we allow you to break the duotone effect if you clear the 2nd color after it's auto-set.

For clearing colors, I didn't want to auto-clear a custom color if the auto-color was cleared, and it seemed strange to only auto-clear colors that were also auto-set (I tried in 508c9b7 if you wanted to see what that's like), so I ended up just keeping it simple and not bothering with auto-clearing at all. A color may be auto-set, but it will never be auto-cleared. When only one duotone color is selected from clearing just one of the colors, the duotone effect disables as it did before.

@ajlende
Copy link
Contributor Author

ajlende commented Sep 10, 2020

@jasmussen since this is getting ready to ship, we're going to need some plugin directory assets. If you can get those to me, I can start the submission process.

@jasmussen
Copy link
Member

Latest status:

duotone

Love it. The effect is instant, the color choice is totally fine, and there isn't any shift. This is superb.

For clearing colors, I didn't want to auto-clear a custom color if the auto-color was cleared, and it seemed strange to only auto-clear colors that were also auto-set

Agree, I think it works excellently as is.

@ajlende ajlende marked this pull request as ready for review September 17, 2020 17:19
@ajlende ajlende merged commit 4d14221 into master Sep 17, 2020
@ajlende ajlende deleted the fix/duotone-filter branch September 17, 2020 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants