Amp byside content improvements #18962
Conversation
examples/amp-byside-content.amp.html
Outdated
| @@ -50,6 +50,7 @@ <h2>Fixed layout content</h2> | |||
| layout="fixed" | |||
| width="120" | |||
| height="40" | |||
| sandbox="allow-scripts allow-same-origin allow-popups" | |||
There was a problem hiding this comment.
why does this need to be exposed on the component? to the page author, the implementation details that this is an iframe should be irrelevant. Is there a single default the component can use? If not, under what conditions does the page author need to change them? ( and how would they know what to change them to?)
There was a problem hiding this comment.
Hi @aghassemi, thanks for your comments. We are going to assume pre-defaults for the component and set iframe attributes automatically.
| this.element.setAttribute('scrolling', 'no'); | ||
| } | ||
|
|
||
| if (!this.element.hasAttribute('frameborder')) { |
There was a problem hiding this comment.
ditto above, it is strange to expose iframe attributes on this component. why don't the defaults work?
There was a problem hiding this comment.
As above, we are now assuming defaults.
| // On iOS the iframe at times fails to render inside the `overflow:auto` | ||
| // container. To avoid this problem, we set the `overflow:auto` property | ||
| // 1s later via `amp-active` class. | ||
| if (this.container_ != this.element) { |
There was a problem hiding this comment.
we have been dealing with similar problems for scrollable areas in iOS. The 1000 timeout is arbitrary though. Maybe you wanna run this first time embed-size message is received.
There was a problem hiding this comment.
Applied the same behaviour to when the embed-size message is received.
| this.channel_ = (this.element.getAttribute('data-channel') || ''); | ||
| this.lang_ = (this.element.getAttribute('data-lang') || DEFAULT_LANG_); | ||
| this.fid_ = (this.element.getAttribute('data-fid') || ''); | ||
|
|
||
| this.isResizable_ = this.element.hasAttribute('resizable'); |
There was a problem hiding this comment.
we really like to avoid sub-areas of page that are vertically scrollable. It created bad UX. May I ask why scrollable iframe is preferred to resized version in some situations in your case?
There was a problem hiding this comment.
Removed resizable custom property, always assume non scrollable.
change debounce call
bysidedevel3rdparty
force-pushed
the
amp-byside-content-improvements
branch
from
482ec8d
to
0c1091b
Compare
| * @param {!Element} element | ||
| * @return {!Element} The container or the iframe. | ||
| */ | ||
| function makeIOsScrollable(element) { |
There was a problem hiding this comment.
there shouldn't be a need for this or container_ anymore since iframe is now never scrollable. Also amp-active stuff should not be needed.
There was a problem hiding this comment.
You're right. Updated code to remove the container_ and no longer use the amp-active class.
This PR implements improvements to amp-byside-content extension based on code review suggestions discussed on #12798 .
Also allow to set sandbox attribute in iframe if requested in component.