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

Remove the BlockAsyncRenderProvider and render parents asynchronously #19343

Merged
merged 2 commits into from Jan 6, 2020

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 27, 2019

This component was used to synchronously render parents for selected blocks. It was useful to avoid some design glitches when we had borders over parent blocks I think.

I think with the last design updates, this is not necessary anymore. I didn't see anything strange in my testing of nested blocks but I want to confirm with @gziolo @jasmussen

Also, this represents something like 10% performance improvement when typing.

@youknowriad youknowriad requested review from ellatrix and talldan as code owners Dec 27, 2019
@youknowriad youknowriad self-assigned this Dec 27, 2019
@youknowriad youknowriad requested review from gziolo and jasmussen Dec 27, 2019
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 28, 2019

I remember some positive impact on the Columns block behavior when we added this change. There were also very noticeable delay on the UI feedback in Safari which this change introduced. I see that there is one e2e test failing on Travis which might be relevant to that. I don’t think it’s something that you can easily catch in manual testing unless you try with CPU throttling and browsers like Safari or IE11. If it doesn’t regress then you surely should land this optimization 👍

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 28, 2019

Yep, if this is what I think it is, it's important to test the select tool for nested blocks in Safari before merging. I.e. create a columns block with several layers of nesting, then use the select tool and verify you can click to first select the Columns, then the Column, then a Paragraph inside, and that it works as expected.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 2, 2020

Tested this on safari, it seems to work properly. I'm going to check the failing test.

@youknowriad youknowriad requested review from ajitbohra, nerrad and ntwb as code owners Jan 2, 2020
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 2, 2020

The test is using a a block that updates its content based on changes in its children block. And this happens async now when the block is not selected. I updated the test to "waitforchange" instead of testing synchronously. (This is normal and can happen with any block using the data module to fetch data while not selected)

@youknowriad youknowriad merged commit 6bf89a4 into master Jan 6, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the update/async-render-parents branch Jan 6, 2020
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 6, 2020

👍

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 6, 2020

I do apologize for not having a chance to test this before, I'm only just back now. However the same regression is now present:

safari

This is why we could not use async in the past, this appears to have been unchanged.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 6, 2020

Oh sorry @jasmussen can you explain a bit more what's wrong here?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 6, 2020

Oh I see, you get the "embed" block to select itself first after clicking outside. I'm not able to reproduce this personally though. (Safari 13)

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 6, 2020

It's a bit hard to explain, but yes you got it. If you use the select tool to drill down to a specific level, then click to deselect, then click again, you go directly to the same level you were at before. The layer should always be reset back to the topmost when you deselect.

This is a Safari only issue. I see it on Safari 13.0.4. I can reproduce it easily and consistently with the Columns block, but weirdly I can't reproduce it with for exaple a Cover block.

I believe there were additional side-effects as well, though I can't recall them. Overall the issue seems less pressing than it was when the clickthrough was default, but still worth at least ticketing, if not to fix, then at least as a place to reference in case similar issues surface.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 7, 2020

I'm on the fence here about whether we should revert or not. the issue is a bit hard to trigger and 10% perf improvement is not negligible.

Would you mind creating an issue about it @jasmussen I still can't reproduce on my end.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 7, 2020

I'm on the fence here about whether we should revert or not. the issue is a bit hard to trigger and 10% perf improvement is not negligible.

I hear you. I'm personally for revert, since it's an issue re-introduced that is potentially a symptom of additional issues elsewhere. But given that it's more edgecase than it was previously, it's also not a strong opinion, and I created this ticket to track it: #19454. Feel free to label it better in case I missed something!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.