Improve cover block heuristics for is-light/is-dark-theme #45076
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajlende
added
[Block] Cover
|
Size Change: +77 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
getdave
approved these changes
Nov 2, 2022
|
@ajlende Is this ready to merge? |
ajlende
force-pushed
the
fix/cover-block-default-text-color
branch
from
4b1eada
to
9ff0204
Compare
skorasaurus
added
the
[Focus] Accessibility (a11y)
added 15 commits
August 4, 2023 09:44
A >50% dimmed block should use the overlay color for computation; otherwise, the media should be used as shown in this table. | >50% dimmed | media isDark | overlayColor isDark | setIsDark() | |:----------- |:------------ |:------------------- |:----------- | | false | false | false | false | | false | false | true | false | | false | true | false | true | | false | true | true | true | | true | false | false | false | | true | false | true | true | | true | true | false | false | | true | true | true | true |
ajlende
force-pushed
the
fix/cover-block-default-text-color
branch
from
28eea2c
to
2041c8b
Compare
This was referenced Aug 4, 2023
ajlende
added
[Type] Bug
ajlende
changed the title
ajlende
commented
Aug 4, 2023
| const popupColorPicker = screen.getByRole( 'button', { | ||
| name: 'Color: Black', | ||
| } ); | ||
| await userEvent.click( popupColorPicker ); | ||
| expect( coverBlock ).toHaveClass( 'is-light' ); | ||
| expect( coverBlock ).not.toHaveClass( 'is-light' ); |
There was a problem hiding this comment.
@glendaviesnz I think the behavior tested here was wrong. The default value in cover block CSS is black, so the block should have the is-dark-theme class, not is-light if there is no color manually set.
There was a problem hiding this comment.
Glen must agree because it was fixed in 56f6bc5#diff-a98ee8d5e36c3c5d47d887be50f9058b8dff86e6b475d000b4a2c7b80c1e6888L407-R435.
I'm not sure this version is fixed though. Nothing here causes changes to the classes between the assertions and the assertions are the same. It looks like the test didn't fail before 7f87feb was reverted and it should have by intent.
There was a problem hiding this comment.
I've gone ahead and created #53375 from this branch. It includes 0b6eb9a, changing this test to work as was done by Glen. It also includes 08ebc9d fixes for a couple other failing tests (the same two that are still failing on this branch at present due to act warnings).
If you'd like I'd be glad to push just those changes to this branch. In case you agree and don't want to wait for me here’s all it should take:
git fetch upstream fix/cover-is-dark-flicker1a
git cherry-pick 08ebc9d 0b6eb9a
There was a problem hiding this comment.
@glendaviesnz I think the behavior tested here was wrong.
Yes, it was wrong. Those tests were testing the existing behaviour in trunk, which was in fact broken, and as @stokesman notes, the broken behaviour was fixed in #53253 and the test updated.
ajlende
commented
Aug 4, 2023
Comment on lines
+91
to
+92
| <!-- wp:cover {"layout":{"type":"constrained"}} --> | ||
| <div class="wp-block-cover"><span aria-hidden="true" class="wp-block-cover__background has-background-dim-100 has-background-dim"></span><div class="wp-block-cover__inner-container"></div></div> |
There was a problem hiding this comment.
Previously the useEffect was getting triggered on insertion of a block. Now that the default value is falsy, it doesn't get triggered until after selecting a background.
This reverts commit 7f87feb.
glendaviesnz
added a commit
that referenced
this pull request
Aug 6, 2023
|
I think that now #53253 has merged we can close this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Why?
Fixes #53253.
Previously the heuristic was to use the image average brightness at 50% overlay opacity or lower and the overlay color at greater than 50% overlay opacity.
No heuristic will be perfect, but I think this one produces better results than the previous one.
How?
Simplifies
useCoverIsDarkinto a singleuseEffectthat prevents the oscillation seen in #53211.Defaults to black background in calculations like the block CSS.
Calculates
isDarkbased on the average image color with overlay.Simple alpha compositing is done in the browser with the Porter Duff source over operation. Colord, which we're using for color operations, doesn't include the source over operation, so it has been implemented in this PR. The
mixoperation is close, but does it's calculations in the Lab color space instead of sRGB like the browser.Example numbers for the image below and the default overlay.
rgb(152, 141, 133)rgba(0, 0, 0, 0.5)rgb(76, 71, 67)Testing Instructions
Try different combinations of images and overlay colors to see if the text color matches your expectations. Below is an example image with an average color brightness of 56%.
Screenshots or screencast
Before
Overlay opacity was ≤50%, so text color was based solely on image's average color brightness of 56%, so black text is used.
After
Image average color overlaid with the 50% opacity black has a brightness of 28%, so white text is used.