Closed Bug 1725569 Opened 3 years ago Closed 3 years ago

visualViewport doesn't emit resize event for iframes

Categories

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

Firefox 91
defect

Tracking

()

VERIFIED FIXED
95 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- verified
firefox91 --- wontfix
firefox92 --- wontfix
firefox93 --- wontfix
firefox94 --- verified
firefox95 --- verified

People

(Reporter: timocov, Assigned: tnikkel)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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

Has Regression Range: --- → yes
Component: Untriaged → Panning and Zooming
Product: Firefox → Core

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

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.

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?

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

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.

Flags: needinfo?(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.

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.

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.

Severity: -- → S2
Priority: -- → P2
Assignee: nobody → jwatt
Status: UNCONFIRMED → NEW
Ever confirmed: true

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

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?

(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?

Assignee: jwatt → tnikkel
Flags: needinfo?(tnikkel)

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

Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/452b230b7276 Send visualViewport resize events for iframes. r=botond

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

Flags: needinfo?(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.

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.

Depends on: 1735266

Bug 1735266 for the issue described in comment 18.

Flags: needinfo?(tnikkel)
Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c82acc1c4509 Send visualViewport resize events for iframes. r=botond
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch

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
Attachment #9244959 - Flags: approval-mozilla-beta?

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

Approved for 94.0b6

Attachment #9244959 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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
Attachment #9244959 - Flags: approval-mozilla-esr91?

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

Approved for 91.3esr.

Attachment #9244959 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

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

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

Flags: needinfo?(iriz1994)
Depends on: 1749661
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: