Bug 167734

Summary: [Cocoa] Would like modern API for setting the viewport configuration, overriding anything the webpage specifies
Product: WebKit Reporter: mitz
Component: WebKit APIAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dbates, dino, esprehn+autocc, ews-watchlist, kangil.han, simon.fraser, sroberts, thorton, webkit-bot-watchers-bugzilla, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description mitz 2017-02-02 08:45:14 PST
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.
Comment 1 Radar WebKit Bug Importer 2017-02-02 10:54:57 PST
<rdar://problem/30331795>
Comment 2 Tim Horton 2019-04-09 18:46:15 PDT
Created attachment 367094 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-04-09 19:04:09 PDT
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"?
Comment 4 Tim Horton 2019-04-09 19:24:53 PDT
(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)
Comment 5 Simon Fraser (smfr) 2019-04-09 19:46:26 PDT
(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.
Comment 6 Tim Horton 2019-04-09 22:37:35 PDT
Created attachment 367103 [details]
Patch
Comment 7 Tim Horton 2019-04-09 22:40:21 PDT
> > > 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.
Comment 8 Simon Fraser (smfr) 2019-04-10 09:53:54 PDT
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.
Comment 9 Tim Horton 2019-04-10 11:15:41 PDT
(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)
Comment 10 Tim Horton 2019-04-10 12:32:08 PDT
Created attachment 367146 [details]
Patch
Comment 11 Tim Horton 2019-04-10 13:21:13 PDT
Created attachment 367153 [details]
Patch
Comment 12 WebKit Commit Bot 2019-04-10 14:33:04 PDT
Comment on attachment 367153 [details]
Patch

Clearing flags on attachment: 367153

Committed r244151: <https://trac.webkit.org/changeset/244151>
Comment 13 WebKit Commit Bot 2019-04-10 14:33:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Shawn Roberts 2019-04-15 10:45:39 PDT
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
Comment 15 Shawn Roberts 2019-04-17 11:27:12 PDT
Any progress on this failure? It is failing every run on the bots.
Comment 16 Tim Horton 2019-04-17 12:53:57 PDT
(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 :(
Comment 17 Tim Horton 2019-04-17 14:03:13 PDT
(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.