Improve cover block heuristics for is-light/is-dark-theme#45076
Closed
Improve cover block heuristics for is-light/is-dark-theme#45076
Conversation
|
Size Change: +77 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
getdave
approved these changes
Nov 2, 2022
Contributor
getdave
left a comment
There was a problem hiding this comment.
Screen.Capture.on.2022-11-02.at.13-11-18.mp4
I tested this and it seemed to give me sensible text color choices.
I also found some more unusual images to try and trick the system. This PR works far better than on trunk.
This PR
Trunk
I would say 🚢
Contributor
|
@ajlende Is this ready to merge? |
4b1eada to
9ff0204
Compare
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 |
28eea2c to
2041c8b
Compare
This was referenced Aug 4, 2023
ajlende
commented
Aug 4, 2023
| } ); | ||
| await userEvent.click( popupColorPicker ); | ||
| expect( coverBlock ).toHaveClass( 'is-light' ); | ||
| expect( coverBlock ).not.toHaveClass( 'is-light' ); |
Contributor
Author
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.
Contributor
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.
Contributor
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
Contributor
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> |
Contributor
Author
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.
stokesman
reviewed
Aug 4, 2023
This reverts commit 7f87feb.
glendaviesnz
added a commit
that referenced
this pull request
Aug 6, 2023
Contributor
|
I think that now #53253 has merged we can close this. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.