Enable all web platform tests for the Visual Viewport API

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
19 days ago

People

(Reporter: tanushree, Assigned: botond)

Tracking

(Blocks 1 bug)

62 Branch
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [gfx-noted][wptsync upstream])

Attachments

(5 attachments)

Reporter

Description

11 months ago
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.
Reporter

Updated

11 months ago
Assignee: nobody → tpodder
Blocks: 1357785
Priority: -- → P3
Whiteboard: [gfx-noted]
Reporter

Updated

10 months ago
Assignee: tanushree.podder.103 → nobody
Depends on: 1515199
Changing title to re-use this bug as a tacker bug.
Summary: Enable Web Platform tests for the Visual Viewport API → Enable all web platform tests for the Visual Viewport API
Assignee

Updated

2 months ago
Assignee: nobody → botond
Mentor: botond
Assignee

Comment 3

2 months ago

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

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

Blocks: 1546387
Assignee

Comment 4

2 months ago

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?

Assignee

Comment 5

2 months ago

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

Depends on: 1547827
Assignee

Comment 6

2 months ago

Needinfo for comment 4. Any suggestions are appreciated!

Flags: needinfo?(hikezoe)
Flags: needinfo?(emilio)
Assignee

Comment 7

2 months ago

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

Assignee

Comment 8

2 months ago

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

Flags: needinfo?(hikezoe)

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.

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

Flags: needinfo?(bzbarsky)
Assignee

Comment 12

2 months ago

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

Depends on D29089

Assignee

Comment 14

2 months ago

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

Depends on D30416

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

Comment 16

2 months ago
Pushed by bballo@mozilla.com:
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
Regressions: 1522443
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16880 for changes under testing/web-platform/tests
Whiteboard: [gfx-noted] → [gfx-noted][wptsync upstream]
Upstream PR merged
Regressions: 1550595
You need to log in before you can comment on or make changes to this bug.