REGRESSION (Safari 16.3): Passing "noopener" as third argument opens tabs into new window

[RemyDemarest]

301fe8a

REGRESSION (Safari 16.3): Passing "noopener" as third argument opens tabs into new window
https://bugs.webkit.org/show_bug.cgi?id=259093
rdar://112086061

Reviewed by BJ Burg and Chris Dumez.

The changes herein bring WebKit up to the HTML specs for window.open().

First, this commit now parses the popup field which is specified in the specs but
was missing in our parser. We also start parsing the resizable field which is defined
in our API but was never parsed. Finally, we mark all other member fields of optional
rather than setting a default value for each of them.

Second, this adds the member fields wantsPopup and hasAdditionalFeatures:
hasAdditionalFeatures is serialized and sent to the UI Process to indicate that we
detected other strings than the ones we normally parse.

The field wantsPopup implements the algorithm described here:
https://html.spec.whatwg.org/multipage/nav-history-apis.html#apis-for-creating-and-navigating-browsing-contexts-by-name
aimed at determining whether we should display a popup or not. Its value is determined
in the following way:
  - features noopener and noreferrer do not count towards the tested feature string.
  - empty features (two commas in a row), strings made of spaces only do not towards
    the tested feature string. So spaces and commas don't make the string not empty.
  - every other words or characters are counted.

As such the first test to determine the value of wantsPopup is to check whether any
feature is defined or if the string contains additional features, the latter flag is
only set if the string contains anything but spaces and commas. This matches the
behavior from other browsers and prescribed by the specs.

Finally, despite being declared as nullable, the corresponding properties in WKWindowFeatures
could never be in fact nil; changing all member fields in WindowFeatures to std::optional
allows us to return nil in the corresponding properties when those features are missing.
We also add private properties for the corresponding WindowFeatures member fields.

These changes ostensibly fixes the issue in question since we needed the ability to check
for the absence of the window features. However, to be correct a browser should adopt the
new properties as well.

* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::createWindow):

* Source/WebCore/page/Chrome.cpp:
(WebCore::Chrome::createWindow):

* Source/WebCore/page/LocalDOMWindow.cpp:
(WebCore::LocalDOMWindow::createWindow):

* Source/WebCore/page/WindowFeatures.cpp:
(WebCore::WindowFeatures::wantsPopup const):
  Implements the specs algorithm to check if we need to get a popup. No features declared
  besides noopener and noreferrer means we do not want a popup.

(WebCore::parseWindowFeatures):
  Stop setting default values for the window features to avoid indicating that those features
  were declared in the string. Compute wantsPopup after we are done parsing the string.

(WebCore::setWindowFeature):
  Add a parse step for the popup feature and set the additional features when they are not empty
  strings or strings made of spaces.

(WebCore::parseDialogFeatures):
  Dialogs are always popups.

* Source/WebCore/page/WindowFeatures.h:
  Make all potential features optional. Add convenience member functions to use instead of
  using noopener and noreferrer directly.

(WebCore::WindowFeatures::wantsNoOpener const):
(WebCore::WindowFeatures::wantsNoReferrer const):

* Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in:
  Match the declaration above. Serialize the three new fields.

* Source/WebKit/UIProcess/API/C/WKPage.cpp:
(WKPageSetPageUIClient):
  Save the new fields in the window feature dictionary for legacy clients.

* Source/WebKit/UIProcess/API/Cocoa/WKWindowFeatures.mm:
(-[WKWindowFeatures menuBarVisibility]):
(-[WKWindowFeatures statusBarVisibility]):
(-[WKWindowFeatures toolbarsVisibility]):
(-[WKWindowFeatures allowsResizing]):
(-[WKWindowFeatures _wantsPopup]):
(-[WKWindowFeatures _hasAdditionalFeatures]):
(-[WKWindowFeatures _popup]):
(-[WKWindowFeatures _locationBarVisibility]):
(-[WKWindowFeatures _scrollbarsVisibility]):
(-[WKWindowFeatures _fullscreenDisplay]):
(-[WKWindowFeatures _dialogDisplay]):

* Source/WebKit/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:
  Check if the value is defined and return it as a boolean otherwise return nil.

* Source/WebKit/UIProcess/API/glib/WebKitWindowProperties.cpp:
  Convert optional features into glib API.

(webkitWindowPropertiesSetGeometry):
(webkitWindowPropertiesSetWantsPopup):

* Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:
(WebChromeClient::createWindow):
  Write the new fields in the window feature dictionary for legacy clients.
  Only write optional fields if they are not null.

* Tools/TestWebKitAPI/Tests/WebKitCocoa/OpenAndCloseWindow.mm:
(TEST(WebKit, OpenWindowFeatures)):
  - Reenable the tests checking for the nil case when the string is empty.
  - Add checks for the new fields in the API.
  - Add tests for noopener and noreferrer features.
  - Add test for hasAdditionalFeatures.
  - Add test for a string containing only separators.

Canonical link: https://commits.webkit.org/267354@main

0205dfc

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	⏳ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 68c577c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 api-gtk
	❌ 🛠 tv	❌ 🧪 mac-AS-debug-wk2
	❌ 🛠 tv-sim	❌ 🧪 mac-wk2-stress
	❌ 🛠 watch
	❌ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 7c41371)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	🧪 api-mac	❌ 🛠 gtk
	🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	🧪 mac-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv	🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 300c8ab)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	🧪 api-mac	❌ 🛠 gtk
	🧪 ios-wk2-wpt	🧪 mac-wk1	❌ 🧪 gtk-wk2
	🧪 api-ios	🧪 mac-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv	🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash ad3f715)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	❌ 🧪 api-mac	✅ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[mcatanzaro]

So WebKitWindowProperties is public API for GTK/WPE ports. There are special requirements for adding new APIs. We would additionally need (a) API tests (which may be simple) to ensure it doesn't break in the future, and (b) implementation in at least one client, e.g. GTK MiniBrowser, since having unused APIs is not useful, and (c) approval from two GTK/WPE reviewers in addition to any other approvals.

It's a lot more than is required for most changes in WebKit, and I wonder if perhaps you did not intend to stumble into this when trying to fix a Safari bug. :) The changes do look OK, but maybe you would prefer to not deal with all the extra work? If so, one option would be to just drop the changes to WebKitWindowProperties and leave the new properties unexposed. Another option is to report a bug for somebody to follow-up in the future if desired, attach a patch there, and remove the changes from this pull request.

If we do want to add these, then I have some additional questions. (a) What are the differences between wants-popup and popup properties? It needs to be documented with greater detail. (b) Is has-additional-features really useful? I can't think of anything that an application would ever be able to do with this. Maybe we shouldn't expose that one?

Also, all the new properties and methods would need to have Since tags to indicate the first available release. 2.42 has branched so 2.44 is the next stable branch. It would look like:

    /**
     * WebKitWindowProperties:wants-popup:
     *
     * Whether to show a popup based on HTML specs.
     *
     * Since: 2.44
     */

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 7a3fe38)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	🧪 gtk-wk2
⏳ 🛠 🧪 jsc	🧪 api-ios	🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[RemyDemarest]

Thank you for the comment, I will take your proposal to retract the API changes and just make sure the existing APIs work properly with the new code.

But for posterity I'll answer those questions:

If we do want to add these, then I have some additional questions. (a) What are the differences between wants-popup and popup properties? It needs to be documented with greater detail.

The difference between wantsPopup and popup is that popup is a feature you can add to your WindowFeatures argument in window.open(), whereas wantsPopup is the result of computing whether we should display a popup based on the features in that argument. If popup is defined in that list of features, wantsPopup will be whatever value popup is, otherwise wantsPopup will be true if both toolBarVisible and locationBarVisible are false or menuBarVisible is false, or resizable is false, or scrollbarsVisible is false, or statusBarVisible is false. This is described in wantsPopupForWindowFeatures() in WindowFeatures.cpp. In addition, the specs say we should not show a popup if the string is empty, but it doesn't mention noopener and noreferrer, so for simplicity I interpret "empty string" as "nothing is defined besides noopener or noreferrer" and check that none of the properties were set and return false early.

This is based on the specs at https://html.spec.whatwg.org/multipage/nav-history-apis.html#apis-for-creating-and-navigating-browsing-contexts-by-name

(b) Is has-additional-features really useful? I can't think of anything that an application would ever be able to do with this. Maybe we shouldn't expose that one?

Regarding, hasAdditionalFeatures the reason to have it is that other browsers say that if they see any feature defined that is not parsed they decide to show a popup anyway, this is to allow us to show a popup in that condition as well.

[webkit-early-warning-system]

EWS run on previous version of this PR (hash d86bcf5)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	❌ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	❌ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 951116f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

[cdumez]

LGTM

[webkit-early-warning-system]

EWS run on previous version of this PR (hash b86b81b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	✅ 🛠 wpe	🛠 wincairo
✅ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	✅ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk2	🧪 api-gtk
	❌ 🛠 tv	❌ 🧪 mac-AS-debug-wk2
	⏳ 🛠 tv-sim	❌ 🧪 mac-wk2-stress
	⏳ 🛠 watch
	❌ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on current version of this PR (hash 0205dfc)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	⏳ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-commit-queue]

Committed 267354@main (301fe8a): https://commits.webkit.org/267354@main

Reviewed commits have been landed. Closing PR #16857 and removing active labels.