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

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

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?

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

Needinfo for comment 4. Any suggestions are appreciated!

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

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

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. :)

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.

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

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

Depends on D29089

Depends on D30415

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

Depends on D30416

Depends on D30417

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