FlyingCarpet: Ensure unbuilt elements eventually layout#6742
FlyingCarpet: Ensure unbuilt elements eventually layout#6742jridgewell merged 3 commits intoampproject:masterfrom
Conversation
|
Assigning to @zhouyx, since you dealt with similar issues in |
| amp-fx-flying-carpet > .-amp-fx-flying-carpet-clip > .-amp-fx-flying-carpet-container { | ||
| position: fixed !important; | ||
| top: 0 !important; | ||
| width: 100%; |
There was a problem hiding this comment.
Defaults to 100% width, so that fixed-height elements get a width during initial measure. This will be updated in the next mutate phase to match the flying-carpet's width exactly.
| throw e; | ||
| } | ||
| this.scheduleLayout(this.children_); | ||
| listen(this.element, 'amp:built', this.layoutBuiltChild_.bind(this)); |
There was a problem hiding this comment.
I think we should listen to 'amp:built' from each child here. As this.element is promised to have been built when we call #layoutCallback. Also I think '#listenOnce` is more appropriate as an element will only be built once.
There was a problem hiding this comment.
I think we should listen to 'amp:built' from each child here.
That would register n callbacks, when only one is needed.
As this.element is promised to have been built when we call #layoutCallback.
I'm counting on that. That way, I know the target of the event is never this element, so it must be one of the children that just built.
Also I think '#listenOnce` is more appropriate as an element will only be built once.
I would like this, but I can't reconcile it with the first point.
There was a problem hiding this comment.
I see, unlike things in sticky-ad, flying-carpet can have multiple different elements as children, which makes it not possible to use listenOnce.
There was a problem hiding this comment.
One last question, does scheduleLayout has any async sub call before checking for isBuilt? My concern is in that way we may schedule layout twice for one element.
There was a problem hiding this comment.
No, everything is sync. Even if it were scheduled twice, the task system handles that.
|
LGTM |
…6742) * FlyingCarpet: Ensure unbuilt elements eventually layout Fixes ampproject#6735. * Ensure only directly owned elements get layout * Linting
…6742) * FlyingCarpet: Ensure unbuilt elements eventually layout Fixes ampproject#6735. * Ensure only directly owned elements get layout * Linting
…6742) * FlyingCarpet: Ensure unbuilt elements eventually layout Fixes ampproject#6735. * Ensure only directly owned elements get layout * Linting
Fixes #6735.