Implement "hidden" trigger for VisibilityV2 (IntersectionObserver based)#6634
Implement "hidden" trigger for VisibilityV2 (IntersectionObserver based)#6634lannka merged 6 commits intoampproject:masterfrom
Conversation
…ed visibility tracking).
jridgewell
left a comment
There was a problem hiding this comment.
Everything looks fine to me, but this is something @avimehta should be reviewing.
| } | ||
|
|
||
| /** @private */ | ||
| onDocumentHidden_() { |
There was a problem hiding this comment.
I don't believe the backwards iteration is necessary here.
|
|
||
| const listeners = this.listeners_[resource.getId()]; | ||
| for (let j = listeners.length - 1; j >= 0; j--) { | ||
| const listener = listeners[j]; |
There was a problem hiding this comment.
Can you explain what shouldBeVisible is?
There was a problem hiding this comment.
this indicates if it is a "visible" trigger or "hidden" trigger
| // TODO: support "hidden" spec. | ||
| // Hidden trigger | ||
| if (!this.visibilityListenerRegistered_) { | ||
| this.viewer_.onVisibilityChanged(() => { |
There was a problem hiding this comment.
Can we only register this when the first "hidden" visibility listener is registered?
| if (this.updateCounters_( | ||
| lastChange.intersectionRatio * 100, | ||
| listener, shouldBeVisible)) { | ||
| listener, true)) { |
There was a problem hiding this comment.
/* shouldBeVisible */ true.
|
|
||
| const state = listener.state; | ||
| const lastChange = state[LAST_CHANGE_ENTRY]; | ||
| const lastVisible = lastChange ? lastChange.intersectionRatio * 100 : 0; |
There was a problem hiding this comment.
ditto: Inline comment for false
| this.updateCounters_(visible, listener, shouldBeVisible); | ||
|
|
||
| if (!shouldBeVisible) { | ||
| // For "hidden" trigger, only update state, don't trigger. |
There was a problem hiding this comment.
The way I understand this, it looks like the hidden conditions will never trigger when the page is in background and the visibility conditions start meeting. Is that correct?
There was a problem hiding this comment.
hidden trigger is triggered on visibilityChanged event. See line 372
There was a problem hiding this comment.
Yes, saw that. I think I might be understanding this wrong. So just want to make sure that I understand it correctly. What happens for following situation:
"hidden", minContinuousTime = 5 seconds
- Page is loaded, element is on screen for 2 seconds
- Page goes in background
- Does the trigger fire after 3 seconds or after the page comes to foreground again?
There was a problem hiding this comment.
in your example, since the spec is about "continuous" time. It will fire after page back to foregound for another 5 seconds.
|
|
||
| // Update states and check if all conditions are satisfied | ||
| if (this.updateCounters_(visible, listener, shouldBeVisible)) { | ||
| const conditionsMet = |
There was a problem hiding this comment.
This is being used only once. WHy move it into a variable?
There was a problem hiding this comment.
because we want this.updateCounters_ to run no matter of shouldBeVisible
There was a problem hiding this comment.
ooh. Missed that the if block was inside !shouldBeVisible. looks good.
| }); | ||
| }); | ||
|
|
||
| it('"hidden" trigger should work with duration condition', () => { |
There was a problem hiding this comment.
add a test for hidden trigger without duration conditions?
There was a problem hiding this comment.
Does it really make sense to have a hidden trigger without duration condition?
For example, what does it mean to have a spec like following?
"hidden": {
"on": "hidden",
"visibilitySpec": {
"selector": "#img-2",
"visiblePercentageMin": 10
},
}To me, it should probably be done with a "visible" trigger instead.
There was a problem hiding this comment.
Sorry I meant without any conditions at all. so something like this:
"hidden": {
"on": "hidden",
"visibilitySpec": {
"selector": "#img-2"
},
}
A test like that might exist but was not sure and hence brought it up.
There was a problem hiding this comment.
I'm not sure how exactly a spec like that should behave.
Does that mean "fires when page goes to background and the element has been visible"?
If so, I think a "visible" trigger should be used instead.
Or does that mean "fires when page goes to background and element is currently visible"? Why does people want to track that?
There was a problem hiding this comment.
In fact, I think the V1 code for hidden trigger does not take non-duration condition into consideration either.
I tested the V1 logic, your example spec behaves exactly the same as:
"hidden": {
"on": "hidden"
}There was a problem hiding this comment.
Interesting. I thought my example would mean: "fires when page goes to background and element is currently visible". Anyway, I think this is another edge case that we should document.
…ed) (ampproject#6634) * Implement "hidden" trigger for VisibilityV2 (IntersectionObserver based visibility tracking). * Give test cases better names. * Address comments. * Address comment. * Fix tests.
…ed) (ampproject#6634) * Implement "hidden" trigger for VisibilityV2 (IntersectionObserver based visibility tracking). * Give test cases better names. * Address comments. * Address comment. * Fix tests.
For #5697