| Summary: | Continual resizeobserver-driven flashing on Jupyter notebook page | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||
| Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Critical | CC: | bfulgham, cathiechen, fred.wang, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||
| Priority: | P1 | Keywords: | InRadar | ||||||||||||||
| Version: | Safari Technology Preview | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Simon Fraser (smfr)
2022-02-08 19:54:42 PST
Yeah, I can reproduce it, and I will take a look today. Any progress here? I'm trying to debug the page, but it seems very hard to simplify the page, so the progress is slow. It seems there is a loop in calculating the iframe size. The iframe with `width: 100%; height: 100%` is inside a `<div>` as a render container, iframe's size is determined by this `<div>`. Inside iframe, there is an resizeObserver observing the size of html, and it sends the html size to the main frame by a resize event. When the main frame gets the resize event, it sets the size to the `<div>`. The wierd part is, after the `<div>` gets the size that is calculated when iframe is without scrollbar, somehow with this size, iframe shows scrollbar, that causes a different size of iframe `<html>`, because the width is changed if scrollbar is on. Then it triggers another round, on and on. Created attachment 453593 [details]
Patch
Created attachment 454241 [details]
Patch
Comment on attachment 454241 [details]
Patch
Hi Simon!
I think the patch is ready for review. Please take a look, thanks!
Created attachment 454321 [details]
Patch
Created attachment 454325 [details]
Patch
Created attachment 460740 [details]
Reduction (based on testcase)
(In reply to cathiechen from comment #4) > The wierd part is, after the `<div>` gets the size that is calculated when > iframe is without scrollbar, somehow with this size, iframe shows scrollbar, > that causes a different size of iframe `<html>`, because the width is > changed if scrollbar is on. > Then it triggers another round, on and on. I think this is the key. The testcase oscillates between two heights for the <iframe>, 328px (scrollbar hidden) and 346px (scrollbar visible). However, when we lay out with the height of 346px, we drop the scrollbar and size the iframe's <html> to 200x328, showing that we should be able to render in that size without the scrollbar. But ScrollView::updateScrollbars() brings the scrollbar back. When the iframe height is set to 328px, we hit RenderWidget::setWidgetGeometry() with 200x328 and call into updateScrollbars(), but the document still has the old height of 346 so here: newHasVerticalScrollbar = docSize.height() > visibleHeight(); but this is based on stale layout and we don't do a second pass of scrollbars updates to figure out that we can actually fit. Comment on attachment 454325 [details]
Patch
It's not clear to me that this is the correct fix yet. I think the fix is more about updateScrollbars()
Created attachment 460747 [details]
Test showing incorrect iframe sizing behavior
Comment on attachment 454325 [details]
Patch
Actually this patch is very close to what I cam up with independently, so I'm confident it's close to correct. I will suggest some renaming. I also have a non-ResizeObserver testcase for iframe resizing which is a clearer demonstration of the underlying issue.
Sorry for the long delay in reviewing, Cathie, and I'm glad we converged on a similar solution. The code around updating scrollbars and layout is tricky, and I wanted to be sure that this was the right fix. Hi Simon! It's all right, I understand this part is tricky, and we need to be careful. And thanks for the review! I will rebase the code later:) Pull request: https://github.com/WebKit/WebKit/pull/2258 Committed 252440@main (912b33473fa3): <https://commits.webkit.org/252440@main> Reviewed commits have been landed. Closing PR #2258 and removing active labels. |