✨ Landscape ad support for amp-story-auto-ads #40264
Conversation
|
thanks @qwang-mlb we're taking a look |
|
Thanks for looking into this, @erwinmombay and @ychsieh. Do you have an ETA for completing PR review? Ideally, we want your feedback by EOD tomorrow (4/9). |
|
Apologies we are swamped this week. We can finish the full review before mid next week. |
|
Thanks for the update @ychsieh. If we could get a review this week that would help us immensely. |
|
Ok I have taken a look but it seems mostly about the ad components that I have very limited experience in. @powerivq would be the best person to review this. |
| * @param {?Element} adElement | ||
| * @return {number} | ||
| * */ | ||
| const getAdAspectRatio = (adElement) => { |
There was a problem hiding this comment.
Nit: make this a private member function.
Would it not work for text ads?
There was a problem hiding this comment.
Updated it to be a private member function and added code to check for text ads:
It could work for text ads but the HTML content they serve must have a set width and height. Otherwise, we aren't able to get those dimensions and figure out the best way to display.
|
|
||
| /** | ||
| * Sets the landscape ad class on the page element if the asset's | ||
| * aspect ratio is greater than or equal to 31/40. |
There was a problem hiding this comment.
Can you clarify why 31/40 is the limit?
There was a problem hiding this comment.
I'm using 31/40 because that is the aspect ratio in which the amp-story switches from the default (portrait) view to fullbleed mode.
I'm happy to change this if you think another ratio would work better.
…o text content. Updated unit tests.
|
Seems that the |
erwinmombay
requested review from
ychsieh,
Copilot and
erwinmombay
and removed request for
ychsieh
There was a problem hiding this comment.
Pull Request Overview
This PR adds (partial) support for landscape ads in amp-story-auto-ads by applying a new CSS class based on the ad asset's aspect ratio and updating tests accordingly. Key changes include:
- New tests in test-story-ad-page.js to validate landscape ad behavior for video, image, and text ads.
- Updated logic in story-ad-page.js to calculate ad aspect ratios and conditionally apply the landscape ad class.
- Adjustments in amp-story-auto-ads.js and test-amp-story-auto-ads.js to handle the DESKTOP_FULLBLEED attribute in the context of landscape ads.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| extensions/amp-story-auto-ads/0.1/test/test-story-ad-page.js | Added tests for checking landscape ad classification for various asset types. |
| extensions/amp-story-auto-ads/0.1/test/test-amp-story-auto-ads.js | Added tests to ensure the desktop-fullbleed attribute is not propagated for landscape ads. |
| extensions/amp-story-auto-ads/0.1/story-ad-page.js | Introduced helper methods to compute the ad aspect ratio and conditionally apply the landscape ad class. |
| extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js | Updated desktop UI attribute logic considering landscape ad support. |
Files not reviewed (1)
- extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css: Language not supported
Comments suppressed due to low confidence (1)
extensions/amp-story-auto-ads/0.1/test/test-amp-story-auto-ads.js:440
- [nitpick] Consider stubbing 'visibleAdPage_' with an object that more closely mimics the real implementation to improve test realism and reliability.
autoAds.visibleAdPage_ = {hasLandscapeAd: () => true};
| return 0; | ||
| } | ||
|
|
||
| const adVideo = adElement.querySelector('video'); |
There was a problem hiding this comment.
How could it tell if this ad container only has a video/img tag?
There was a problem hiding this comment.
Sorry, I am not too experienced in how ads work/how they should be.
How would we figure out which piece of content coming from the provider is the "main" content? I do notice that some ads will return a video AND an image. Right now, the code is assuming that if a video exists, it is the "main" content we want to display.
There was a problem hiding this comment.
@qwang-mlb Yes, this is my question. Wouldn't this be very error prone? An ad could contain multiple img and you calculate the aspect ratio by using the first element you queried. One of these img elements could even be a analytics beacon. Can you think of some better ways to approach this?
There was a problem hiding this comment.
I'm looking at the different responses from the ad networks and there doesn't seem to be any indicator which is the "main" content.
A different approach would be to have a property in the config that tells us that it's landscape. Right now, we target landscape ads with the property json.targeting.view but not sure if every ad network accepts a json config. How about data-orientation?
{
"ad-attributes": {
"type": "doubleclick",
"data-slot": "/30497360/a4a/amp_story_dfp_example",
"data-orientation": "landscape"
}
}
There was a problem hiding this comment.
@qwang-mlb I think most ads will have the winning ad unit's slot dimensions returned to you. Do you see anything like that?
data-orientation is part of the ad request, and it probably won't guarantee the response will be horizontal.
There was a problem hiding this comment.
@powerivq I don't see anything specifically referring to a winning ad unit's slot dimensions. I see this extractSize method in amp-a4a that checks the response headers for the size but all the examples that I have return null. If that's null, it'll use the size defined by the amp-ad container:
amp-ad container size:
https://github.com/ampproject/amphtml/blob/main/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css#L32-L44
Since we can't determine the winning ad unit's slot dimensions easily, how about we simplify this? I'll remove all the checks for aspect ratios and just create an experiment. If this experiment is enabled and AMP is in fullbleed mode, we will override the CSS in amp-story-auto-ads.css so that the ad can be displayed properly.
There was a problem hiding this comment.
@qwang-mlb I checked the code and we are measuring iframe dimension. I am also okay if you want to force fullscreen provided that you add an opt-in.
There was a problem hiding this comment.
Thanks for taking a look!
I've created the experiment story-ad-allow-fullbleed and removed the aspect ratio checks. Now the ads will be fullscreen if the experiment is turned on AND AMP is in DESKTOP_FULLBLEED mode.
Let me know if there's another way to add experiments or if I missed something there. I tested it locally and it seems to work.
| this.mutateElement(() => { | ||
| adPage.toggleVisibility(); | ||
| this.visibleAdPage_ = adPage; | ||
| if (this.visibleAdPage_.hasLandscapeAd()) { |
There was a problem hiding this comment.
Can we add a doc-level opt-in for this feature (using meta tags)? See getExperimentToggles
This way we do not mess up others until we can fully assess the feature, while you can optin to use it.
There was a problem hiding this comment.
That's a good idea, thanks!
396942d#diff-1b3f44791e63fb9d0c101b7471e291c160017ca7f4a1a7620edfcd4498194de8
…in fullbleed mode, the ads will be shown in fullbleed.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces partial support for landscape ads in amp-story-auto-ads by adding an experiment flag and updating both ad rendering and associated tests.
- Adds a new experiment configuration "story-ad-allow-fullbleed" to enable fullbleed mode for certain ads.
- Updates ad page and auto ads logic to conditionally apply fullbleed styling based on the experiment flag.
- Introduces new tests to verify fullbleed attribute propagation and behavior in both portrait and landscape scenarios.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/experiments/experiments-config.js | Adds new experiment configuration for fullbleed ads. |
| extensions/amp-story-auto-ads/0.1/test/test-story-ad-page.js | Introduces tests ensuring the fullbleed ad class is applied when the experiment is active. |
| extensions/amp-story-auto-ads/0.1/test/test-amp-story-auto-ads.js | Adds tests to confirm proper attribute propagation behavior with and without the fullbleed experiment. |
| extensions/amp-story-auto-ads/0.1/story-ad-page.js | Updates the ad page logic to apply the fullbleed ad class when the experiment flag is enabled. |
| extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js | Enhances fullbleed ad logic by adjusting the attribute application based on the experiment state. |
Files not reviewed (1)
- extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css: Language not supported
Comments suppressed due to low confidence (2)
extensions/amp-story-auto-ads/0.1/test/test-amp-story-auto-ads.js:437
- [nitpick] Instead of directly modifying the internal experiment flag, consider using the toggleExperiment utility to simulate the experiment condition for consistency with production code.
autoAds.fullbleedAdExpEnabled_ = true;
extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js:348
- [nitpick] Consider refactoring or extracting the logic for applying the DESKTOP_FULLBLEED attribute into a helper function to improve clarity and maintainability.
if (uiState === UIType_Enum.DESKTOP_FULLBLEED && !this.fullbleedAdExpEnabled_) {
|
👋 @powerivq Thanks for all the feedback/review. Any chance we can move this forward this week? We're eager to integrate landscape ads into landscape stories now that we're scaling up this new content type across our sites. |
|
@qwang-mlb You might need to rebase caz it contains fixes to circle test failures. |
|
@powerivq Looks like Edge is failing to start so the test failed. How should I proceed? |
|
Last question @qwang-mlb how does it work on mobile if you opt-in the experiment? It looks like that you'll still apply your css? |
|
@powerivq Yep, if you opt in on mobile, we want the CSS applied there as well. Here are a couple of screenshots: 
|
|
@qwang-mlb In mobile, it's always full screen. So it really is not different? If you agree, I would suggest to limit the scope to desktop? |
|
@powerivq Sorry not sure what you mean by that. On mobile, if you have landscape enabled and you do NOT opt into this experiement, this is what's shown (and not what we want):
|
|
@qwang-mlb Sorry, let me rephrase. This is not a vertical screen device. My question is what happens with a vertical screen device? |
|
@powerivq 😅 Sorry maybe I am still misunderstanding. A vertical screen wouldn't add this new CSS because it wouldn't have 
If you mean devices with large screens like iPads, here are the different scenarios:
|
|
I see, that is correct then. |
Addresses #40183
Add (partial) support for landscape ads in amp-story-auto-ads. This has only been tested with ad types
customanddoubleclickbecause I do not have access to other ad networks. Implementers for other ad networks may need to update or improve upon this.This change should keep the original way of handling ads in landscape mode, i.e. a portrait ad with letter boxing. Only ads that have an aspect ratio of
31 / 40or greater will have the classi-amphtml-landscape-adadded.Currently, this only checks for ad videos or images, any other type of ad will not be considered. If a video exists, it assumes that the video is the "main" ad asset and only check that asset's aspect ratio.