Bug 236346 - Continual resizeobserver-driven flashing on Jupyter notebook page
Summary: Continual resizeobserver-driven flashing on Jupyter notebook page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-08 19:54 PST by Simon Fraser (smfr)
Modified: 2022-07-13 23:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2022-03-02 05:36 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (8.51 KB, patch)
2022-03-09 08:32 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (9.23 KB, patch)
2022-03-09 23:30 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (10.09 KB, patch)
2022-03-10 01:35 PST, cathiechen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Reduction (based on testcase) (1.58 KB, text/html)
2022-07-07 10:07 PDT, Simon Fraser (smfr)
no flags Details
Test showing incorrect iframe sizing behavior (1.06 KB, text/html)
2022-07-07 14:59 PDT, Simon Fraser (smfr)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2022-02-08 19:54:42 PST
Turn on always-on scrollbars on macOS, and load https://github.com/pytorch/opacus/blob/main/tutorials/intro_to_advanced_features.ipynb

The content continually oscillates between two sizes, with and without the scrollbar showing. This resize is driven by a resizerObserver. Does not happen in Chrome.
Comment 1 Simon Fraser (smfr) 2022-02-14 13:38:29 PST
<rdar://problem/88399120>
Comment 2 cathiechen 2022-02-15 07:11:47 PST
Yeah, I can reproduce it, and I will take a look today.
Comment 3 Simon Fraser (smfr) 2022-02-22 14:58:49 PST
Any progress here?
Comment 4 cathiechen 2022-02-23 08:47:24 PST
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.
Comment 5 cathiechen 2022-03-02 05:36:53 PST
Created attachment 453593 [details]
Patch
Comment 6 cathiechen 2022-03-09 08:32:12 PST
Created attachment 454241 [details]
Patch
Comment 7 cathiechen 2022-03-09 09:07:04 PST
Comment on attachment 454241 [details]
Patch

Hi Simon!
I think the patch is ready for review. Please take a look, thanks!
Comment 8 cathiechen 2022-03-09 23:30:05 PST
Created attachment 454321 [details]
Patch
Comment 9 cathiechen 2022-03-10 01:35:02 PST
Created attachment 454325 [details]
Patch
Comment 10 Simon Fraser (smfr) 2022-07-07 10:07:04 PDT
Created attachment 460740 [details]
Reduction (based on testcase)
Comment 11 Simon Fraser (smfr) 2022-07-07 12:14:43 PDT
(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.
Comment 12 Simon Fraser (smfr) 2022-07-07 12:31:16 PDT
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 13 Simon Fraser (smfr) 2022-07-07 14:32:36 PDT
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()
Comment 14 Simon Fraser (smfr) 2022-07-07 14:59:51 PDT
Created attachment 460747 [details]
Test showing incorrect iframe sizing behavior
Comment 15 Simon Fraser (smfr) 2022-07-07 22:16:33 PDT
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.
Comment 16 Simon Fraser (smfr) 2022-07-07 22:18:43 PDT
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.
Comment 17 cathiechen 2022-07-08 15:01:24 PDT
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:)
Comment 18 Simon Fraser (smfr) 2022-07-08 21:41:31 PDT
Pull request: https://github.com/WebKit/WebKit/pull/2258
Comment 19 EWS 2022-07-13 23:17:23 PDT
Committed 252440@main (912b33473fa3): <https://commits.webkit.org/252440@main>

Reviewed commits have been landed. Closing PR #2258 and removing active labels.