Merge pull request #177 from googleanalytics/page-visibility-bugs

Fix extra long pageVisibilityTracker visible time reporting

gulpfile.js

@@ -26,6 +26,7 @@ const gulp = require('gulp');
 const gutil = require('gulp-util');
 const webdriver = require('gulp-webdriver');
 const gzipSize = require('gzip-size');
+const path = require('path');
 const {rollup} = require('rollup');
 const nodeResolve = require('rollup-plugin-node-resolve');
 const babel = require('rollup-plugin-babel');
@@ -90,7 +91,7 @@ gulp.task('javascript:unit', ((compiler) => {
     return webpack({
       entry: glob.sync('./test/unit/**/*-test.js'),
       output: {
-        path: 'test/unit',
+        path: path.resolve(__dirname, 'test/unit'),
         filename: 'index.js',
       },
       devtool: '#source-map',

lib/externs/max-scroll-tracker.js

@@ -11,6 +11,13 @@
  */
 var MaxScrollTrackerOpts;
 
+/**
+ * MaxScrollTracker store data schema.
+ * @typedef {{
+ *   sessionId: (string|undefined),
+ * }}
+ */
+var MaxScrollStoreData;
 
 /**
  * @interface

lib/externs/page-visibility-tracker.js

@@ -20,6 +20,7 @@ var PageVisibilityTrackerOpts;
  *   time: (number|undefined),
  *   state: (string|undefined),
  *   pageId: (string|undefined),
+ *   sessionId: (string|undefined),
  * }}
  */
 var PageVisibilityStoreData;

lib/externs/session.js

@@ -1,8 +1,8 @@
 /**
  * Session store data schema.
  * @typedef {{
+ *   id: (string|undefined),
  *   hitTime: (number|undefined),
- *   sessionCount: (number|undefined),
  *   isExpired: (boolean|undefined),
  * }}
  */

lib/plugins/max-scroll-tracker.js

@@ -65,7 +65,7 @@ class MaxScrollTracker {
         tracker.get('trackingId'), 'plugins/max-scroll-tracker');
 
     // Creates the session and binds session events.
-    this.session = new Session(
+    this.session = Session.getOrCreate(
         tracker, this.opts.sessionTimeout, this.opts.timeZone);
 
     // Override the built-in tracker.set method to watch for changes.
@@ -109,8 +109,19 @@ class MaxScrollTracker {
     const scrollPercentage = Math.min(100, Math.max(0,
         Math.round(100 * (scrollPos / (pageHeight - windowHeight)))));
 
-    // If the session has expired, clear old scroll data and send no events.
-    if (this.session.isExpired()) {
+    // If the max scroll data gets out of the sync with the session data
+    // (for whatever reason), clear it.
+    const sessionId = this.session.getId();
+    if (sessionId != this.store.get().sessionId) {
+      this.store.clear();
+      this.store.set({sessionId});
+    }
+
+    // If the session has expired, clear the stored data and don't send any
+    // events (since they'd start a new session). Note: this check is needed,
+    // in addition to the above check, to handle cases where the session IDs
+    // got out of sync, but the session didn't expire.
+    if (this.session.isExpired(this.store.get().sessionId)) {
       this.store.clear();
     } else {
       const maxScrollPercentage = this.getMaxScrollPercentageForCurrentPage();
@@ -187,7 +198,10 @@ class MaxScrollTracker {
    * @param {number} maxScrollPercentage
    */
   setMaxScrollPercentageForCurrentPage(maxScrollPercentage) {
-    this.store.set({[this.pagePath]: maxScrollPercentage});
+    this.store.set({
+      [this.pagePath]: maxScrollPercentage,
+      sessionId: this.session.getId(),
+    });
   }
 
   /**

lib/plugins/page-visibility-tracker.js

@@ -63,7 +63,7 @@ class PageVisibilityTracker {
         assign(defaultOpts, opts));
 
     this.tracker = tracker;
-    this.lastPageState = null;
+    this.lastPageState = document.visibilityState;
     this.visibleThresholdTimeout_ = null;
     this.isInitialPageviewSent_ = false;
 
@@ -79,15 +79,14 @@ class PageVisibilityTracker {
     this.store.on('externalSet', this.handleExternalStoreSet);
 
     // Creates the session and binds session events.
-    this.session = new Session(
+    this.session = Session.getOrCreate(
         tracker, this.opts.sessionTimeout, this.opts.timeZone);
 
     // Override the built-in tracker.set method to watch for changes.
     MethodChain.add(tracker, 'set', this.trackerSetOverride);
 
     window.addEventListener('unload', this.handleWindowUnload);
     document.addEventListener('visibilitychange', this.handleChange);
-    this.handleChange();
 
     // Postpone sending any hits until the next call stack, which allows all
     // autotrack plugins to be required sync before any hits are sent.
@@ -97,6 +96,12 @@ class PageVisibilityTracker {
           this.sendPageview({isPageLoad: true});
           this.isInitialPageviewSent_ = true;
         }
+        this.store.set(/** @type {PageVisibilityStoreData} */ ({
+          time: now(),
+          state: VISIBLE,
+          pageId: PAGE_ID,
+          sessionId: this.session.getId(),
+        }));
       } else {
         if (this.opts.sendInitialPageview && this.opts.pageLoadsMetricIndex) {
           this.sendPageLoad();
@@ -121,32 +126,33 @@ class PageVisibilityTracker {
       return;
     }
 
-    const lastStoredChange = this.validateChangeData(this.store.get());
+    const lastStoredChange = this.getAndValidateChangeData();
 
     /** @type {PageVisibilityStoreData} */
     const change = {
       time: now(),
       state: document.visibilityState,
       pageId: PAGE_ID,
+      sessionId: this.session.getId(),
     };
 
     // If the visibilityState has changed to visible and the initial pageview
     // has not been sent (and the `sendInitialPageview` option is `true`).
     // Send the initial pageview now.
-    if (this.lastPageState &&
-        document.visibilityState == VISIBLE &&
+    if (document.visibilityState == VISIBLE &&
         this.opts.sendInitialPageview && !this.isInitialPageviewSent_) {
       this.sendPageview();
       this.isInitialPageviewSent_ = true;
     }
 
     // If the visibilityState has changed to hidden, clear any scheduled
     // pageviews waiting for the visibleThreshold timeout.
-    if (this.visibleThresholdTimeout_ && document.visibilityState == HIDDEN) {
+    if (document.visibilityState == HIDDEN && this.visibleThresholdTimeout_) {
       clearTimeout(this.visibleThresholdTimeout_);
     }
 
-    if (this.session.isExpired()) {
+    if (this.session.isExpired(lastStoredChange.sessionId)) {
+      this.store.clear();
       if (this.lastPageState == HIDDEN &&
           document.visibilityState == VISIBLE) {
         // If the session has expired, changes from hidden to visible should
@@ -165,10 +171,6 @@ class PageVisibilityTracker {
           this.store.set(change);
           this.sendPageview({hitTime: change.time});
         }, this.opts.visibleThreshold);
-      } else if (document.visibilityState == HIDDEN) {
-        // Hidden events should never be sent if a session has expired (if
-        // they are, they'll likely start a new session with just this event).
-        this.store.clear();
       }
     } else {
       if (lastStoredChange.pageId == PAGE_ID &&
@@ -195,10 +197,12 @@ class PageVisibilityTracker {
    * retroactively update the stored data to reflect the current page as being
    * the page on which the last change event occured and measure visibility
    * from that point.
-   * @param {PageVisibilityStoreData} lastStoredChange
-   * @return {PageVisibilityStoreData}
+   * @return {!PageVisibilityStoreData}
    */
-  validateChangeData(lastStoredChange) {
+  getAndValidateChangeData() {
+    const lastStoredChange =
+        /** @type {PageVisibilityStoreData} */ (this.store.get());
+
     if (this.lastPageState == VISIBLE &&
         lastStoredChange.state == HIDDEN &&
         lastStoredChange.pageId != PAGE_ID) {
@@ -213,7 +217,7 @@ class PageVisibilityTracker {
    * Sends a Page Visibility event to track the time this page was in the
    * visible state (assuming it was in that state long enough to meet the
    * threshold).
-   * @param {PageVisibilityStoreData} lastStoredChange
+   * @param {!PageVisibilityStoreData} lastStoredChange
    * @param {{hitTime: (number|undefined)}=} param1
    *     - hitTime: A hit timestap used to help ensure original order in cases
    *                where the send is delayed.
@@ -322,7 +326,7 @@ class PageVisibilityTracker {
    * @return {number} The time (in ms) since the last change.
    */
   getTimeSinceLastStoredChange(lastStoredChange, {hitTime} = {}) {
-    return lastStoredChange.time && !this.session.isExpired() ?
+    return lastStoredChange.time ?
         (hitTime || now()) - lastStoredChange.time : 0;
   }
 
@@ -347,7 +351,8 @@ class PageVisibilityTracker {
     // this page is the one that needs to send the event (so all dimension
     // data is correct).
     if (oldData.pageId == PAGE_ID &&
-        oldData.state == VISIBLE) {
+        oldData.state == VISIBLE &&
+        !this.session.isExpired(oldData.sessionId)) {
       this.sendPageVisibilityEvent(oldData, {hitTime: newData.time});
     }
   }