Skip to content

Commit

Permalink
Add "true" to truthy values list for windowFeatures
Browse files Browse the repository at this point in the history
Prior to this CL, window.open(url, '', 'noopener=true') would treat
'noopener=true' as if noopener is false. This is currently per-spec [1]
but there is a desire [2][3] to change that.

The I2S is here:
https://groups.google.com/a/chromium.org/g/blink-dev/c/ePJ4GE6VzVc/m/urs3-4rHDAAJ

[1] https://html.spec.whatwg.org/multipage/window-object.html#concept-window-open-features-parse-boolean
[2] whatwg/html#7425
[3] whatwg/html#7399

Fixed: 1277613
Change-Id: I5b3a7e985a9bb392c2150846b50369cfcd9b05fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3495659
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#977722}
NOKEYCHECK=True
GitOrigin-RevId: 0c49a8d7e96bbad1102c934687836d3795073603
  • Loading branch information
Mason Freed authored and copybara-github committed Mar 4, 2022
1 parent 48bb4a4 commit 4bfc92e
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 1 deletion.
3 changes: 2 additions & 1 deletion blink/renderer/core/page/create_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ WebWindowFeatures GetWindowFeaturesFromString(const String& feature_string,

// Listing a key with no value is shorthand for key=yes
int value;
if (value_string.IsEmpty() || value_string == "yes") {
if (value_string.IsEmpty() || value_string == "yes" ||
value_string == "true") {
value = 1;
} else if (value_string.Is8Bit()) {
value = CharactersToInt(value_string.Characters8(), value_string.length(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,48 +26,59 @@
// The explicit popup feature.
["popup", false],
["popup=1", false],
["popup=true", false],
["popup=0", true],

// Other feature alone results in popup.
["location", false],
["location=yes", false],
["location=true", false],
["location=no", false],

["toolbar", false],
["toolbar=yes", false],
["toolbar=true", false],
["toolbar=no", false],

["menubar", false],
["menubar=yes", false],
["menubar=true", false],
["menubar=no", false],

["resizable", false],
["resizable=yes", false],
["resizable=true", false],
["resizable=no", false],
],
"single-2": [
["scrollbars", false],
["scrollbars=yes", false],
["scrollbars=true", false],
["scrollbars=no", false],

["status", false],
["status=yes", false],
["status=true", false],
["status=no", false],

["titlebar", false],
["titlebar=yes", false],
["titlebar=true", false],
["titlebar=no", false],

["close", false],
["close=yes", false],
["close=true", false],
["close=no", false],

["minimizable", false],
["minimizable=yes", false],
["minimizable=true", false],
["minimizable=no", false],

["personalbar", false],
["personalbar=yes", false],
["personalbar=true", false],
["personalbar=no", false],
],
"position": [
Expand Down Expand Up @@ -113,6 +124,8 @@

// The explicit popup feature has priority than others.
["popup=1,location,toolbar,menubar,resizable,scrollbars,status", false],
["popup=yes,location,toolbar,menubar,resizable,scrollbars,status", false],
["popup=true,location,toolbar,menubar,resizable,scrollbars,status", false],
["popup=0,location,toolbar,menubar,resizable,scrollbars", true],
],
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<script>
const channelName = location.search.substr(1),
channel = new BroadcastChannel(channelName);

const haveOpener = window.opener !== null;
const haveReferrer = document.referrer !== null && document.referrer !== "";
const allBarProps = [
window.locationbar.visible,
window.menubar.visible,
window.personalbar.visible,
window.scrollbars.visible,
window.statusbar.visible,
window.toolbar.visible
];
const isPopup = allBarProps.every(x=>!x);

channel.postMessage({haveOpener, haveReferrer, isPopup});

// Because messages are not delivered synchronously and because closing a
// browsing context prompts the eventual clearing of all task sources, this
// document should not be closed until the opener document has confirmed
// receipt.
channel.onmessage = () => window.close();
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
{ testDescription: "noopener=1 means the same as noopener",
secondWindowFeatureString: "noopener=1",
shouldReturnWindow: false },
{ testDescription: "noopener=true means the same as noopener",
secondWindowFeatureString: "noopener=true",
shouldReturnWindow: false },
{ testDescription: "noopener=0 means lack of noopener",
secondWindowFeatureString: "noopener=0",
shouldReturnWindow: true },
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<!doctype html>
<meta charset=utf-8>
<meta name="timeout" content="long">
<title>window.open() windowFeature value parsing</title>
<link rel="author" href="mailto:masonf@chromium.org">
<link rel="help" href="https://html.spec.whatwg.org/multipage/window-object.html#concept-window-open-features-parse-boolean">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
function testValueGeneric(val, expectTrue, property, testFn) {
const windowFeatureStr = val === "" ? property : `${property}=${val}`;
async_test(t => {
const windowName = '' + Math.round(Math.random()*1e12);
const channel = new BroadcastChannel(windowName);
channel.onmessage = t.step_func_done(e => {
// Send message first so if asserts throw the popup is still closed
channel.postMessage(null);
testFn(e.data);
});
window.open("support/windowFeature-values-target.html?" + windowName, windowName, windowFeatureStr);
},`Test ${windowFeatureStr}, expected interpretation is ${expectTrue ? 'true' : 'false'}`);
}

function testValueForNoReferrer(val, expectTrue) {
testValueGeneric(val, expectTrue, "noreferrer", (data) => {
if (expectTrue) {
assert_false(data.haveReferrer);
assert_false(data.haveOpener);
} else {
assert_true(data.haveReferrer);
assert_true(data.haveOpener);
}
});
}

function testValueForNoOpener(val, expectTrue) {
testValueGeneric(val, expectTrue, "noopener", (data) => {
assert_equals(data.haveOpener, !expectTrue);
});
}

function testValueForPopup(val, expectTrue) {
testValueGeneric(val, expectTrue, "popup", (data) => {
assert_equals(data.isPopup, expectTrue);
});
}

function testValue(val, expectTrue) {
const quotes = val === "" ? [''] : ['','"',"'"];
let noQuotes = true;
for (const quote of quotes) {
const thisExpectTrue = expectTrue && noQuotes;
const thisVal = quote + val + quote;
testValueForNoReferrer(thisVal, thisExpectTrue);
testValueForNoOpener(thisVal, thisExpectTrue);
testValueForPopup(thisVal, thisExpectTrue);
noQuotes = false;
}
}

testValue('',true); // Just the parameter means true
testValue('yes',true); // Yes means true
testValue('true',true); // True means true
testValue('foo',false); // If parsing as an integer is an error, false
testValue('0',false); // 0 is false
testValue('00',false); // 0 is false
testValue('1',true); // Non-zero is true
testValue('99999',true); // Non-zero is true
testValue('-1',true); // Non-zero is true
testValue('1foo',true); // This parses to 1
testValue('0foo',false); // This parses to 0
</script>

0 comments on commit 4bfc92e

Please sign in to comment.