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.

Sign up for GitHub

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

Jump to bottom

attemptChangeSize not denied for above-the-fold iframe #19941

Closed
aghassemi opened this issue Dec 18, 2018 · 7 comments
Closed

attemptChangeSize not denied for above-the-fold iframe #19941

aghassemi opened this issue Dec 18, 2018 · 7 comments
Assignees
@jridgewell
Labels
P1: High Priority Type: Bug WG: runtime
Projects
Runtime
Milestone
Fixit End of 2018

Comments

@aghassemi
Copy link
Contributor

https://beta.ctvnews.ca/national/canada/2018/10/10/1_4128456.html

This page has an iframe above the fold but somehow attemptChangeSize is not denied and it causes content shifting.

/to @jridgewell @choumx
@dreamofabear
Copy link

Hmm, I'm not sure attemptChangeSize is succeeding. Adding a breakpoint in the callback (below) shows that the request is denied.

amphtml/src/service/resources-impl.js

Lines 892 to 899 in 962e125

this.scheduleChangeSize_(Resource.forElement(element), newHeight,
newWidth, opt_newMargins, /* force */ false, success => {
if (success) {
resolve();
} else {
reject(new Error('changeSize attempt denied'));
}
});

It might appear to be changing size due to the iframed content changing during load.

@dreamofabear
Copy link

In fact, you can see the overflow button "Click to Expand" being shown. @aghassemi did you notice differently?

@dreamofabear dreamofabear assigned aghassemi and unassigned jridgewell Dec 18, 2018
@aghassemi
Copy link
Contributor Author

@choumx trying it on Prod and Canary repros the content jump for me in all browsers:
Hitting it with local proxy does not however. I suspect there is a race condition.

2

@dreamofabear
Copy link

Weird, I can repro now. Thanks for the GIF.

@dreamofabear dreamofabear assigned jridgewell and unassigned aghassemi Dec 19, 2018
@dreamofabear dreamofabear added this to the Fixit End of 2018 milestone Dec 19, 2018
@jridgewell
Copy link
Contributor

Huh, funnily enough, this is related to #13343. It's falling into https://github.com/ampproject/amphtml/blob/master/src/service/resources-impl.js#L1368-L1371, which uses contentHeight to determine how close the element is to the bottom. But, contentHeight returns 0! That's obviously a bug.

@jridgewell
Copy link
Contributor

And it's because their .article-body-wrapper (a direct child of <body>) has float: left.

@jridgewell jridgewell mentioned this issue Dec 21, 2018
Merged
@dreamofabear dreamofabear added this to Prioritized in Runtime Jan 7, 2019
@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. @jridgewell Do you have any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jridgewell jridgewell

Labels
P1: High Priority Type: Bug WG: runtime
Projects
Runtime
Prioritized
Milestone
Fixit End of 2018
Development

No branches or pull requests
4 participants
@jridgewell @aghassemi @dreamofabear @ampprojectbot