visualViewport doesn't emit resize event for iframes
Categories
(Core :: Panning and Zooming, defect, P2)
Tracking
()
People
(Reporter: timocov, Assigned: tnikkel)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr91+
|
Details |
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
Reporter | ||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 1•3 years ago
|
||
Set release status flags based on info from the regressing bug 1551302
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
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.
Assignee | ||
Comment 3•3 years ago
|
||
This is the call that triggers the resize event for the top level case
Comment 4•3 years ago
|
||
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?
Reporter | ||
Comment 5•3 years ago
|
||
(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.
Updated•3 years ago
|
Comment 6•3 years ago
|
||
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.
Assignee | ||
Comment 7•3 years ago
|
||
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.
Assignee | ||
Comment 8•3 years ago
|
||
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•3 years ago
|
||
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.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 10•3 years ago
|
||
Set release status flags based on info from the regressing bug 1551302
Reporter | ||
Comment 11•3 years ago
|
||
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•3 years ago
|
||
(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 | ||
Comment 13•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 14•3 years ago
|
||
Turned out to be simpler then I thought, so just posted a patch.
Comment 15•3 years ago
|
||
Comment 16•3 years ago
|
||
Backed out changeset 452b230b7276 (Bug 1725569) for causing mochitest failures on test_recursive_frames.html.
Backout link
Push with failures - 1
Failure Log
Assignee | ||
Comment 17•3 years ago
|
||
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.
Assignee | ||
Comment 18•3 years ago
|
||
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.
Assignee | ||
Comment 19•3 years ago
|
||
Bug 1735266 for the issue described in comment 18.
Comment 20•3 years ago
|
||
Comment 21•3 years ago
|
||
bugherder |
Assignee | ||
Comment 22•3 years ago
|
||
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
Updated•3 years ago
|
Updated•3 years ago
|
Comment 23•3 years ago
|
||
Comment on attachment 9244959 [details]
Bug 1725569. Send visualViewport resize events for iframes. r?botond
Approved for 94.0b6
Comment 24•3 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 25•3 years ago
|
||
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•3 years ago
|
||
Comment on attachment 9244959 [details]
Bug 1725569. Send visualViewport resize events for iframes. r?botond
Approved for 91.3esr.
Comment 27•3 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Comment 28•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
Valeria, would you mind filing a new bug? Thanks!
Comment 31•3 years ago
|
||
Description
•