Support display observer in the lightbox #32701
Conversation
|
@jridgewell I refactored the code per your recommendation to combine container and observer into one structure. It does look cleaner that way. PTAL. |
| * @param {!Element} container | ||
| * @private | ||
| */ | ||
| containerObserved_(isDisplayed, container) { |
There was a problem hiding this comment.
Are we sure this optimization is necessary? I imagine destroying/creating the InObs is actually more expensive than having an initialized one that's not observing anything.
There was a problem hiding this comment.
Yeah, not sure about this, tbh. Recreating InOb would definitely have its cost. The issue I'm having: the intersection observer on the container may very well see the intersections - it'd do so unless the container or the observed child are truly set display:none. However, from the point of view of the document, the container itself as a whole might be not-displayed. So, I'd then need to override the container observations with false somehow. So, what I do here instead is disconnect the observer and instead overwrite all values to false, knowing that the observer cannot flip them back to true until it's recreated. WDYT?
There was a problem hiding this comment.
Does the container's InOb not alert all the targets that they are not visible anymore? I had assumed we could make this function just a noop, but if more is required then it makes sense to disconnect as display changes.
There was a problem hiding this comment.
Not always. It's possible to have a situation where the container's InOb considers a target displayable, but the container itself is not. This is because InOb doesn't have a concept of "displayable" - it can only answer the question of whether a target "intersects" the container.
|
@jridgewell Responded. PTAL. |
| */ | ||
| setObservation_(target, index, value, callbacks) { | ||
| let observations = this.targetObservations_.get(target); | ||
| if (!observations) { |
There was a problem hiding this comment.
I believe this only happens during observe(target, cb), right? If so, we could just move that there and simplify setObservation_.
There was a problem hiding this comment.
I think so. But this still feels a bit safer since the map.get() by type returns a X|undefined value.
| * @param {!Element} container | ||
| * @private | ||
| */ | ||
| containerObserved_(isDisplayed, container) { |
There was a problem hiding this comment.
Does the container's InOb not alert all the targets that they are not visible anymore? I had assumed we could make this function just a noop, but if more is required then it makes sense to disconnect as display changes.
Partial for #31540.
See explainer here.