Bug 174239 – WKWindowFeatures needs to expose scrollbar, fullscreen, and dialog configuration properties

• Component: WebKit API
• Status: RESOLVED FIXED

[Ansh Shukla]

Safari needs this to adopt new createPage API.

[Radar WebKit Bug Importer]

<rdar://problem/33174155>

[Ansh Shukla]

Created attachment 314819 [details]
Patch

[Brady Eidson]

Comment on attachment 314819 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:37
> +@property (nonatomic, readonly, getter=_isLocationBarVisible) BOOL _locationBarVisible WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> +@property (nonatomic, readonly, getter=_areScrollbarsVisible) BOOL _scrollbarsVisible WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> +@property (nonatomic, readonly, getter=_isFullscreen) BOOL _fullscreen WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> +@property (nonatomic, readonly, getter=_isDialog) BOOL _dialog WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

These should be exposed as NSNumber *s the same way the API bool properties are.

[Ansh Shukla]

Comment on attachment 314819 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:37
>> +@property (nonatomic, readonly, getter=_isDialog) BOOL _dialog WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> These should be exposed as NSNumber *s the same way the API bool properties are.

width, height, x, and y are the only API::WindowFeature properties that are optional. All others are always specified, and the documentation on the public API is incorrect because, for example, menuBarVisibility can never be nil. If I'm right, then I don't think we should bring this mistake into the private API. I can change these to NSNumber * if I'm misinterpreting when values can be unspecified or if you think it's better to keep it consistent regardless.

[Brady Eidson]

(In reply to Ansh Shukla from comment #4)
> Comment on attachment 314819 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314819&action=review
> 
> >> Source/WebKit2/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:37
> >> +@property (nonatomic, readonly, getter=_isDialog) BOOL _dialog WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > These should be exposed as NSNumber *s the same way the API bool properties are.
> 
> width, height, x, and y are the only API::WindowFeature properties that are
> optional. 

This is *currently* true for the WebCore::WindowFeatures object, but it's not actually true for the web API itself.

> All others are always specified

This is not true. You can call window.open with *no* window features string.

https://html.spec.whatwg.org/multipage/window-object.html#dom-open
https://html.spec.whatwg.org/multipage/window-object.html#window-open-steps
https://html.spec.whatwg.org/multipage/window-object.html#concept-window-open-features-tokenize

Or more casually:
https://developer.mozilla.org/en-US/docs/Web/API/Window/open
"An optional parameter listing the features (size, position, scrollbars, etc.) of the new window as a DOMString."

> The documentation on the public API is incorrect because, for example, menuBarVisibility can never be
> nil. 

The documentation on the public API is correct. Our implementation is wrong.

> I can change these to NSNumber * if I'm misinterpreting when
> values can be unspecified or if you think it's better to keep it consistent
> regardless.

Please correctly make the new SPI represent the optionality of these features.

I filed https://bugs.webkit.org/show_bug.cgi?id=174271 to fix the implementation.

[Ansh Shukla]

(In reply to Brady Eidson from comment #5)
> 
> Or more casually:
> https://developer.mozilla.org/en-US/docs/Web/API/Window/open
> "An optional parameter listing the features (size, position, scrollbars,
> etc.) of the new window as a DOMString."

I saw this page, but the later statement

"the features that are not listed will be disabled or removed (except titlebar and close, which are by default yes)"

led me to believe that WebCore::WindowFeatures was intentionally reverting to defaults for unspecified values. Since it seems that our implementation is broken, I'll upload a patch with optional properties.

[Ansh Shukla]

Created attachment 314874 [details]
Patch

[Brady Eidson]

(In reply to Ansh Shukla from comment #6)
> (In reply to Brady Eidson from comment #5)
> > 
> > Or more casually:
> > https://developer.mozilla.org/en-US/docs/Web/API/Window/open
> > "An optional parameter listing the features (size, position, scrollbars,
> > etc.) of the new window as a DOMString."
> 
> I saw this page, but the later statement
> 
> "the features that are not listed will be disabled or removed (except
> titlebar and close, which are by default yes)"
> 
> led me to believe that WebCore::WindowFeatures was intentionally reverting
> to defaults for unspecified values. Since it seems that our implementation
> is broken, I'll upload a patch with optional properties.

The spec has recommended default values.

But you're confusing the expectations of the window with what the API represents, which is whether or not the value was specified at all.

Some examples:

Javascript: 
window.open("about:blank", "_top", "toolbar=yes");
API WKWindowFeatures.toolbarsVisibility:
[NSNumber numberWithBool:YES]

Javascript: 
window.open("about:blank", "_top", "toolbar=no");
API WKWindowFeatures.toolbarsVisibility:
[NSNumber numberWithBool:NO]

Javascript: 
window.open("about:blank", "_top");
API WKWindowFeatures.toolbarsVisibility:
nil


Note: This doesn't actually work today as established - that's what the other bug covers.

[Brady Eidson]

Comment on attachment 314874 [details]
Patch

API test?

You can add on to the API test OpenAndCloseWindow.mm

[Ansh Shukla]

Test unearthed: https://bugs.webkit.org/show_bug.cgi?id=174388

[Ansh Shukla]

Created attachment 315163 [details]
Patch

[Brady Eidson]

/Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/OpenAndCloseWindow.mm:237:91: error: use of undeclared identifier 'NSWindowStyleMaskBorderless'
    auto window = adoptNS([[NSWindow alloc] initWithContentRect:[webView frame] styleMask:NSWindowStyleMaskBorderless backing:NSBackingStoreBuffered defer:YES]);
                                                                                          ^
1 error generated.

Can't use 10.12 SDK features as long as WebKit has to build on 10.11

[Ansh Shukla]

Created attachment 315181 [details]
Patch

[Brady Eidson]

Comment on attachment 315181 [details]
Patch

r=me once EWS is happy

[WebKit Commit Bot]

Comment on attachment 315181 [details]
Patch

Clearing flags on attachment: 315181

Committed r219378: <http://trac.webkit.org/changeset/219378>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

(In reply to Brady Eidson from comment #14)
> Comment on attachment 315181 [details]
> Patch
> 
> r=me once EWS is happy

(In reply to WebKit Commit Bot from comment #15)
> Comment on attachment 315181 [details]> 
> Clearing flags on attachment: 315181
> 
> Committed r219378: <http://trac.webkit.org/changeset/219378>

I notice this landed before EWS was all the way done.

While I think this particular patch was fine as-is, it's not usually acceptable to disregard reviewer feedback like this.