Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle bootup in background tab edge case #129

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

seanjohnson08
Copy link
Collaborator

@xg-wang recently discovered an edge case where by:

  • opening an app in a backgrounded tab
  • closing that tab

Spaniel will fire Impression events in both Safari and Firefox. It is currently unknown why this doesn't happen in Chrome.

Previously, Spaniel assumes that the app is visible (and changes this state in response to the browsers visibilitychange event).
This PR ensures that Spaniel does not assume the app is visible.

Copy link
Contributor

@asakusuma asakusuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should also test to make sure that the visibleTime and duration are correct when the flow is:

  1. Page starts hidden
  2. Page becomes visible
  3. Items are impressed
  4. Items leave viewport and hooks are fired

So basically verify that at step 4, the duration and visibleTime metadata doesn't count the time between step 1 and 2

test/specs/spaniel-observer.spec.js Show resolved Hide resolved
Copy link
Contributor

@asakusuma asakusuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with sean offline, he'll do the additional metadata test in a follow up PR

@seanjohnson08 seanjohnson08 merged commit b0c2207 into linkedin:v2-release Sep 14, 2020
@xg-wang
Copy link
Contributor

xg-wang commented Sep 15, 2020

The fix is not sufficient, any work scheduled by raf (https://github.com/linkedin/spaniel/blob/v2.5.2/src/spaniel-observer.ts#L127) should also check hidden state, only the callback from hide event callback should be allowed to run in a hidden state.


edit: hmm from a quick test FF tells state visible in the background raf in this scenario, looks like we cannot do anything in this edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants