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

Update explainer to talk about configs and use cases, rather than urn:uuids and modes #56

Merged
merged 33 commits into from
Feb 23, 2023

Conversation

gtanzer
Copy link
Collaborator

@gtanzer gtanzer commented Dec 8, 2022

Integrate proposed changes from #48, and in relation to WICG/turtledove#312

explainer/README.md Outdated Show resolved Hide resolved
explainer/README.md Outdated Show resolved Hide resolved
explainer/README.md Outdated Show resolved Hide resolved

##### Example usage

```
HTMLFencedFrameElement.canLoadOpaqueURL();
HTMLFencedFrameElement.canLoadConfig(config);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the API is still canLoadOpaqueURL(). Should we make a reference to that, saying that canLoadConfig() will replace it in the future, and in the meantime both will exist?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed maybe removing this section from the explainer for now. Do you still plan on doing that?

Copy link
Collaborator Author

@gtanzer gtanzer Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I semi-reverted the changes here. Now it describes the same HTMLFencedFrameElement.canLoadOpaqueURL() function, but as "would a fenced frame config with an opaque mapped url be able to load in this context?"

We can fully replace it when the replacement is fully determined/implemented.

explainer/README.md Outdated Show resolved Hide resolved
explainer/fenced_frame_config.md Outdated Show resolved Hide resolved
explainer/fenced_frame_config.md Outdated Show resolved Hide resolved
explainer/fenced_frame_config.md Outdated Show resolved Hide resolved
// ...
// }
var adFrame = document.createElement('fencedframe');
adFrame.setAttribute(config,auctionWinnerConfig);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues here:

  1. You have a comma accessor instead of a period
  2. setAttribute() won't actually work here since the config attribute is not a content attribute (string), but only a WebIDL attribute, which are only settable via JS. You can tell this because we don't have [Reflect] in our WebIDL: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.idl;l=10;drc=d99163d64d43b85a9efcf0110a0cfa81886b4616

Copy link

@dmdabbs dmdabbs Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FF Opaque Source explainer example shows the auction result promise resolving to an opaque url...

navigator.runAdAuction(myAuctionConfig).then((auctionWinnerUrl) => {
  // auctionWinnerUrl value e.g.
  // urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6
  var adFrame = document.createElement('fencedframe');
  adFrame.setAttribute(src,auctionWinnerUrl);
});

One chooses this path 'upstream' by returning a simple 'render': url from generateBid().

And the means to opt into the new way is to return

'render': {url: renderUrl, size: {width: renderWidth, height: renderHeight}},

and then use

navigator.runAdAuction(myAuctionConfig).then((auctionWinnerConfig) => {
  const adFrame = document.createElement('fencedframe');
  adFrame.config = auctionWinnerConfig;
});

Is that correct?
Or does the embedder set dimensions here...

navigator.runAdAuction(myAuctionConfig).then((auctionWinnerConfig) => {
  const adFrame = document.createElement('fencedframe');
  adFrame.config = new FencedFrameConfig(
     {
        'url': auctionWinnerConfig,
        'width' 300,
        'height': 250
     }  
  );
  
});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then use
navigator.runAdAuction(myAuctionConfig).then((auctionWinnerConfig) => { [.....]

Actually the way you'll get FLEDGE's promise to resolve a FencedFrameConfig (instead of a URN) is by having resolveToConfig: true inside the myAuctionConfig (that you pass into runAdAuction()). That's how you communicate your preference to deal in terms of configs (instead of URNs) to FLEDGE. This behavior isn't enabled yet though, so if you do it today you'll still get a URN. In fact, even once the behavior is enabled, it will only be rolled out gradually to more users for testing, so if you pass in resolveToConfig there will be a transition period where FLEDGE may still give you a URN. Long-term, FLEDGE will always resolve to a FencedFrameConfig with or without resolveToConfig: true in the auction configuration.

The reason all of this is not super clear yet is because I have not made the PR to the FLEDGE repository which describes exactly how to take advantage of this, but I'll be doing that soon.


Regarding your first question:

And the means to opt into the new way is to return
'render': {url: renderUrl, size: {width: renderWidth, height: renderHeight}},

I am not sure about this (not as familiar with FLEDGE). @gtanzer do you know?

Copy link
Collaborator Author

@gtanzer gtanzer Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the combination of those things is correct. In order to hook into the new size behavior in FLEDGE, you will need to declare ad sizes in the interest group declaration, make a bid that includes both url and size, put resolveToConfig in the auction config, and then store the winning config in .config as @dmdabbs's first example shows (to the extent that the fenced frames changes are rolled out). (You can't/don't need to modify a config that gets returned from FLEDGE; it already has everything baked in.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it may not be necessary to use configs to hook into the FLEDGE size changes: we would just need to do some extra implementation work to make urns work. We'll get back to you on that.

Copy link

@dmdabbs dmdabbs Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An IG ad's size metdata may be percentages,

...screen dimension coordinates (e.g. 100sw or 100sh)

The penny hasn't dropped for me how the internal size (I forget your terminology) is set and the external (page-visible dimensions).

Also how pubs/sellers deal with the URN->Newfangled Config rollout/transition.

I appreciate your explanations, @domfarolino and @gtanzer .

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domfarolino Re the original comment, updated to adFrame.config = auctionWinnerConfig;

explainer/interaction_with_content_security_policy.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM with a few comments.

explainer/fenced_frame_config.md Outdated Show resolved Hide resolved

##### Example usage

```
HTMLFencedFrameElement.canLoadOpaqueURL();
HTMLFencedFrameElement.canLoadConfig(config);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed maybe removing this section from the explainer for now. Do you still plan on doing that?

explainer/fenced_frame_config.md Outdated Show resolved Hide resolved
@domfarolino domfarolino merged commit f14ad02 into WICG:master Feb 23, 2023
github-actions bot added a commit that referenced this pull request Feb 23, 2023
…es (#56)

SHA: f14ad02
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants