Bug 174239 - WKWindowFeatures needs to expose scrollbar, fullscreen, and dialog configuration properties
Summary: WKWindowFeatures needs to expose scrollbar, fullscreen, and dialog configurat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-06 21:17 PDT by Ansh Shukla
Modified: 2017-07-11 22:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.83 KB, patch)
2017-07-06 22:49 PDT, Ansh Shukla
no flags Details | Formatted Diff | Diff
Patch (8.86 KB, patch)
2017-07-07 14:03 PDT, Ansh Shukla
no flags Details | Formatted Diff | Diff
Patch (15.39 KB, patch)
2017-07-11 14:00 PDT, Ansh Shukla
no flags Details | Formatted Diff | Diff
Patch (15.16 KB, patch)
2017-07-11 16:09 PDT, Ansh Shukla
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ansh Shukla 2017-07-06 21:17:17 PDT
Safari needs this to adopt new createPage API.
Comment 1 Radar WebKit Bug Importer 2017-07-06 21:17:44 PDT
<rdar://problem/33174155>
Comment 2 Ansh Shukla 2017-07-06 22:49:50 PDT
Created attachment 314819 [details]
Patch
Comment 3 Brady Eidson 2017-07-07 11:32:42 PDT
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.
Comment 4 Ansh Shukla 2017-07-07 12:44:43 PDT
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.
Comment 5 Brady Eidson 2017-07-07 13:00:31 PDT
(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.
Comment 6 Ansh Shukla 2017-07-07 13:32:11 PDT
(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.
Comment 7 Ansh Shukla 2017-07-07 14:03:37 PDT
Created attachment 314874 [details]
Patch
Comment 8 Brady Eidson 2017-07-07 14:31:05 PDT
(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.
Comment 9 Brady Eidson 2017-07-07 14:33:12 PDT
Comment on attachment 314874 [details]
Patch

API test?

You can add on to the API test OpenAndCloseWindow.mm
Comment 10 Ansh Shukla 2017-07-11 13:48:51 PDT
Test unearthed: https://bugs.webkit.org/show_bug.cgi?id=174388
Comment 11 Ansh Shukla 2017-07-11 14:00:16 PDT
Created attachment 315163 [details]
Patch
Comment 12 Brady Eidson 2017-07-11 15:53:18 PDT
/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
Comment 13 Ansh Shukla 2017-07-11 16:09:34 PDT
Created attachment 315181 [details]
Patch
Comment 14 Brady Eidson 2017-07-11 16:51:13 PDT
Comment on attachment 315181 [details]
Patch

r=me once EWS is happy
Comment 15 WebKit Commit Bot 2017-07-11 17:25:49 PDT
Comment on attachment 315181 [details]
Patch

Clearing flags on attachment: 315181

Committed r219378: <http://trac.webkit.org/changeset/219378>
Comment 16 WebKit Commit Bot 2017-07-11 17:25:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Brady Eidson 2017-07-11 22:28:10 PDT
(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.