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

Impression declaration for links opened through Window.open #77

Closed
wilsonssun opened this issue Oct 20, 2020 · 9 comments
Closed

Impression declaration for links opened through Window.open #77

wilsonssun opened this issue Oct 20, 2020 · 9 comments
Labels
inactive? Issue may be inactive

Comments

@wilsonssun
Copy link

The API currently only specifies implementation for links opened through HTML anchor tags, but we also have links that are programmatically navigated through Window.open. What, if any, is the appropriate way to do impression declaration for these?

@johnivdel
Copy link
Collaborator

Supporting JS navigation APIs seems reasonable, especially in the context of #22 and privacycg/private-click-measurement#31, which discuss adding JS support for conversion registration.

A few directions we could go to support window.open navigations are:

Supply impression registration attributes directly to the window.open() call

Add a new param to window.open which takes an impression for the navigation.

Window? open(
  optional USVString url="",
  optional DOMString target = "_blank",
  optional [TreatNullAs=EmptyString] DOMString features = "",
  optional ImpressionInfo impression);

where ImpressionInfo is a new IDL dictionary:

dictionary ImpressionInfo {

  required DOMString impressionData;

  // This can be optional as we already have the |url| from the
  // window.open call, but this is still necessary if the
  // site is doing redirects.
  optional USVString conversionDestination;

  optional USVString reportingOrigin;
  optional unsigned long impressionExpiry;
}

One downside to this approach is that it does not work for other JS Navigation API's such as window.location.assign(), and params would need to be added to support those navigations as well.

Add a separate API to associate navigation urls with impression attributes

Add an API surface which associates an ImpressionInfo dictionary with a future navigation url.

[Exposed=Window] interface Measurement {

 // ForNavigation?
 void setImpressionForTarget( USVString url , ImpressionInfo info);
}

Example usage:

// This navigation has no impression.
window.open("https://advertiser.example/a.html", "_blank")

window.measurement.setImpressionForTarget("https://advertiser.example/a.html", {
  impressionData: "1234",
  reportingOrigin: "https://reporting.example" });

// This navigation has an impression!
window.open("https://advertiser.example/a.html", "_blank")

// This navigation doesn't!
window.open("https://advertiser.example/b.thml", "_blank")

This would allow impressions to be registered for any navigation, including window.open and navigations from anchor tags.

Impression registered in this manner would not make impressiondata visible to other scripts on the page. This could help with some issues discussed on #13 involving reporting fake conversions for impressions via scraping impression attributes.

@johnivdel
Copy link
Collaborator

cc @csharrison for thoughts

@csharrison
Copy link
Collaborator

Add a separate API to associate navigation urls with impression attributes

I like this idea John! Let me cc @domenic for thoughts as well. Some initial thoughts

  • Having some global map scoped to the window is non-ideal but allowing it to be targeted at a fine grain at the URL level makes sense
  • We should avoid "target" in the name since there is a target attribute in <a> that has a different meaning. Maybe "ForURL"?
  • We should be careful about scoping this map to the initiating window, so something like window.top.location works and doesn't accidentally use the URL map associated with window.top.
  • We might want to extend the matching in other ways so you could consider making the matching URL part of the init dictionary rather than a dedicated param to the method.
    e.g.
window.measurement.configureImpression({
  matchingUrl: "https://foo.com/",
  ...
});

This way it would be trivial to add other extensions if we wanted to match on other things like domains, origins, etc.

@domenic
Copy link

domenic commented Nov 13, 2020

A global map that only impacts the next call to specific APIs is a bit strange. It's like if your C++ code, instead of calling a function by passing arguments, set a global variable and then expected the function to look up its input data in the global variable. Generally this is discouraged in most codebases.

Another big thing to consider on global-map vs. per-call-site is whether it's OK for any script running in the window to modify the impression mapping. E.g., if I include <script src="https://adtech.example/ad.js"></script> or <script src="https://cdn.example/js-utility-library.js"></script>, is it OK that those scripts can modify the global impression data mapping? The per-call-site architecture prohibits that, since you have to actually be writing the window.open(), location.assign(), etc. calls to associate the impression data.

Overall, to me added arguments to window.open() and location.assign() makes the most sense.

@csharrison
Copy link
Collaborator

A global map that only impacts the next call to specific APIs is a bit strange

Oh I saw this as a permanent map not one that just impacts the next call. I see your point about it being strange and I agree it is non-ideal. The benefit in my mind is just a single interface to use the API rather than changing all possible API surfaces for navigation.

Another big thing to consider on global-map vs. per-call-site is whether it's OK for any script running in the window to modify the impression mapping

I think this is reasonable but to be fair can't all those JS APIs be shimmed and mutated as well if they are running in the same context?

Overall, to me added arguments to window.open() and location.assign() makes the most sense.

Thanks, I appreciate this feedback. I'm happy to go with that option, especially if we can just use the same init struct in all cases. @johnivdel what do you think?

@domenic
Copy link

domenic commented Nov 13, 2020

I think this is reasonable but to be fair can't all those JS APIs be shimmed and mutated as well if they are running in the same context?

Good point! That pretty much invalidates that paragraph of my response :)

@csharrison
Copy link
Collaborator

Discussed offline with domenic and johnivdel. We all agreed that adding direct support to window.open, location.assign, and possibly some new API on the anchor element was the best path forward. Those APIs could all take the same init struct to configure the API.

An execution-scoped map has too many downsides (action at a distance, etc) to be worth it to remove an argument to a few call sites.

johnivdel added a commit that referenced this issue Nov 23, 2020
Adds a section denoting how to register impressions for window.open navigations per #77
@maudnals maudnals added the inactive? Issue may be inactive label Jun 17, 2021
@apasel422
Copy link
Collaborator

Is there anything left to do here? As far as I know, we support impression declaration via window.open now, though it hasn't been spec'd yet.

@johnivdel
Copy link
Collaborator

Closing as this was superseded by #130

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

6 participants