WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
174239
WKWindowFeatures needs to expose scrollbar, fullscreen, and dialog configuration properties
https://bugs.webkit.org/show_bug.cgi?id=174239
Summary
WKWindowFeatures needs to expose scrollbar, fullscreen, and dialog configurat...
Ansh Shukla
Reported
2017-07-06 21:17:17 PDT
Safari needs this to adopt new createPage API.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2017-07-06 21:17:44 PDT
<
rdar://problem/33174155
>
Ansh Shukla
Comment 2
2017-07-06 22:49:50 PDT
Created
attachment 314819
[details]
Patch
Brady Eidson
Comment 3
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.
Ansh Shukla
Comment 4
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.
Brady Eidson
Comment 5
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.
Ansh Shukla
Comment 6
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.
Ansh Shukla
Comment 7
2017-07-07 14:03:37 PDT
Created
attachment 314874
[details]
Patch
Brady Eidson
Comment 8
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.
Brady Eidson
Comment 9
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
Ansh Shukla
Comment 10
2017-07-11 13:48:51 PDT
Test unearthed:
https://bugs.webkit.org/show_bug.cgi?id=174388
Ansh Shukla
Comment 11
2017-07-11 14:00:16 PDT
Created
attachment 315163
[details]
Patch
Brady Eidson
Comment 12
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
Ansh Shukla
Comment 13
2017-07-11 16:09:34 PDT
Created
attachment 315181
[details]
Patch
Brady Eidson
Comment 14
2017-07-11 16:51:13 PDT
Comment on
attachment 315181
[details]
Patch r=me once EWS is happy
WebKit Commit Bot
Comment 15
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
>
WebKit Commit Bot
Comment 16
2017-07-11 17:25:50 PDT
All reviewed patches have been landed. Closing bug.
Brady Eidson
Comment 17
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug