Fix Measuring Child AMP Elements #9279
Conversation
|
Preview at http://output.jsbin.com/soniwap/quiet (Hard refresh, should always show images) |
jridgewell
changed the title
css/amp.css
Outdated
| @@ -271,7 +271,7 @@ i-amphtml-sizer { | |||
| color: transparent !important; | |||
| } | |||
|
|
|||
| .i-amphtml-notbuilt > * { | |||
| .i-amphtml-notbuilt:not(.i-amphtml-layout-container ) > * { | |||
There was a problem hiding this comment.
nit: remove trailing space before )
src/service/resource.js
Outdated
| } | ||
| // If this ancestor isn't built yet, that means we don't know what kind | ||
| // of styles will be applied to this element. We must wait. | ||
| if (!ancestor.isBuilt()) { |
There was a problem hiding this comment.
This is only true for non-container ancestors. With containers, we want to measure as soon as we can. We non-containers, we already all good, right? It will measure to 0 and then re-measure to the right value.
There was a problem hiding this comment.
With containers, we want to measure as soon as we can
Fixed.
We non-containers, we already all good, right? It will measure to 0 and then re-measure to the right value.
Unfortunately not. If the non-container isn't built yet, it will measure to 0 and never re-remeasure (because it thinks it has a proper measure). That's the bug.
| @@ -147,6 +147,9 @@ export class Resource { | |||
| /** @private {!AmpElement|undefined|null} */ | |||
| this.owner_ = undefined; | |||
|
|
|||
| /** @private {!AmpElement|undefined|null} */ | |||
| this.ampAncestor_ = undefined; | |||
There was a problem hiding this comment.
This needs to be discussed a bit more. But CSS changes seem rather clear to me. Can we merge them first and split this part into another PR? I'd like to get a bit deeper on this.
| const resource = Resource.forElementOptional(ancestor); | ||
| // If there's an AMP ancestor that's still not stubbed (has a Resource), | ||
| // we don't know what to do yet. | ||
| if (!resource) { |
There was a problem hiding this comment.
I believe, unfortunately, we do have a very few amp- tags that are not custom elements. Could you pls double-check via validator? Though validator wouldn't give the full picture since some extensions might create them via createElement
There was a problem hiding this comment.
I don't understand.
There was a problem hiding this comment.
I.e.
This code is probably benign, since no one ads stuff into this element from what I can tell. But others might not be so lucky.
There was a problem hiding this comment.
@zhouyx: Is that supposed to be publicly stylable?
There was a problem hiding this comment.
Reviewed, #9283. Will update so those aren't considered.
jridgewell
force-pushed
the
measure-child-amp-element
branch
from
f531e71 to
4e676ef
Compare
jridgewell
force-pushed
the
measure-child-amp-element
branch
from
574da7e to
55d9a0e
Compare
| const tag = element.tagName; | ||
| // Use prefix to recognize AMP element. This is necessary because stub | ||
| // may not be attached yet. | ||
| return startsWith(tag, 'AMP-') && |
There was a problem hiding this comment.
why is checking element.classList.contain('i-amphtml-element') not sufficient?
There was a problem hiding this comment.
From the original comment
We can tell whether the parent has been stubbed by whether a resource has been attached to it.
That means we have to be able to tell regardless of where the element is in the CustomElement lifecycle. And an element has to have been stubbed (#connectedCallback, the first step) to have the i-amphtml-element class.
jridgewell
force-pushed
the
measure-child-amp-element
branch
from
55d9a0e to
219428c
Compare
|
This issue seems to be in Pending Triage for awhile. Please triage this to an appropriate milestone. |
|
Thanks for the pull request. Since this PR has been stale for a while, I am closing it. Please feel free to reopen as needed. |
|
@aghassemi I Have the same issue with AMP image loading, sometimes images loaded for a long time, or even I just have seen a white box instead of the image. And with just resize the page even for 1 px, all images have been load immediately, It seems to need a shock to load images:). |
|
@miladmhp Could you please file an issue with a repro Url? |
AMP elements will not be measured until their ancestor AMP elements have been built. This excludes placeholders, which are laid out independent of their ancestor.
Fixes #9267, likely others.