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

Global Styles → Blocks: Radius control very slow and laggy, possibly due to inline preview #47352

Closed
jasmussen opened this issue Jan 23, 2023 · 14 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended

Comments

@jasmussen
Copy link
Contributor

If you go into Global Styles, then Blocks, then for example Button, the border radius control is very slow and laggy:

slow

That same control is plenty responsive in the editor view:

editor radius

It seems possible that the inline block preview can be related.

@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Jan 23, 2023
@jasmussen
Copy link
Contributor Author

@madhusudhand I think you worked on the block preview feature in #46727, do you think it could be related? Thank you for looking!

@kathrynwp kathrynwp added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Jan 23, 2023
@madhusudhand
Copy link
Member

madhusudhand commented Jan 24, 2023

I have done a quick check. It is still an issue without inline preview.

with preview without preview
border-preview border-no-preview

@jasmussen
Copy link
Contributor Author

Thank you for looking!

@abhi3315
Copy link
Contributor

@jasmussen I have added a debounce of 1ms to onChange used in BorderRadiusControl (file: packages/edit-site/src/components/global-styles/border-panel.js).

Here are the results-

Before

before.mov

After

after.mov

@jasmussen
Copy link
Contributor Author

Thanks for that, do you have a PR for it? It looks better but still not entirely perfect, so I'm curious what might be going on under the hood. I'll add a label and hopefully we can get some additional eyes.

@jasmussen jasmussen added the Needs Technical Feedback Needs testing from a developer perspective. label Jan 25, 2023
@Mamaduka
Copy link
Member

I did quick profiling for this. Changing block global styles settings rerenders the whole Site Editor, which can be expensive and make UI feel unresponsive.

@jasmussen
Copy link
Contributor Author

Could we the inline previewe update instantly, but defer updating the site editor until mouseup, or something along those lines?

@Mamaduka
Copy link
Member

@jasmussen, I think that would be the right direction. We only want instant feedback in previews.

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Jan 29, 2023

The problem I can see with only updating in the editor on mouseup or blur is that a user may be looking at a button in the editor as they update the control, rather than the preview, and they may wonder why that button is not changing as that is the expected UX for all other global styles panel controls.

I put a draft PR up with the debounce option mentioned by @abhi3315, which seems to improve things - I wonder if it is worth doing this for 6.2 and exploring a wider solution to the preview/site edit perf issues after that?

@jasmussen
Copy link
Contributor Author

It seems like this would fall in bugfix territory which allows us more time than feb 7 no? But which technical solution to go with, I'll refer to you all. It should feel completely fluid when dragging the slider, I guess we can also update the canvas when you stop moving the mouse? Or when a value is fixed for a short while?

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Jan 30, 2023

It seems like this would fall in bugfix territory which allows us more time than feb 7 no?

In theory, but at a quick glance it seems like a robust/permanent solution to this could involve some major replumbing of how these controls work. Currently, the preview and the editor are reacting to the same data updates. The slider position and value input box are also reacting to this same data update (this is why the debounce can't be wound up any higher than about 5ms or the slider and input box updates get very sluggish), so these would all need to be decoupled somehow. If we go down this path it probably makes sense to look at it from a wider global styles perspective rather than just the border radius, so could turn into more than a simple bug fix.

I would still push for adding the debounce to improve the situation slightly and circle back after 6.2 to revisit how these controls in general interact with the preview and editor canvas ... but I may be overlooking another quick and easy fix for this.

@annezazu
Copy link
Contributor

@glendaviesnz are you able to progress on #47532? Trying to see if we can indeed get some iteration in place for 6.2 or whether the entire thing needs to be punted for now.

@glendaviesnz
Copy link
Contributor

@jasmussen This seems much more performant now on trunk:

radius.mp4

Are you still seeing the performance issues?

@jasmussen
Copy link
Contributor Author

Hmm, yes, not sure what happened, but it is indeed much better. I still feel like it could be even better still, but for now I'm going to close this one as fixed. CC: @youknowriad for awareness, I know he's done some perf work recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

No branches or pull requests

7 participants