Bug 179385 – Implement VisualViewport API attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ali Juma]

Implement the attributes defined by the VisualViewport API: https://wicg.github.io/visual-viewport/#the-visualviewport-interface

[Ali Juma]

Created attachment 326250 [details]
Patch

[Ali Juma]

Comment on attachment 326250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326250&action=review

> Source/WebCore/page/VisualViewport.idl:28
> +EnabledBySetting=VisualViewport

Should this be using a new setting, or is it ok to use this existing VisualViewport setting?

[Simon Fraser (smfr)]

I think it should be a new Experimental Feature (VisualViewportAPI).

You should update features.json so https://webkit.org/status/#?search=viewport says "Under development".

[Build Bot]

Comment on attachment 326250 [details]
Patch

Attachment 326250 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5143035

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/service-worker-csp-default.https.html

[Build Bot]

Created attachment 326302 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Ali Juma]

Created attachment 326342 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 326342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326342&action=review

Thanks for working on this Ali! This looks good to me. I have a few comments inline.

> Source/WebCore/page/DOMWindow.cpp:811
> +{

Should we return null when !isCurrentlyDisplayedInFrame()? That's what other APIs do...

If so, it should be mentioned in the spec and we should have a WPT test for that case.

> Source/WebCore/page/DOMWindow.idl:137
> +    [EnabledBySetting=VisualViewportAPI, Replaceable] readonly attribute VisualViewport visualViewport; // FIXME: Should be [SameObject].

In general, we open follow-up bugs and mention them in FIXME comments.

> Source/WebCore/page/VisualViewport.cpp:135
> +

It looks like you can introduce a helper function that returns a FrameView* or an optional<FrameView> and performs the updateLayoutIgnorePendingStylesheets if the result is non-null. Maybe it can even ASSERT that m_frame->pageZoomFactor() is non-null. Then you can use it in the functions offsetTop, offsetLeft, pageTop, pageLeft, width and height above, and avoid some code duplication.

> Source/WebCore/page/VisualViewport.cpp:142
> +        return 1;

Can you please explain why we check the isMainFrame() condition? It seems page() is also available for non-main frame and the special case is not obvious to me from https://wicg.github.io/visual-viewport/#dom-visualviewport-scale. Maybe add a comment? And a test?

> Source/WebCore/page/VisualViewport.cpp:147
> +

Not sure we will re-use the above code in the future, but similarly you might consider introducing a helper function to get the Page* / optional<Page> and do the updateLayoutIgnorePendingStylesheets call.

> LayoutTests/ChangeLog:21
> +        * platform/gtk/TestExpectations:

Please mention that you skip the GTK tests because of the lack of UIScriptController::zoomToScale. We should really have a bug entry for that and have it mentioned here and in TestExpectations...

> LayoutTests/ChangeLog:25
> +        * platform/ios/imported/w3c/web-platform-tests/viewport/viewport-unscaled-size-iframe-expected.txt: Added.

I suppose it's on purpose that not as many tests pass on iOS as in macOS. Again, this need to be explained in the ChangeLog and we should have follow-up bug to handle that.

> LayoutTests/ChangeLog:26
> +        * platform/mac-wk1/TestExpectations:

I guess you should repeat the explanation for Mac WK1 here too.

> LayoutTests/imported/w3c/ChangeLog:8
> +        Update expectations for viewport WPTs that now pass.

Again, this does not seem to true for iOS as you added expected failures. (Hopefully, since they don't use UIScriptController::zoomToScale they will pass on GTK too!)

[Ali Juma]

Created attachment 327105 [details]
Patch

Addressed review comments.

[Ali Juma]

Comment on attachment 326342 [details]
Patch

Thanks for the detailed review!

View in context: https://bugs.webkit.org/attachment.cgi?id=326342&action=review

>> Source/WebCore/page/DOMWindow.cpp:811
>> +{
> 
> Should we return null when !isCurrentlyDisplayedInFrame()? That's what other APIs do...
> 
> If so, it should be mentioned in the spec and we should have a WPT test for that case.

Thanks for pointing this out! I opened a spec issue to clarify this (https://github.com/WICG/visual-viewport/issues/51) and it sounds like this is going to get added to the spec. I'll make the change here after the spec gets updated.

>> Source/WebCore/page/DOMWindow.idl:137
>> +    [EnabledBySetting=VisualViewportAPI, Replaceable] readonly attribute VisualViewport visualViewport; // FIXME: Should be [SameObject].
> 
> In general, we open follow-up bugs and mention them in FIXME comments.

Added a link to the bug for implementing SameObject.

>> Source/WebCore/page/VisualViewport.cpp:135
>> +
> 
> It looks like you can introduce a helper function that returns a FrameView* or an optional<FrameView> and performs the updateLayoutIgnorePendingStylesheets if the result is non-null. Maybe it can even ASSERT that m_frame->pageZoomFactor() is non-null. Then you can use it in the functions offsetTop, offsetLeft, pageTop, pageLeft, width and height above, and avoid some code duplication.

Done.

>> Source/WebCore/page/VisualViewport.cpp:142
>> +        return 1;
> 
> Can you please explain why we check the isMainFrame() condition? It seems page() is also available for non-main frame and the special case is not obvious to me from https://wicg.github.io/visual-viewport/#dom-visualviewport-scale. Maybe add a comment? And a test?

This is covered by fast/visual-viewport/viewport-dimensions-iframe.html as well as by a manual WPT (viewport-scale-iframe-manual.html). But I agree that this requirement isn't clear from the spec language, so I've opened a spec issue (https://github.com/WICG/visual-viewport/issues/52) to get the spec language clarified.

>> Source/WebCore/page/VisualViewport.cpp:147
>> +
> 
> Not sure we will re-use the above code in the future, but similarly you might consider introducing a helper function to get the Page* / optional<Page> and do the updateLayoutIgnorePendingStylesheets call.

Left this as-is for now, I think makes sense to add a helper once we need to reuse this code.

>> LayoutTests/ChangeLog:21
>> +        * platform/gtk/TestExpectations:
> 
> Please mention that you skip the GTK tests because of the lack of UIScriptController::zoomToScale. We should really have a bug entry for that and have it mentioned here and in TestExpectations...

Added comment mentioning the existing bug. The bug number (webkit.org/b/168050) is already mentioned in TestExpectations :)

>> LayoutTests/ChangeLog:25
>> +        * platform/ios/imported/w3c/web-platform-tests/viewport/viewport-unscaled-size-iframe-expected.txt: Added.
> 
> I suppose it's on purpose that not as many tests pass on iOS as in macOS. Again, this need to be explained in the ChangeLog and we should have follow-up bug to handle that.

Added comments in the ChangeLog. The three iframe-related tests fail because iframes aren't scrollable on iOS -- filed a bug for these. The other test (viewport-unscaled-size) has a PASS expectation, but the numbers in the output are different just because of the different window size, so that doesn't need any follow-up.

>> LayoutTests/ChangeLog:26
>> +        * platform/mac-wk1/TestExpectations:
> 
> I guess you should repeat the explanation for Mac WK1 here too.

Done.

>> LayoutTests/imported/w3c/ChangeLog:8
>> +        Update expectations for viewport WPTs that now pass.
> 
> Again, this does not seem to true for iOS as you added expected failures. (Hopefully, since they don't use UIScriptController::zoomToScale they will pass on GTK too!)

Updated the comment, and added comments below about failures.

[Frédéric Wang (:fredw)]

Comment on attachment 327105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327105&action=review

Thanks for addressing the review comments. This basically LGTM, but I prefer to wait a bit the feedback from other more experienced reviewers as well as the changes in the spec wording. I'm happy to r+ the changes to features.json now though. I have some minor comments inline.

> Source/WebCore/features.json:1171
>      },

If you want this part right now so the status is displayed on https://webkit.org/status/#?search=viewport ; you can submit a separate patch and I'll r+ it.

> Source/WebCore/page/DOMWindow.cpp:806
> +{

OK, I think you need to add the isCurrentlyDisplayedInFrame check per https://github.com/WICG/visual-viewport/issues/51#issuecomment-344990083

> Source/WebCore/page/VisualViewport.cpp:57
> +    auto* view = frame? frame->view() : nullptr;

nit: miss a space before the question mark

> Source/WebCore/page/VisualViewport.cpp:69
> +        return 0.0;

nit: I would personally do 

if (auto* view = getFrameViewAndLayoutIfNonNull(m_frame))
    return (...);
return 0.0;

to be slightly more concise.

> Source/WebCore/page/VisualViewport.cpp:121
> +    if (!m_frame || !m_frame->isMainFrame())

Let's wait the reply on https://github.com/WICG/visual-viewport/issues/52 and add a comment if necessary.

> Tools/ChangeLog:11
> +        (enableExperimentalFeatures):

It is weird that this is only done for mac and only DumpRenderTree. I think there are other places to modify, for example InjectedBundle::beginTesting in Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp ; maybe you can compare with what was done for other similar experimental features.

> LayoutTests/ChangeLog:24
> +          Fails because iframes aren't scrollable on iOS (webkit.org/b/179794).

OK, I heard about that bug ;-)

> LayoutTests/fast/visual-viewport/viewport-dimensions-exclude-custom-scrollbars.html:29
> +        internals.settings.setVisualViewportEnabled(true);

Is it necessary to explicitly set this? IIUC you don't do that for WPT tests. Again maybe check what is done for other prefs...

[Ali Juma]

Created attachment 327202 [details]
Patch

Addressed comments.

[Ali Juma]

Comment on attachment 327105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327105&action=review

>> Source/WebCore/features.json:1171
>>      },
> 
> If you want this part right now so the status is displayed on https://webkit.org/status/#?search=viewport ; you can submit a separate patch and I'll r+ it.

Thanks, done in bug 179818.

>> Source/WebCore/page/DOMWindow.cpp:806
>> +{
> 
> OK, I think you need to add the isCurrentlyDisplayedInFrame check per https://github.com/WICG/visual-viewport/issues/51#issuecomment-344990083

Done.

>> Source/WebCore/page/VisualViewport.cpp:57
>> +    auto* view = frame? frame->view() : nullptr;
> 
> nit: miss a space before the question mark

Oops, fixed.

>> Source/WebCore/page/VisualViewport.cpp:69
>> +        return 0.0;
> 
> nit: I would personally do 
> 
> if (auto* view = getFrameViewAndLayoutIfNonNull(m_frame))
>     return (...);
> return 0.0;
> 
> to be slightly more concise.

Done.

>> Tools/ChangeLog:11
>> +        (enableExperimentalFeatures):
> 
> It is weird that this is only done for mac and only DumpRenderTree. I think there are other places to modify, for example InjectedBundle::beginTesting in Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp ; maybe you can compare with what was done for other similar experimental features.

I compared to what's done for LinkPreload and WebAnimations. Neither of those is set in InjectedBundle::beginTesting. I think this isn't needed since for WebKitTestRunner, TestController::resetPreferencesToConsistentValues calls WKPreferencesEnableAllExperimentalFeatures which in turn calls WebPreferences::enableAllExperimentalFeatures (defined in the generated file DerivedSources/WebKit2/WebPreferencesExperimentalFeatures.cpp). But I'd missed updating DumpRenderTree.cpp for Windows; done now.

>> LayoutTests/fast/visual-viewport/viewport-dimensions-exclude-custom-scrollbars.html:29
>> +        internals.settings.setVisualViewportEnabled(true);
> 
> Is it necessary to explicitly set this? IIUC you don't do that for WPT tests. Again maybe check what is done for other prefs...

Note that this isn't setting the new experimental VisualViewportAPI pref; instead, it's setting the existing pref for enabling visual viewports. Visual viewports are already enabled by default on WK2 and on mac-WK1, so this only makes a difference on non-mac WK1 ports. So I guess the question is whether it's worth trying to run these tests on such ports (i.e., whether any such port ever intends to ship visual viewports, which is a prerequisite for the Visual Viewport API). If not, we can remove these lines. The existing tests in fast/visual-viewport do have these lines though.

For WPTs, we can't add this even if we wanted to (since we can't add WebKit internals calls to cross-browser tests).

[Ali Juma]

Created attachment 327254 [details]
Patch

Rebased

[Frédéric Wang (:fredw)]

Comment on attachment 327254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327254&action=review

OK, thanks for the clarification regarding tests / settings.

It looks like the win port does not build anymore :-(

> Source/WebCore/page/VisualViewport.cpp:69
> +    return 0.0;

Sorry, I forgot about that one: https://webkit.org/code-style-guidelines/#floating-point-literals

[Ali Juma]

Created attachment 327425 [details]
Patch

[Ali Juma]

(In reply to Frédéric Wang (:fredw) from comment #14)
> Comment on attachment 327254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327254&action=review
> 
> OK, thanks for the clarification regarding tests / settings.
> 
> It looks like the win port does not build anymore :-(

I'd missed a few win-specific files that needed to be modified when adding a new setting. Hopefully fixed now.


> > Source/WebCore/page/VisualViewport.cpp:69
> > +    return 0.0;
> 
> Sorry, I forgot about that one:
> https://webkit.org/code-style-guidelines/#floating-point-literals

Thanks, fixed.


Also, added a comment to VisualViewport::scale based on the response to https://github.com/WICG/visual-viewport/issues/52

[Ali Juma]

Created attachment 327426 [details]
Patch

[Ali Juma]

Created attachment 327434 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 327434 [details]
Patch

OK, we got the clarification from spec authors and no complaints from other reviewers, so I think we can take this.

[WebKit Commit Bot]

Comment on attachment 327434 [details]
Patch

Clearing flags on attachment: 327434

Committed r225093: <https://trac.webkit.org/changeset/225093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35664217>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 179938

[Frédéric Wang (:fredw)]

@Ali: I reverted the patch because compilation failed on WinCairo Release Build (for some reason the EWS was green). Can you please check it? Maybe we should ask to the Sony people.

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/8578/steps/compile-webkit/logs/stdio

[Don Olmstead]

(In reply to Frédéric Wang (:fredw) from comment #24)
> @Ali: I reverted the patch because compilation failed on WinCairo Release
> Build (for some reason the EWS was green). Can you please check it? Maybe we
> should ask to the Sony people.
> 
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/8578/
> steps/compile-webkit/logs/stdio

I did a clean build with Ali's patch and I did not see the failure. I have a feeling that one of the generated files was stale.

I'm ok with landing this again and then if necessary forcing a clean build on the WinCairo bots. I would assume the Apple ones might need this as well.

[Ali Juma]

Created attachment 327470 [details]
Patch for re-landing

Rebased

[WebKit Commit Bot]

Comment on attachment 327470 [details]
Patch for re-landing

Rejecting attachment 327470 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 327470, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Frederic Wang found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/5333089

[Ali Juma]

Created attachment 327471 [details]
Patch for re-landing

Added accents to reviewer's name :)

[WebKit Commit Bot]

Comment on attachment 327471 [details]
Patch for re-landing

Clearing flags on attachment: 327471

Committed r225103: <https://trac.webkit.org/changeset/225103>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

I did a clean build for our WinCairo buildbots and that seems to have done the trick. The build is green after this patch landed.

[Frédéric Wang (:fredw)]

(In reply to Don Olmstead from comment #31)
> I did a clean build for our WinCairo buildbots and that seems to have done
> the trick. The build is green after this patch landed.

Thanks for taking care of this!

[Don Olmstead]

(In reply to Frédéric Wang (:fredw) from comment #32)
> (In reply to Don Olmstead from comment #31)
> > I did a clean build for our WinCairo buildbots and that seems to have done
> > the trick. The build is green after this patch landed.
> 
> Thanks for taking care of this!

No problem!

The patch was always fine it looks like there were stale files being included. I opened https://bugs.webkit.org/show_bug.cgi?id=179960 to figure out what happened.