1477610 - Enable all web platform tests for the Visual Viewport API

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Tanushree Podder [:tanushree]]

Currently, web platform tests for the Visual Viewport API are present in the code base (https://searchfox.org/mozilla-central/source/testing/web-platform/meta/visual-viewport) but are expected to fail. As a part of bug1357785 - Implementing the Visual Viewport API, we would like to enable these tests.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Changing title to re-use this bug as a tacker bug.

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1477610 - Flush layout when reporting the visual viewport size via the Visual Viewport API. r=hiro

Comment 3

[Botond Ballo [:botond]]

This patch fixes some of the failing WPTs. Some more remain.

There are also manual tests, which I'll deal with in another bug.

Comment 4

[Botond Ballo [:botond]]

The next failure I'm looking into is viewport-unscaled-size-iframe.html.

In this test, there is an <iframe> with dimensions specified in CSS and no children, and in a <script> that (as far as I can tell) runs right away, we query its visual viewport size.

The test expects the reported VV size to match the iframe's size as specified in CSS, but in fact we are reporting a size of zero, because GetRootScrollFrameAsScrollable() returns nullptr (which in turn is because mFrameConstructor->GetRootFrame() is nullptr).

If I modify the test to run the check inside window.addEventListener("DOMContentLoaded", function() { ... }), then we have a frame, and return the correct size.

==> So, it seems the problem is that, as the test is written, at the time of the check we do not have a frame yet. This is in spite of the fact that I'm flushing layout at the beginning of VisualViewportSize().

Emilio, Hiro, do you have any suggestions here? Should I be doing something other than flushing the layout to ensure the document has a root frame?

Comment 5

[Botond Ballo [:botond]]

Another failure, viewport-resize-event-on-load-overflowing-page.html appears to be a bug or limitation in the test harness; bug 1547827 tracks.

Comment 6

[Botond Ballo [:botond]]

Needinfo for comment 4. Any suggestions are appreciated!

Comment 7

[Botond Ballo [:botond]]

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

There are also manual tests, which I'll deal with in another bug.

I went through the manual WPTs. The only failures are viewport-scroll-event-manual.html, which we're tracking in bug 1543485, and viewport-url-bar-changes-height-manual.html, for which I filed bug 1549622 (but which I don't think should block shipping the API). I also noticed some weird behaviour while running viewport-offset-manual.html, for which I filed bug 1549625.

(There are also two tests which only make sense on platforms that support both pinch- and full-zoom. We don't currently have such platforms, so I didn't run those tests, but we will once we get desktop zooming.)

Comment 8

[Botond Ballo [:botond]]

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

==> So, it seems the problem is that, as the test is written, at the time of the check we do not have a frame yet. This is in spite of the fact that I'm flushing layout at the beginning of VisualViewportSize().

Note, I've confirmed that we reach this line, and that it's being called for the child document.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I am unsure what the right behavior of iframe loading in terms of specs, but with debugging the test, the test runs before we start calling PresShell::BeginLoad() for the iframe's presShell so that we don't yet call PresShell::Initialize() at the moment, thus we have no root frame for the presShell.

I repeat I am unsure what the right behavior is, but it seems to me that the test script should be inside the iframe or the test should wait for onload event of the iframe window. Anyways, I think Emilio can respond the question. :)

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Yeah, so the about blank content viewer gets a pres shell, but the document has no parser or sink, so we never get here:

https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/dom/base/Document.cpp#7301

Which means that we never call PresShell::Initialize in that shell, and thus we create no frames and such.

Boris, is this intentional? Should the "if this is a style or layout flush make sure the shell is initialized" be separate from the content sink flushes?

In particular, should we do something like:

diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
index 72723d4ef3e6..30dab9d42676 100644
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -7295,9 +7295,8 @@ void Document::FlushPendingNotifications(mozilla::ChangesToFlush aFlush) {
   // make sure that layout is started as needed.  But we can skip that
   // part if we have no presshell or if it's already done an initial
   // reflow.
-  if ((!IsHTMLDocument() || (flushType > FlushType::ContentAndNotify &&
-                             mPresShell && !mPresShell->DidInitialize())) &&
-      (mParser || mWeakSink)) {
+  if (!IsHTMLDocument() || (flushType > FlushType::ContentAndNotify &&
+                            mPresShell && !mPresShell->DidInitialize())) {
     nsCOMPtr<nsIContentSink> sink;
     if (mParser) {
       sink = mParser->GetContentSink();
@@ -7309,8 +7308,14 @@ void Document::FlushPendingNotifications(mozilla::ChangesToFlush aFlush) {
     }
     // Determine if it is safe to flush the sink notifications
     // by determining if it safe to flush all the presshells.
-    if (sink && (flushType == FlushType::Content || IsSafeToFlush())) {
-      sink->FlushPendingNotifications(flushType);
+    if (sink) {
+      if (flushType == FlushType::Content || IsSafeToFlush()) {
+        sink->FlushPendingNotifications(flushType);
+      }
+    } else if (mPresShell && !mPresShell->DidInitialize() &&
+               flushType > FlushType::ContentAndNotify && IsSafeToFlush()) {
+      SetMayStartLayout(true);
+      mPresShell->Initialize();
     }
   }

That fixes the test, though I haven't run it through try or Talos. Right now, the pres shell we create for the about:blank viewer gets replaced shortly after without even getting initialized.

In any case, Botond, looks like this test is not wanting to test anything related to about:blank iframes or such, and probably can be rewritten to wait for the load event or such.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Boris, is this intentional?

Nothing about this stuff is "intentional"... ;)

We're talking about the transient initial about:blank, right? It's created, then an about:blank load also starts which will in fact have a parser/sink, etc. The code is running before that load has had a chance to become the current document. For extra fun, per spec that load is not supposed to exist.

But yes, the fact that we never start layout for initial about:blank can be a problem. We had a bug about something like var w = window.open(); w.stop() leading to a window in which DOM elements that get injected (e.g. via innerHTML) don't render, but I can't find it right now.

In particular, should we do something like:

That seems pretty reasonable, though it still wouldn't help with the scenario I describe above, unless something does a layout flush.

Another option is to have CreateAboutBlankContentViewer call SetMayStartLayout(true) on the document before we do the presshell setup. That runs the risk of doing extra layout work on the transient about:blank, though...

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1477610 - Run Visual Viewport web platform tests with APZ zooming prefs enabled. r=kats

This causes some scrollbar-related assertions to fail on desktop because
scrollbar behaviour with desktop zooming is not correct yet.

Depends on D29089

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1477610 - Make sure a resize during page load doesn't get mis-identified as a resize caused by a subsequent layout change. r=hiro

Depends on D30415

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1477610 - Wait for the load event before running viewport-unscaled-size-iframe.html. r=hiro

This ensures that the iframe is loaded by the time we query its visual viewport size.

Depends on D30416

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1477610 - Update bug reference in disabled annotation of viewport-resize-event-on-load-overflowing-page.html. r=hiro

Depends on D30417

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f7cd5ebb041
Flush layout when reporting the visual viewport size via the Visual Viewport API. r=hiro
https://hg.mozilla.org/integration/autoland/rev/ca147aeec45e
Run Visual Viewport web platform tests with APZ zooming prefs enabled. r=kats
https://hg.mozilla.org/integration/autoland/rev/695609f420a1
Make sure a resize during page load doesn't get mis-identified as a resize caused by a subsequent layout change. r=hiro
https://hg.mozilla.org/integration/autoland/rev/a7ec57d41cbb
Wait for the load event before running viewport-unscaled-size-iframe.html. r=hiro
https://hg.mozilla.org/integration/autoland/rev/6d0c3c2fda71
Update bug reference in disabled annotation of viewport-resize-event-on-load-overflowing-page.html. r=hiro

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6f7cd5ebb041
https://hg.mozilla.org/mozilla-central/rev/ca147aeec45e
https://hg.mozilla.org/mozilla-central/rev/695609f420a1
https://hg.mozilla.org/mozilla-central/rev/a7ec57d41cbb
https://hg.mozilla.org/mozilla-central/rev/6d0c3c2fda71

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16880 for changes under testing/web-platform/tests

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged