Skip to content
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

Closed
johnivdel opened this issue Apr 13, 2021 · 6 comments
Closed

Use a different overload to handle window.open navigations #130

johnivdel opened this issue Apr 13, 2021 · 6 comments
Labels
inactive? Issue may be inactive

Comments

@johnivdel
Copy link
Collaborator

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.

@johnivdel
Copy link
Collaborator Author

cc @domenic for thoughts

@domenic
Copy link

domenic commented Apr 14, 2021

Hmm.

Introducing a new method like this seems tricky because window.open() is full of legacy design mistakes. So, it'd be a shame to introduce a whole new method that carries them over and then just adds a single piece of functionality.

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:

  • Not encode left/top/width/height/noopener/noreferrer into strings
  • Not also have a second use where you pass a window name and it navigates that window
  • Not infer "popupness" (as opposed to new-tab-ness) from the width/height/left/top, but instead have it be explicit
  • Allow any referrer policy, not just no-referrer (via today's noreferrer string option)
  • Flip the default so usually you don't get opener access, and you have to opt in to it
  • Not parse the URL relative to the entry settings object (Minimize usage of the entry concept whatwg/html#1431) but instead use the relevant settings object like other modern APIs.
  • Not special case the empty string or about:blank URLs

/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 window.open(), does:

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`

@annevk
Copy link

annevk commented Apr 15, 2021

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?

@johnivdel
Copy link
Collaborator Author

My instinct is it'd be more palatable to design a new method from scratch

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.

However, note that feature detection for such a design would require the same contortions (whatwg/webidl#107) as your current design, of adding to window.open(), does:

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 window.open() breakage.

One possibility would be using the JS API for registering attribution sources as an indicator, document.attributionReporting, which we proposed here.

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?

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.

@johnivdel
Copy link
Collaborator Author

@domenic Do you have an opinion on placing these attributes within features per @annevk's idea?

@domenic
Copy link

domenic commented Apr 22, 2021

It's ugly, but it's consistent. It seems like the path of least resistance.

johnivdel added a commit that referenced this issue Apr 28, 2021
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
johnivdel added a commit that referenced this issue Apr 29, 2021
* 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
pull bot pushed a commit to Mu-L/chromium that referenced this issue May 5, 2021
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}
@maudnals maudnals added the inactive? Issue may be inactive label Jun 17, 2021
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive? Issue may be inactive
Projects
None yet
Development

No branches or pull requests

4 participants