Bug 167734 – [Cocoa] Would like modern API for setting the viewport configuration, overriding anything the webpage specifies

• Component: WebKit API
• Status: RESOLVED FIXED

WebView’s -_setFixedLayoutSize: and a few other methods allow clients such as UIKit to control the viewport parameters in a way that overrides anything done by the webpage. Clients that use WKWebView would like to have similar control. However, WKWebView’s _fixedLayoutSize property does not have the same effect.

[Radar WebKit Bug Importer]

<rdar://problem/30331795>

[Tim Horton]

Created attachment 367094 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 367094 [details]
Patch

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

> Source/WebCore/dom/Document.h:414
> +    ViewportArguments viewportArguments() const { return m_overrideViewportArguments ? *m_overrideViewportArguments : m_viewportArguments; }

valueOr? Or is it value_or()?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:494
> +    [viewportArgumentPairs enumerateKeysAndObjectsUsingBlock:makeBlockPtr([&] (NSString *key, NSString *value, BOOL* stop) {
> +        String keyString = key;
> +        String valueString = value;
> +        WebCore::setViewportFeature(viewportArguments, keyString, valueString, viewportFitEnabled, [] (WebCore::ViewportErrorCode, const String& errorMessage) {
> +            NSLog(@"-[WKWebView _setOverrideViewportArguments]: Error parsing viewport argument: %s", errorMessage.utf8().data());
> +        });
> +    }).get()];

It seems like an easy mistake to make with this SPI would be to use non-string values: @{ @"width" : @1000 }. What's going to happen there?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:511
> +    _page->setOverrideViewportArguments(viewportArgumentsFromDictionary(viewportArguments, _page->preferences().viewportFitEnabled()));

If preferences().viewportFitEnabled() changes after we've pushed these viewport arguments, they will not adjust, right? Should they? Actually, can viewportArguments contain @"viewport-fit"?

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 367094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367094&action=review
> 
> > Source/WebCore/dom/Document.h:414
> > +    ViewportArguments viewportArguments() const { return m_overrideViewportArguments ? *m_overrideViewportArguments : m_viewportArguments; }
> 
> valueOr? Or is it value_or()?

Sure!

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:494
> > +    [viewportArgumentPairs enumerateKeysAndObjectsUsingBlock:makeBlockPtr([&] (NSString *key, NSString *value, BOOL* stop) {
> > +        String keyString = key;
> > +        String valueString = value;
> > +        WebCore::setViewportFeature(viewportArguments, keyString, valueString, viewportFitEnabled, [] (WebCore::ViewportErrorCode, const String& errorMessage) {
> > +            NSLog(@"-[WKWebView _setOverrideViewportArguments]: Error parsing viewport argument: %s", errorMessage.utf8().data());
> > +        });
> > +    }).get()];
> 
> It seems like an easy mistake to make with this SPI would be to use
> non-string values: @{ @"width" : @1000 }. What's going to happen there?

A fair point! We should probably throw an exception.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:511
> > +    _page->setOverrideViewportArguments(viewportArgumentsFromDictionary(viewportArguments, _page->preferences().viewportFitEnabled()));
> 
> If preferences().viewportFitEnabled() changes after we've pushed these
> viewport arguments, they will not adjust, right?

Correct.

> Should they?

Probably? Though I bet we have this problem with <meta viewport> too. And API clients can just do things in the right order :( but it's true, it would be nice if it worked.

> Actually, can viewportArguments contain @"viewport-fit"?

Yes! (if viewportFitEnabled is YES)

[Simon Fraser (smfr)]

(In reply to Tim Horton from comment #4)
> (In reply to Simon Fraser (smfr) from comment #3)

> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:511
> > > +    _page->setOverrideViewportArguments(viewportArgumentsFromDictionary(viewportArguments, _page->preferences().viewportFitEnabled()));
> > 
> > If preferences().viewportFitEnabled() changes after we've pushed these
> > viewport arguments, they will not adjust, right?
> 
> Correct.
> 
> > Should they?
> 
> Probably? Though I bet we have this problem with <meta viewport> too. And
> API clients can just do things in the right order :( but it's true, it would
> be nice if it worked.
> 
> > Actually, can viewportArguments contain @"viewport-fit"?
> 
> Yes! (if viewportFitEnabled is YES)

Oh, viewportFitEnabled is not affected by the value of  @"viewport-fit", OK.

[Tim Horton]

Created attachment 367103 [details]
Patch

[Tim Horton]

> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:511
> > > +    _page->setOverrideViewportArguments(viewportArgumentsFromDictionary(viewportArguments, _page->preferences().viewportFitEnabled()));
> > 
> > If preferences().viewportFitEnabled() changes after we've pushed these
> > viewport arguments, they will not adjust, right?
> 
> Correct.
> 
> > Should they?
> 
> Probably? Though I bet we have this problem with <meta viewport> too. And
> API clients can just do things in the right order :( but it's true, it would
> be nice if it worked.

I'm not sure how to easily get this right, and it doesn't matter while this is SPI; I fixed all the other things.

[Simon Fraser (smfr)]

Comment on attachment 367103 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:508
> +    _overrideViewportArguments = adoptNS([viewportArguments copy]);

Should we deep-copy all the strings? They may be mutable and mutated by the caller after this.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 367103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367103&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:508
> > +    _overrideViewportArguments = adoptNS([viewportArguments copy]);
> 
> Should we deep-copy all the strings? They may be mutable and mutated by the
> caller after this.

We could also make it write-only like some of the other overrides, and not even store the dictionary at all (just the parsed ViewportArguments)

[Tim Horton]

Created attachment 367146 [details]
Patch

[Tim Horton]

Created attachment 367153 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367153 [details]
Patch

Clearing flags on attachment: 367153

Committed r244151: <https://trac.webkit.org/changeset/244151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

New API test TestWebKitAPI.WebKit.OverrideViewportArguments added in https://trac.webkit.org/changeset/244151 is failing on iOS Simulator Debug

https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3239/steps/run-api-tests/logs/stdio

TestWebKitAPI.WebKit.OverrideViewportArguments
        2019-04-10 17:41:45.743 TestWebKitAPI[3470:6770579] -[WKWebView _overrideViewportWithArguments:]: Error parsing viewport argument: Viewport argument key "garbage" not recognized and ignored.
        LEAK: 1 WebProcessPool
        LEAK: 1 WebPageProxy
        
        /Volumes/Data/slave/ios-simulator-12-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/OverrideViewportArguments.mm:54
        Expected equality of these values:
          1.
            Which is: 1
          [webView scrollView].zoomScale
            Which is: 0.25

[Shawn Roberts]

Any progress on this failure? It is failing every run on the bots.

[Tim Horton]

(In reply to Shawn Roberts from comment #15)
> Any progress on this failure? It is failing every run on the bots.

The test passes 100% of the time when run locally :(

[Tim Horton]

(In reply to Tim Horton from comment #16)
> (In reply to Shawn Roberts from comment #15)
> > Any progress on this failure? It is failing every run on the bots.
> 
> The test passes 100% of the time when run locally :(

Ah! It fails in debug. Will take a peek.