1725569 - visualViewport doesn't emit resize event for iframes

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Eugene Timokhov]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

This is an exact copy of Chrome bug https://bugs.chromium.org/p/chromium/issues/detail?id=1213418, which was fixed some time ago. It seems that dom.visualviewport.enabled feature is enabled by default since v91 and it causes the issue.

(1) Open https://jsfiddle.net/f8vmsLka/
(2) Open the console
(3) Try to resize the output iframe

Actual results:

Nothing, no "resize" outputs

Expected results:

prints "resize" on every resize event

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1551302

Comment 2

[Timothy Nikkel (:tnikkel)]

There is a web platform test for this (viewport-resize-event-on-iframe-size-change.html) it got merged into our tree and automatically marked failing in bug 1713003 on June 8. Visual viewport api got enabled in bug 1551302 on June 1. No one who knew about visual viewport api and it getting enabled on desktop knew about the test getting added and marked as failing I don't think. Unfortunate.

Comment 3

[Timothy Nikkel (:tnikkel)]

This is the call that triggers the resize event for the top level case

https://searchfox.org/mozilla-central/rev/a831b7db5643a9ae7a1ce2851330ce94075093ad/layout/generic/nsGfxScrollFrame.cpp#1373

Comment 4

[Botond Ballo [:botond]]

I'm confused about how this can be a regression from bug 1551302, which enabled the Visual Viewport API.

The test case uses the Visual Viewport API, so surely, prior to its enablement, it still printed nothing?

Comment 5

[Eugene Timokhov]

(In reply to Botond Ballo [:botond] from comment #4)

I'm confused about how this can be a regression from bug 1551302, which enabled the Visual Viewport API.

The test case uses the Visual Viewport API, so surely, prior to its enablement, it still printed nothing?

Sorry if I confused you, I added the label "regression" based on the knowledge that it worked well prior this feature was enabled by default in the browser v91, but it didn't work if you enable this feature manually in a version prior v91 (tested in v88) (in terms of "browsers versions" it might look like a regression, but perhaps not in terms of issues). My bad.

Comment 6

[Bob Owen (:bobowen)]

Someone else has reported an issue that seems to be related to this in bug 1717209 comment 20.

Testing https://www.bitstamp.net/market/tradeview/ on windows, the content in the tradingview iframe doesn't resize.
If I flip the dom.visualviewport.enabled pref to false, it starts working again.

Comment 7

[Timothy Nikkel (:tnikkel)]

Thanks for the heads up about this affecting a site. Did a quick search and there appears to be at least one check for the existence of the visualviewport property, so I'm guessing the site depends on the resize event firing in the visualviewport if it exists, otherwise it uses some other method.

Comment 8

[Timothy Nikkel (:tnikkel)]

To fix this I'm thinking we need to store a previous size of every root scroll frame and if it changes after reflow call the visual viewport post resize event. There already is the visual viewport size on the presshell, but I'm not sure if we should set that in the iframe case.

Comment 9

[Botond Ballo [:botond]]

Thanks, based on recent comments I think I now understand how this can be regression.

A site can have code like this:

if ("visualViewport" in window) {
  // register a listener for the visualViewport's resize event
} else {
  // register a listener for the regular resize event
}

The expected behaviour would then be that, if an iframe is resized, then either the regular resize listener or the visualViewport resize listener fires.

With visualViewport disabled, the second branch was taken so the regular resize listener is registered and fires. With visualViewport enabled, the first branch is taken but, due to this bug, no event is fired.

That would indeed be a regression. I was just confused because the page linked in comment 0 didn't contain code like this, it only used visualViewport.

Comment 10

[Marco Castelluccio [:marco]]

Set release status flags based on info from the regressing bug 1551302

Comment 11

[Eugene Timokhov]

Sorry for bothering you, but is there any update when it might be fixed? There is a lot of websites where our products might be installed and we cannot update all them since it is not our projects so all of them will be affected by this issue. Maybe it is possible to disable this feature remotely until the issue is solved?

Comment 12

[Botond Ballo [:botond]]

(In reply to Eugene Timokhov from comment #11)

Sorry for bothering you, but is there any update when it might be fixed?

I'm hopeful we can get to this in the current Nightly cycle (Firefox 95). Depending on the timing and complexity of the fix, we may be able to backport it to the Beta channel (Firefox 94), and possibly to the ESR channel (Firefox 91 ESR) though the latter is a higher bar and usually reserved for security fixes.

There is a lot of websites where our products might be installed and we cannot update all them since it is not our projects so all of them will be affected by this issue. Maybe it is possible to disable this feature remotely until the issue is solved?

Unfortunately, disabling VisualViewport is going to break other sites, such as Google Docs when pinch-zoomed in (bug 1711989).

Is your affected product a framework / library? Can you help us quantify the scope of the breakage, e.g. example sites, number of sites / users?

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1725569. Send visualViewport resize events for iframes. r?botond

Comment 14

[Timothy Nikkel (:tnikkel)]

Turned out to be simpler then I thought, so just posted a patch.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/452b230b7276
Send visualViewport resize events for iframes. r=botond

Comment 16

[Marian-Vasile Laza]

Backed out changeset 452b230b7276 (Bug 1725569) for causing mochitest failures on test_recursive_frames.html.
Backout link
Push with failures - 1
Failure Log

Comment 17

[Timothy Nikkel (:tnikkel)]

Hmm, this is weird, nothing about the test seems to have been changed by my patch. I'm guessing firing a few more events is causing some timing related things to change and the test had an existing issue. The is an intermittent from months ago with what looks like the same failure mode, but it was also resolved months ago because of no new occurrences.

Comment 18

[Timothy Nikkel (:tnikkel)]

The failing test (docshell/test/navigation/test_recursive_frames.html) is checking that a series of nesting iframes stops after 10. We fail because there is an 11th document, not the document requested by the test but about:blank. This happens because the session store code gets called (not sure exactly why but I assume it gets called at many points), and this line https://searchfox.org/mozilla-central/rev/2c4b830b924f42283632b70f39a60fd36433dd4d/toolkit/components/sessionstore/SessionStoreUtils.cpp#1349 eventually leads to the creation of a document where the iframe that we refuse to load is via the path nsPIDOMWindowOuter::GetDoc, nsPIDOMWindowOuter::MaybeCreateDoc, nsDocShell::GetDocument, nsDocShell::EnsureContentViewer, nsDocShell::CreateAboutBlankContentViewer. I suppose that dispatching these extra dom events causes just enough delay for the sessionstore code to have time to be called.

Making the GetDoc call in the sessionstore code to use GetExtantDoc instead (which doesn't create a doc if there isn't one) fixes the problem.

Comment 19

[Timothy Nikkel (:tnikkel)]

Bug 1735266 for the issue described in comment 18.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c82acc1c4509
Send visualViewport resize events for iframes. r=botond

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c82acc1c4509

Comment 22

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9244959 [details]
Bug 1725569. Send visualViewport resize events for iframes. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: some websites depend on this event in iframes when resizing, otherwise the content doesn't resize eg https://www.bitstamp.net/market/tradeview/
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1735266
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): just sending a dom event to iframes that we already send to root documents
• String changes made/needed: none

Comment 23

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9244959 [details]
Bug 1725569. Send visualViewport resize events for iframes. r?botond

Approved for 94.0b6

Comment 24

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f22555d3d476

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9244959 [details]
Bug 1725569. Send visualViewport resize events for iframes. r?botond

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: significant visible problem with some websites
• User impact if declined: some websites depend on this event in iframes when resizing, otherwise the content doesn't resize eg https://www.bitstamp.net/market/tradeview/
• Fix Landed on Version: 95
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): just sending a dom event to iframes that we already send to root documents
• String or UUID changes made by this patch: none

Comment 26

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9244959 [details]
Bug 1725569. Send visualViewport resize events for iframes. r?botond

Approved for 91.3esr.

Comment 27

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/fc62b3eb0862

Comment 28

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nightly 95.0a1, Firefox 94, and Firefox 91.3.0 esr - "resize" is now printed in the Console. Verified on macOS Big Sur 11.6, Windows 10 x64, and Ubuntu 20.04 x 64.

Comment 29

[Valeria Fedorova]

Resize event for visualViewport in iframe doesn't fire if container element was hidden.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:95.0) Gecko/20100101 Firefox/95.0

Steps to reproduce:

(1) Open https://jsfiddle.net/kq1Lc0fo/
(2) Open the console

Actual results:

Only "W resize" output

Expected results:

Print "VW resize" and "W resize" after page is loaded

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Valeria, would you mind filing a new bug? Thanks!

Comment 31

[Valeria Fedorova]

Done https://bugzilla.mozilla.org/show_bug.cgi?id=1749661