Fix: Setting a contrast color, triggers a contrast warning. #14963
Conversation
jorgefilipecosta
requested review from
aduth,
ajitbohra,
chrisvanpatten,
gziolo,
noisysocks,
notnownikki,
Soean,
talldan and
youknowriad
as code owners
jorgefilipecosta
added
[Type] Bug
| if ( isShallowEqual( this.lastRecomputeValues, newRecomputeValues ) ) { | ||
| return false; | ||
| } | ||
| this.lastRecomputeValues = newRecomputeValues; |
There was a problem hiding this comment.
Is there any alternative way to implement it which would avoid using instance variable? In general, it's been always very hard to make it work in a predictable way in other parts of Gutenberg.
There was a problem hiding this comment.
Hi @gziolo this PR does not use instance variables now. We take advantage of the function scope which is more predictable, only calls to shouldRecomputeValues use and update lastRecomputeValues. The other possible alternative is using state but it would cause unnecessary rerenders.
jorgefilipecosta
force-pushed
the
fix/setting-a-black-color-triggers-contrast-warning
branch
from
1d6165b
to
95f3250
Compare
jorgefilipecosta
force-pushed
the
fix/setting-a-black-color-triggers-contrast-warning
branch
from
95f3250
to
9851545
Compare
|
To be honest, I'm not really satisfied with how this HoC. as it's a bit complex in addition to the fact that it doesn't work when you have a parent block changing the background (group...) which I think is going to happen more and more often. I also noticed that even if it's updated often, it can cause performance issues (I didn't check recently though) Do you think we can instead look at ways we can simplify this and improve the contrast checking maybe at a more global level. It could even be an "on demand" thing or something executed regularly but not in a synchronous way? |
|
|
||
| export default ( mapNodeToProps ) => createHigherOrderComponent( | ||
| export default ( mapNodeToProps, computeWhenChange ) => createHigherOrderComponent( |
There was a problem hiding this comment.
It is a public API change so we would have to document it and ensure it is backward compatible. It increases the complexity to use this HOC as well.
| const { grabStylesCompleted, fallbackStyles } = this.state; | ||
| if ( this.nodeRef && ! grabStylesCompleted ) { | ||
| const { fallbackStyles } = this.state; | ||
| if ( this.nodeRef && this.shouldRecomputeValues() ) { |
There was a problem hiding this comment.
Is mapNodeToProps that expensive to run that we need to cache it?
It looks like it would solve the issue if we would avoid the local state in the first place. Otherwise, we need to find a way to reset it when the styles change outside, e.g. you apply a class name to the block or change its style variant. which is not taken into account in the proposed solution.
|
Closing this PR as we are refactoring the paragraph to use useColors and during the refactor we should address the issue. |
jorgefilipecosta
deleted the
fix/setting-a-black-color-triggers-contrast-warning
branch
Description
Fixes: #14687
Alternative to: #14716
withFallbackStyles is a HOC that allows components to query styles from the dom. This allows retrieving colors used in contrast checking when explicit colors are not set.
To avoid querying the dom on each prop change, this component had an optimization as soon as all the fallback colors are known it never computed them again.
This assumed that fallBack colors of specific block instance (colors set with CSS) never changed during the editor execution. This assumption was wrong.
When we set a background color dark in twenty nineteen the CSS class also sets a text color (the text color attribute is not changed), so in this case setting a background color changes the fallback text color.
This PR adds a second parameter to withFallbackStyles, a function that receives the props and should return an array with values that may be relevant to the fallback styles. When a change happens to these values the fallback styles are recomputed.
Remaining task: document the new functionality
How has this been tested?
I repeated the steps described in #14687 and verified the bug is not happening anymore.