-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use a different overload to handle window.open navigations #130
Comments
cc @domenic for thoughts |
Hmm. Introducing a new method like this seems tricky because My instinct is it'd be more palatable to design a new method from scratch. Something like: window.openWindow(url, { allowOpenerAccess, referrerPolicy, impressionParams });
window.openPopup(url, { left, top, width, height, allowOpenerAccess, referrerPolicy, impressionParams }); In particular such a clean slate would:
/cc @annevk as he might also be enthusiastic about fixing some of these legacy mistakes. I'd be happy to help with the spec for such a method if this path sounds interesting to you. However, note that feature detection for such a design would require the same contortions (whatwg/webidl#107) as your current design, of adding to let supportsImpressionParams = false;
try {
window.openPopup("about:blank", {
get impressionParams() {
// If this getter is hit then the implementation is reading
// the impressionParams dictionary member and so supports them.
supportsImpressionParams = true;
},
get width() {
// Since we don't actually want to open a popup, throw in an
// alphabetically-later dictionary member.
throw new Error();
}
} catch {}
// Use `supportsImpressionParams` |
I consider browsing context groups being able to point to multiple top-level browsing contexts a mistake so I rather not add API surface. Can we not embed this information in features? And you do feature detection through some related API and make it clear to implementers that it all needs to be implemented at once? |
I think these methods are overall quite a bit cleaner. I'm not sure if there are existing flows navigating a named window that would want to use the Attribution Reporting API, but we would be breaking those.
As far as feature detection goes, using the presence of a related API is a reasonable end state as @annevk suggested. I think we can treat the feature detection issue separately from resolving the issue with the One possibility would be using the JS API for registering attribution sources as an indicator, document.attributionReporting, which we proposed here.
It's worth noting that the browser associates the attribution source with the window calling the API, rather than the window being opened. This is somewhat disjoint from the non-normative understanding of features, as described in the html spec. But this would be sufficient to spec the API behavior. This would suffer from needing to encode everything as strings as @domenic pointed out. For Attribution Reporting as-is, most of the attributes are encoded as strings to begin with so I don't think this is a real issue. |
It's ugly, but it's consistent. It seems like the path of least resistance. |
This updates the window.open overload to supply attribution reporting attributes in the features parameter, rather than in a new window.open overload per #130
* Update window.open interface to use features parameter This updates the window.open overload to supply attribution reporting attributes in the features parameter, rather than in a new window.open overload per #130 * Update event_attribution_reporting.md * Update README.md * Update README.md
What: Parse Attribution Reporting API attributes from the features string provided to window.open and register impressions. window.open support was removed previously due to conflicting with existing window.open usage. Parsing the attributes from features will avoid running into this issue, see the discussion here for rationale: WICG/attribution-reporting-api#130 This behavior is covered in the Attribution Reporting Explainer: https://github.com/WICG/conversion-measurement-api#registering-attribution-sources-for-windowopen-navigations Note that while the explainer names are updated, the blink implementation still uses structures which use the old naming scheme. This is being addressed separately. Bug: 1204575 Change-Id: I4768e36108dc9564f907a0857cedffa1af345ba8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2862265 Commit-Queue: John Delaney <johnidel@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Nate Chapin <japhet@chromium.org> Cr-Commit-Position: refs/heads/master@{#879011}
What: Parse Attribution Reporting API attributes from the features string provided to window.open and register impressions. window.open support was removed previously due to conflicting with existing window.open usage. Parsing the attributes from features will avoid running into this issue, see the discussion here for rationale: WICG/attribution-reporting-api#130 This behavior is covered in the Attribution Reporting Explainer: https://github.com/WICG/conversion-measurement-api#registering-attribution-sources-for-windowopen-navigations Note that while the explainer names are updated, the blink implementation still uses structures which use the old naming scheme. This is being addressed separately. Bug: 1204575 Change-Id: I4768e36108dc9564f907a0857cedffa1af345ba8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2862265 Commit-Queue: John Delaney <johnidel@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Nate Chapin <japhet@chromium.org> Cr-Commit-Position: refs/heads/master@{#879011} NOKEYCHECK=True GitOrigin-RevId: 358e05386f68a0fcdec577208ffd42a9c25c3067
As pointed out in https://crbug.com/1164959, there is some existing web usage of a fourth parameter to window.open, "replace", as documented in this old draft spec https://www.w3.org/TR/2009/WD-html5-20090423/browsers.html#dom-open.
This raises concerns that modifying the pre-existing window.open API makes it hard to detect browser support for the feature. There is no way to determine whether a native function accepts a specific type of argument in a spot.
This could be avoided by exposing a separate window associated function like
openWithAttributionSource
, which avoids the legacy usage collision, and provides explicit feature detection.The text was updated successfully, but these errors were encountered: