ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Intent to Implement: Allow Timer Trigger to be aware of page visibility #4844

Closed dynes closed 7 years ago

dynes commented 8 years ago

Currently, there is no way to determine if the page is visible for a timer trigger.

The use case that this fits into is only wanting to send a timer event when the page is visible to know when the user is actively engaging/looking at the page. Unfortunately, using totalEngagedTime does not allow for this as it does not increase if the user does not cause a page event. A user could be actively looking at the page without scrolling, clicking, etc.

There are two potential solutions for the use case.

One, add a new optional field, whenVisible, to the timerSpec:

"timerSpec": { "interval": 5, "maxTimerLength": 20, "whenVisible": true },

If whenVisible is set to true, the Timer Trigger will not fire.

Or, second, add the backgrounded variable that exists for a Visibility Trigger to the Timer Trigger. This would allow the endpoint to decide what to do with a backgrounded timer event.

jpettitt commented 8 years ago

Knowing how much foreground time a page had would be really useful along with how many discreet visibility periods made up the time needed to fire the event. That would let use get a sense of user behavior in the carousel - are they swiping past our articles and coming back etc.

ericlindley-g commented 7 years ago

cc/ @rudygalfi

dynes commented 7 years ago

Should I submit a pull request with potential changes, or does a discussion need to take place for what is the better approach?

avimehta commented 7 years ago

@dynes Is there a desire to track visibility of a particular event for timer or just the whole page?

I like the idea of making backgrounded available to all of AMP instead of being a visibility only variable. @rudygalfi what do you think?

dynes commented 7 years ago

@avimehta For my use case, tracking visibility for the whole page.

dynes commented 7 years ago

5240

Pull request linked.

rudygalfi commented 7 years ago

The documentation for backgrounded says: "A binary variable with possible values of 1 and 0 to indicate that the page/tab was sent to background at any point before the hit was sent. 1 indicates that the page was backgrounded while 0 indicates that the page has always been in the foreground."

Does a backgrounding ever in the page lifecycle make this return 1 for the rest of the page lifecycle, or can it reset to 0? For instance, I'm thinking that with the timer trigger, you might want the value to go to 1 during an interval when the page was backgrounded, but then return to 0 for a hit where the page was foregrounded again during the entire interval.

If the values are monotonic, then I don't think that behavior would meet the original feature request. However, adding a config to the timer itself would allow for this kind of counting to happen.

dynes commented 7 years ago

When you say counting you would prefer more than just a binary value? Something along the lines of, a count of requests since backgrounded that gets reset whenever the window is foregrounded?

rudygalfi commented 7 years ago

I'm OK with it being a binary value. I was just pointing out that my reading of backgrounded suggests that once the page meets the "backgrounded" condition, the value will always be 1 for future hits. However, if the desire is to count something like engaged time, then the value of backgrounded should reset to 0 once the page is back in the foreground, if it's desired to use backgrounded in this way.

dynes commented 7 years ago

Yes, the desire would to reset backgrounded to 0 on foregrounded. I have attached the pull request to should satisfy that requirement. Unfortunately, the CI test failed, and I cannot reproduce that locally. Is there a way to kick off the CI build again, or do I need to submit another commit in order to do that?

dynes commented 7 years ago

Pull request has now passed CI.

rudygalfi commented 7 years ago

Added @avimehta to review, although it looks like @jridgewell is taking a look as well.