Bug 236346

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:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Reduction (based on testcase)	none
Test showing incorrect iframe sizing behavior	none

Simon Fraser (smfr)

Reported 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.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2022-02-14 13:38:29 PST

<rdar://problem/88399120>

cathiechen

Comment 2 2022-02-15 07:11:47 PST

Yeah, I can reproduce it, and I will take a look today.

Simon Fraser (smfr)

Comment 3 2022-02-22 14:58:49 PST

Any progress here?

cathiechen

Comment 4 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.

cathiechen

Comment 5 2022-03-02 05:36:53 PST

Created attachment 453593 [details] Patch

cathiechen

Comment 6 2022-03-09 08:32:12 PST

Created attachment 454241 [details] Patch

cathiechen

Comment 7 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!

cathiechen

Comment 8 2022-03-09 23:30:05 PST

Created attachment 454321 [details] Patch

cathiechen

Comment 9 2022-03-10 01:35:02 PST

Created attachment 454325 [details] Patch

Simon Fraser (smfr)

Comment 10 2022-07-07 10:07:04 PDT

Created attachment 460740 [details] Reduction (based on testcase)

Simon Fraser (smfr)

Comment 11 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.

Simon Fraser (smfr)

Comment 12 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.

Simon Fraser (smfr)

Comment 13 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()

Simon Fraser (smfr)

Comment 14 2022-07-07 14:59:51 PDT

Created attachment 460747 [details] Test showing incorrect iframe sizing behavior

Simon Fraser (smfr)

Comment 15 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.

Simon Fraser (smfr)

Comment 16 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.

cathiechen

Comment 17 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:)

Simon Fraser (smfr)

Comment 18 2022-07-08 21:41:31 PDT

Pull request: https://github.com/WebKit/WebKit/pull/2258

EWS

Comment 19 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.

Note You need to log in before you can comment on or make changes to this bug.