Cover block: fix flickering when inserted in templates and also fix isDark calculation bugs #53253
Conversation
glendaviesnz
added
[Type] Bug
glendaviesnz
changed the title
|
Size Change: +934 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
glendaviesnz
changed the title
glendaviesnz
changed the title
glendaviesnz
removed
the
[Status] In Progress
| if ( mediaUrl && useFeaturedImage ) { | ||
| setCoverIsDark( mediaUrl, dimRatio, overlayColor.color ); | ||
| } |
There was a problem hiding this comment.
Can this logic also be moved inside the event handler? This will cause attribute updates on every render when a condition is true and trigger another re-render.
While it's okay to call setState during the render, I'm not sure if the same applies to the setAttributes.
There was a problem hiding this comment.
yeh, this is the one bit I wasn't sure on. Because the setting of featured image is outside the component we don't have access to the event handle, so maybe this is one case when an effect is better - will have a play.
| @@ -103,7 +103,7 @@ function CoverEdit( { | |||
| 'featured_media', | |||
| postId | |||
| ); | |||
|
|
|||
| const setCoverIsDark = useCoverIsDark( setAttributes ); | |||
There was a problem hiding this comment.
This could become getCoverIsDark and let the components update the attribute. What do you think?
There was a problem hiding this comment.
I was initially doing that and it just seemed like a lot of replicated:
__unstableMarkNextChangeAsNotPersistent();
setAttributes( { isDark: color.isDark } );
in each of the event handlers so moving to the hook removed the duplication, but I don't have a strong opinion on this. If you think this is better handled in the edit component I can move it back?
There was a problem hiding this comment.
If we can "batch" the attribute updates, I think it's okay to include this in the undo history and drop the "non-persistent" action call. But I see that most attribute updaters have wrappers like setMedia; that would mean unwrapping them as well, probably not worth it at the moment.
There was a problem hiding this comment.
batching the updates where possible was a much better approach!
There was a problem hiding this comment.
This feels like a promising direction @glendaviesnz! It's fixed the flickering issue for me in templates that contain a pattern that has a cover block within it 👍
One subtle issue that I noticed while playing around with this is that if I adjust the overlay opacity very quickly, with this PR applied it's possible for it to not update when expected.
In the following screengrabs, on trunk the is-light class is correctly present when quickly dragging to the right. With this PR applied, it only appears to update when dragging slowly (or at least not quickly):
| Trunk | This PR |
|---|---|
 |
 |
Also a couple of the Color panel tests are currently failing — could that be related?
| const { __unstableMarkNextChangeAsNotPersistent } = | ||
| useDispatch( blockEditorStore ); | ||
|
|
||
| const getCoverIsDark = useCallback( |
There was a problem hiding this comment.
In edit.js this is used as setCoverIsDark. Should we call it that here, too?
There was a problem hiding this comment.
Depends on the answer to this question - what do you think?
There was a problem hiding this comment.
George does make a good point! I don't feel too strongly about it, though. Using getCoverIsDark might help neaten things a bit potentially as the isDark value could be updated in the same setAttributes call as other attributes (e.g. in onClearMedia, onUpdateDimRatio, etc)... but I suppose one challenge is that for images the update needs to happen asynchronously via the .then() since we won't immediately know the dark value?
| if ( mediaUrl && useFeaturedImage ) { | ||
| setCoverIsDark( mediaUrl, dimRatio, overlayColor.color ); | ||
| } |
There was a problem hiding this comment.
Will this be called on every render? Would it be worth only calling if mediaUrl, dimRatio, or overlayColor.color changes?
That might wind up re-introducing some form of useEffect, though, which might not be the goal here 🤔
There was a problem hiding this comment.
|
I think this showcases how fragile attribute updates via effects can be in blocks. We should probably try and move similar logic into event handlers for other blocks as well. |
|
Thanks for the great feedback @Mamaduka and @andrewserong, I have made your suggested changes and I think this makes things much more robust - have pushed the changes but have run out of time to fully test. I will give it a better test in the morning, but feel free to give it a quick once over and see if I have missed anything obvious. |
This seems to have been resolved by batching the update with the dimRatio attribute update. |
|
I will sort out the unit tests tomorrow. |
|
I also need to add some awaits onto the getCoverIsDark calls I think as this can now return a promise. |
|
Thanks for the updates!
That does appear to resolve the issue of dragging quickly not updating correctly. One issue I'm seeing now is that for an image that in its Here's a quick screengrab demoing the above (note that the text is only dark in this screengrab when the Overlay color is set): 2023-08-03.15.47.56.mp4 |
| // If no overlay color exists the overlay color is black (isDark ) | ||
| setIsDark( true ); | ||
| return; | ||
| export default function useCoverIsDark() { |
There was a problem hiding this comment.
I think now this could be just a callback getCoverIsDark, and there's no need for useCallback memoization.
There was a problem hiding this comment.
yeh, now we are not saving state or setting attributes there isn't any point in it being a hook, have moved it to a standard method in the shared methods folder.
| const getCoverIsDark = useCallback( | ||
| ( url, dimRatio = 50, overlayColor ) => { | ||
| // If the dimRatio is less than 50, the image will have the most impact on darkness. | ||
| if ( url && dimRatio <= 50 ) { |
There was a problem hiding this comment.
Before, when wrapped in an effect, this branch didn’t execute unless url had changed. Now, it may execute needlessly for changes to overlayColor or dimRatio (given dimRatio is less than 50). Since the url hasn't changed the color average and resulting isDark determination will end up the same. Not sure it’s objectionable or worth optimizing but thought it should be noted.
There was a problem hiding this comment.
Thanks for pointing that out. I have done some testing and this doesn't seem to introduce any performance issues, even in the places where this might get called a lot, like dragging the mouse around the custom color picker, so given the extra complexity it would add in order to better track when and when not to run this, and the fact that is now only explicitly run in event handlers, I don't think it is worth trying to optimise it at this stage.
@andrewserong I think this was due to the missing awaits, which I have now added and it seems to work as expected now. |
| @@ -88,8 +88,8 @@ exports[`Inserting blocks inserts a block in proper place after having clicked \ | |||
| <p>First paragraph</p> | |||
| <!-- /wp:paragraph --> | |||
|
|
|||
There was a problem hiding this comment.
I think it was a bug in the hook that caused isDark attrib to be set to false when a Cover block was first inserted.
I don't think there are any backwards compat issues to it now behaving correctly and not setting this unless a light image or background is selected after the block is inserted, but let me know if you can see any issues.
I added a Cover block placeholder to a post on trunk and saved, checked out this PR and reloaded the post and there were no errors, and selecting color or image on the placeholder worked as expected.
|
Switching from hook to a standard method is a reasonably big change so probably needs another thorough review sorry. |
There was a problem hiding this comment.
Nice one @glendaviesnz! This is looking really good, each of the event handlers appears to be updating the isDark value as expected and I haven't run into any issues across all actions (uploading media, using featured images, clearing media, changing overlay colors and dimRatio, etc), and it still fixes the flickering issue 👍
Just left a couple of comments — now that there's an Edit: nevermind, I was misunderstanding how async functions work in this way.await in getColorIsDark, I was wondering if the other handlers need to be "async" if getColorIsDark never returns a promise?
| * getCoverIsDark is a method that specifyies if the cover background is dark or not and | ||
| * applies the relevant attribute to help ensure that text is visible by default. |
There was a problem hiding this comment.
Nit: getCoverIsDark no longer applied the attribute, rather it's handled in the block's code. Shall we update this comment?
There was a problem hiding this comment.
This is testing great for me! Might be worth waiting for another review before merging, too, just to make sure we're covered since changing this part of the Cover block can be pretty tricky.
Great work here, though, the approach feels like a big improvement for the block, while resolving the tricky flickering issue! ✨
|
👋 Hey folks. I've been following along here (and on the issue before this PR). I'm pleased to see how this progressed. The approach matches where my mind first went on thinking of a solution, i.e. "Try not to derive the attribute in an effect when it could be done with event handlers". Still, out of curiosity, I tried an alternate approach in revamping the hook #53317. It solves the issue in my testing and due to its fewer changes might be more compelling from a maintenance perspective. If there’s time available, I'd like it if it could be considered. Much respect to all the great work and collaboration that's gone into this and I'm offering it up in the same spirit. |
Thanks for this @stokesman, I ran out of time today to get to this, and it is my end of week here, so will try and take a closer look on Monday if someone else doesn't get to it. |
|
It seems my old unmerged PR #45076 that improves the heuristic also seems to fix it. The PR already has one approval, but if you'd like to take a look, it might be another alternative. I explained what I think is going on and how it fixes it in the original issue. |
Thanks Glen! Now that Alex has brought up his PR, I'm very much in favor of it. So please disregard the PR I made as an alternate to this and instead have a look at Alex’s to see if you agree we should incorporate it first. |
|
@ajlende, @stokesman, the heuristic used in #45076 does seem to provide a better outcome in terms of the dark/light setting, but for me it does not fix the flickering issue. I have copied the updated heuristic to this PR, and this gives the same improved results while still also fixing the flickering. @stokesman, I understand your preference for a small change over a major refactor, especially in a block as commonly used as the Cover. If this was going into 6.3.1 I would also be very onboard with that. If we are thinking 6.4 for this change, given we have a couple of months until the next release I wonder if we should take the opportunity to make a bigger improvement in the structure of this block. This is not really a good use of an effect, and across the project we should be looking to move from the use of effects to update internal state in preference for using event handlers where possible, and this is a good opportunity to do this. I don't have a strong opinion on this though, so happy to hear what others think. @andrewserong, @Mamaduka what are your thoughts? |
Yes, I saw the same.
I usually like the idea of trying out smaller changes, and doing things iteratively where we can. In this case, from my perspective, each of the changes are fairly significant in the sense that we'll need to thoroughly test every permutation of changes to the cover block to make sure that the So in that respect, I'm currently leaning toward the idea of seeing if we can land an approach that we think is what we'd like in the longer term for the block. I don't feel very strongly about it, though, so happy to hear what everyone else thinks, too! |
|
My first instinct is to go for an overhaul like this PR because it should be more efficient. On the other hand, I’d like to avoid introducing complexity to an already complex block while possible. I'm glad it’s being considered and don't mind much what decision is reached.
I agree, though not without exceptions. Strictly speaking, it has not proven possible for this PR. As is, we have to lean on it for the one case anyway so if we reduce complexity by employing it for all cases it seems justifiable.
I'm glad you saw that PR. I just want to point out the change to the dependency array is only the addition of two dependencies that are stable. They could be left out, just as they are in trunk and as they are in the similar effect in this PR. Anyway, I agree no matter what we do care must be taken! |
Even though one case does still use an effect, in 4 out of the 6 use cases we are now bundling the attribute update with the source event which I think is enough of a potential efficiency gain to still warrant the change. Good discussion @stokesman, thanks for raising it. @Mamaduka did you have any additional thoughts to add before we try and make a decision on this? |
|
I like the direction of this PR. The only time the effect is used is with the featured image. Every time we refactor away from using effects, it would require a lot of changes, but that's good, IMO. |
There was a problem hiding this comment.
I've tested all the listed scenarios:
- When an overlay image is added, changed or removed
- When the featured image is selected as the overlay image, or removed from the overlay
- When the overlay color is changed
- When the overlay color is removed
- When the dimRatio is changed
They all seem to be working for me. I was never able to reproduce the original flickering issue, but things do still work as I expect here.
What?
Fixes: #53211
Also fixes bugs with the calculation if
isDarkthat were identified while working on the above fix:isDarkis set to falseisDarksetting so a black overlay dimRatio of 0 is still considered darkWhy?
The
useEffectsused in the calculation ofisDarkare the cause for both the jitter and the above bugs, so easier to fix both at the same time.How?
Replace the
useEffectswith setting ofisDarkin the relevant event handlers.Testing Instructions
Flickering bug
templatesdirectory).my-template.html, add the synced block pattern. To get the markup to add to the template, you can add the synced pattern to a post or page, then view the code editor view and copy + paste to yourmy-template.htmlfilemy-template.html.Example markup from my
my-template.htmlfile:isDark bug and verifying all isDark workflows
In summary, my thinking is that whenever the image/featured image/overlay color/dim ratio is changed the isDark value should be updated in order to prevent text disappearing. N.B. - the only flow that will not reset the isDark setting is if a featured image is removed from the post via the post settings tab. The only way I can see to fix this is by adding a useEffect, but it is a very edge case and I don't think it is worth adding extra complexity to handle this.
Screenshots or screencast
Flicker before:
2023-08-01.11.44.52.mp4
Flicker after:
flicker-after.mp4
Is dark before:
isdark-before.mp4
Is dark after:
isdark-after.mp4