Assert slide parent is connected #9432
Conversation
dreamofabear
force-pushed
the
shadow-slides-assert
branch
from
9bfb9a5
to
c42a7da
Compare
src/dom.js
Outdated
| break; | ||
| } | ||
| } while (true); | ||
| return (n instanceof Document); |
There was a problem hiding this comment.
how bad is this for performance - should we add guidelines of when/whom should use this? (may be add a presubmit review hook?)
There was a problem hiding this comment.
I don't think it's much worse than closestNode etc. that walk up the ancestor chain.
src/dom.js
Outdated
| break; | ||
| } | ||
| } while (true); | ||
| return (n instanceof Document); |
There was a problem hiding this comment.
instanceof has bug when crossing boundaries. Instead, use n.nodeType == 9
There was a problem hiding this comment.
@jridgewell wow that is interesting! could you share if you have any reading material on this
There was a problem hiding this comment.
There was a problem hiding this comment.
Good to know, though this shouldn't cross iframe boundary. Done.
| @@ -186,6 +187,8 @@ export class AmpSlideScroll extends BaseSlides { | |||
|
|
|||
| // Slides must only be re-parented to DOM-connected nodes to avoid | |||
| // errors when used in a shadow document (#9291). | |||
| dev().assert(isConnectedNode(slideWrapper), | |||
There was a problem hiding this comment.
This'll probably affect everything that reparents.
There was a problem hiding this comment.
Fair, created an issue to investigate: #9441
|
@jridgewell @camelburrito Any other comments? |
|
Thanks! |
Fixes ampproject#12849. Reverts part of ampproject#9432, since the bug is being addressed in runtime. Closes ampproject#9441, since root bug is being addressed.
Fixes ampproject#12849. Reverts part of ampproject#9432, since the bug is being addressed in runtime. Closes ampproject#9441, since root bug is being addressed.
Follow-up to #9426.
/to @jridgewell @camelburrito