Do not initiate amp-ad Fast Fetch if height/width of slot is 0 #7498
Conversation
| @@ -404,10 +404,16 @@ export class AmpA4A extends AMP.BaseElement { | |||
| // onLayoutMeasure gets called multiple times. | |||
| return; | |||
| } | |||
| this.layoutMeasureExecuted_ = true; | |||
| const slotRect = this.getIntersectionElementLayoutBox(); | |||
| if (slotRect.height == 0 || slotRect.width == 0) { | |||
There was a problem hiding this comment.
Should there be an explicit check for display:none here? Or is that subsumed by the dimension checks?
There was a problem hiding this comment.
I copied over the method the scheduler is using to decide whether to call layoutCallback to ensure parity with Delayed Fetch (see https://github.com/ampproject/amphtml/blob/master/src/service/resource.js#L614)
There was a problem hiding this comment.
Then maybe the thing to do is to call isDisplayed directly, rather than duplicating the code? Also, I worry that that logic is special-purpose and isn't intended to cover CSS styling display:none.
| it('should not send request if display none', () => { | ||
| a4aElement.style.display = 'none'; | ||
| return fixture.addElement(a4aElement).then(element => { | ||
| expect(xhrMock.called).to.be.false; |
There was a problem hiding this comment.
Can you make this expect(xhrMock).to.not.be.called?
| @@ -709,6 +714,21 @@ describe('amp-a4a', () => { | |||
| expect(a4a.onLayoutMeasure.bind(a4a)).to.throw(/fixed/); | |||
| }); | |||
| }); | |||
| it('does not initial promise chain if display none', () => { | |||
| @@ -139,6 +140,10 @@ describe('amp-a4a', () => { | |||
| return ampdocService.getAmpDoc(element); | |||
| }; | |||
| element.isBuilt = () => {return true;}; | |||
| element.getLayoutBox = () => { | |||
| const visible = element.style.display != 'none'; | |||
| return layoutRectLtwh(0, 0, visible ? 200 : 0, visible ? 50 : 0); | |||
There was a problem hiding this comment.
This entangles checking display:none with dimension checks. Can we have separate tests for the different cases? (Just paranoia: I want to be sure that chain is blocked regardless of whether web designer sets styling or dimensions.)
|
@tdrl back to you! Thanks |
| 'type': 'adsense', | ||
| }); | ||
| element.getAmpDoc = () => { | ||
| const ampdocService = ampdocServiceFor(doc.defaultView); | ||
| return ampdocService.getAmpDoc(element); | ||
| }; | ||
| element.isBuilt = () => {return true;}; | ||
| element.getLayoutBox = () => { | ||
| const visible = element.style.display != 'none'; | ||
| return layoutRectLtwh(0, 0, visible ? 200 : 0, visible ? opt_height : 0); |
There was a problem hiding this comment.
But you still have the same issue here, don't you? If the 'does not initialize promise chain if display none' test passes, you're not sure whether it's because you are detecting display:none or because you're detecting the width == 0. Would it work to just get rid of this override of getLayoutBox and test height, width, and display:none directly, in independent tests?
There was a problem hiding this comment.
Unfortunately the way we're initializing this test, it will fail without explicitly adding getLayoutBox (presumably this is why other getters have been attached to element). Perhaps we need to rework the tests but I would rather not delay this PR.
In any event, I reworked so that rect is passed as optional item but in the end display none was not actually being tested. Instead I reworked to verify behavior with 0 height or width.
| xhrMock.onFirstCall().returns(Promise.resolve(mockResponse)); | ||
| return createAdTestingIframePromise().then(fixture => { | ||
| const doc = fixture.doc; | ||
| const a4aElement = createA4aElement(doc, 0); |
There was a problem hiding this comment.
Good, but there should probably be a separate test that checks width == 0 as well. (Sorry to be a PITA about tests, but it continues to be one of our weak spots. :-( )
|
@tdrl PTAL |
…oject#7498) * add height/width 0 check to onLayoutMeasure * Re-execute check on later onLayoutMeasure calls * code cleanup * PR review * PR review
…oject#7498) * add height/width 0 check to onLayoutMeasure * Re-execute check on later onLayoutMeasure calls * code cleanup * PR review * PR review
See #7491