update scroll logic

packages/rum-core/src/browser/scroll.spec.ts

@@ -1,6 +1,6 @@
 import { addEventListener, DOM_EVENT, isIE } from '@datadog/browser-core'
 import type { RumConfiguration } from '../domain/configuration'
-import { getScrollHeight, getScrollX, getScrollY } from './scroll'
+import { getScrollX, getScrollY } from './scroll'
 
 function isMobileSafari12() {
   return /iPhone OS 12.* like Mac OS.* Version\/12.* Mobile.*Safari/.test(navigator.userAgent)
@@ -66,11 +66,4 @@ describe('scroll', () => {
       expect(getScrollY()).toBe(window.scrollY || window.pageYOffset)
     })
   })
-
-  describe('getScrollHeight', () => {
-    it('should return the scroll height', () => {
-      addVerticalScrollBar()
-      expect(getScrollHeight()).toBe(5026)
-    })
-  })
 })

packages/rum-core/src/browser/scroll.ts

@@ -23,5 +23,3 @@ export function getScrollY() {
   }
   return Math.round(scrollY)
 }
-
-export const getScrollHeight = () => Math.round((document.scrollingElement || document.documentElement).scrollHeight)

packages/rum-core/src/domain/rumEventsCollection/view/viewMetrics/trackCommonViewMetrics.ts

@@ -1,6 +1,5 @@
 import type { ClocksState, Duration, Observable } from '@datadog/browser-core'
 import { noop } from '@datadog/browser-core'
-import { createScrollHeightObservable } from '../../../../browser/scrollHeightObservable'
 import type { ViewLoadingType } from '../../../../rawRumEvent.types'
 import type { RumConfiguration } from '../../../configuration'
 import type { LifeCycle } from '../../../lifeCycle'
@@ -47,7 +46,6 @@ export function trackCommonViewMetrics(
     (newScrollMetrics) => {
       commonViewMetrics.scroll = newScrollMetrics
     },
-    createScrollHeightObservable(),
     computeScrollValues
   )
 

packages/rum-core/src/domain/rumEventsCollection/view/viewMetrics/trackScrollMetrics.spec.ts

@@ -1,17 +1,36 @@
-import type { RelativeTime, TimeStamp, Duration } from '@datadog/browser-core'
-import { Observable, DOM_EVENT } from '@datadog/browser-core'
-import type { Clock } from '@datadog/browser-core/test'
-import { createNewEvent, mockClock } from '@datadog/browser-core/test'
+import type { Duration, RelativeTime, TimeStamp } from '@datadog/browser-core'
+import { DOM_EVENT } from '@datadog/browser-core'
+import { collectAsyncCalls, createNewEvent } from '@datadog/browser-core/test'
 import type { TestSetupBuilder } from '../../../../../test'
 import { setup } from '../../../../../test'
 import type { RumConfiguration } from '../../../configuration'
 import type { ViewTest } from '../setupViewTest.specHelper'
 import { setupViewTest } from '../setupViewTest.specHelper'
-import { THROTTLE_SCROLL_DURATION, type ScrollMetrics, trackScrollMetrics } from './trackScrollMetrics'
+import { trackScrollMetrics } from './trackScrollMetrics'
 
 describe('trackScrollMetrics', () => {
   let setupBuilder: TestSetupBuilder
   let viewTest: ViewTest
+  let stopTrackScrollMetrics: () => void
+  let callbackSpy: jasmine.Spy
+  let assertCallbackSpyEqual: (expected: any, done: DoneFn, times?: number) => void
+  const getMetrics = jasmine.createSpy('getMetrics')
+
+  const mockGetMetrics = (scrollParams: { scrollHeight: number; scrollDepth: number; scrollTop: number }) => {
+    getMetrics.and.returnValue(scrollParams)
+  }
+
+  const newScroll = (scrollParams: { scrollHeight: number; scrollDepth: number; scrollTop: number }) => {
+    mockGetMetrics(scrollParams)
+    window.dispatchEvent(createNewEvent(DOM_EVENT.SCROLL))
+  }
+
+  const increaseHeight = (scrollParams: { scrollHeight: number; scrollDepth: number; scrollTop: number }) => {
+    mockGetMetrics(scrollParams)
+    const node = document.createElement('div')
+    node.style.height = `${scrollParams.scrollHeight}px`
+    document.body.replaceChildren(node)
+  }
 
   beforeEach(() => {
     setupBuilder = setup()
@@ -20,121 +39,85 @@ describe('trackScrollMetrics', () => {
         viewTest = setupViewTest(buildContext)
         return viewTest
       })
+    callbackSpy = jasmine.createSpy('callback')
+    stopTrackScrollMetrics = trackScrollMetrics(
+      {} as RumConfiguration,
+      { relative: 0 as RelativeTime, timeStamp: 0 as TimeStamp },
+      callbackSpy,
+      getMetrics,
+      0
+    ).stop
+    document.body.style.margin = '0'
+
+    assertCallbackSpyEqual = (expected, done, times = 1) => {
+      collectAsyncCalls(callbackSpy, times, (calls) => {
+        expect(calls.mostRecent().args[0]).toEqual(expected)
+        done()
+      })
+    }
   })
 
   afterEach(() => {
+    stopTrackScrollMetrics()
+    document.body.replaceChildren()
     setupBuilder.cleanup()
   })
 
-  describe('on scroll', () => {
-    let scrollMetrics: ScrollMetrics | undefined
-    let stopTrackScrollMetrics: () => void
-    let clock: Clock
-    let configuration: RumConfiguration
-    const scrollHeightObservable: Observable<number> = new Observable()
-
-    const getMetrics = jasmine.createSpy('getMetrics')
-
-    const mockGetMetrics = (scrollParams: { scrollHeight: number; scrollDepth: number; scrollTop: number }) => {
-      getMetrics.and.returnValue(scrollParams)
-    }
-
-    const emitObservableValue = (scrollParams: { scrollHeight: number; scrollDepth: number; scrollTop: number }) => {
-      mockGetMetrics({ ...scrollParams, scrollHeight: 0 })
-      scrollHeightObservable.notify(scrollParams.scrollHeight)
-    }
-
-    const newScroll = (scrollParams: { scrollHeight: number; scrollDepth: number; scrollTop: number }) => {
-      mockGetMetrics(scrollParams)
-      window.dispatchEvent(createNewEvent(DOM_EVENT.SCROLL))
-      clock.tick(THROTTLE_SCROLL_DURATION)
-    }
-
-    beforeEach(() => {
-      configuration = {} as RumConfiguration
-      clock = mockClock()
-      stopTrackScrollMetrics = trackScrollMetrics(
-        configuration,
-        { relative: 0 as RelativeTime, timeStamp: 0 as TimeStamp },
-        (s) => (scrollMetrics = s),
-        scrollHeightObservable,
-        getMetrics
-      ).stop
-    })
-
-    afterEach(() => {
-      stopTrackScrollMetrics()
-      scrollMetrics = undefined
-      clock.cleanup()
-    })
-
-    it('should initially be undefined', () => {
-      expect(scrollMetrics).toBeUndefined()
-    })
-
-    it('should update scroll metrics before scrolling', () => {
-      emitObservableValue({
-        scrollHeight: 99,
-        scrollDepth: 99,
-        scrollTop: 99,
-      })
-      expect(scrollMetrics).toEqual({
-        maxDepthScrollHeight: 99,
-        maxDepth: 99,
-        maxDepthScrollTop: 99,
-        maxDepthTime: 0 as Duration,
-      })
-    })
+  it('should update scroll height if resize occurred before scroll', (done) => {
+    increaseHeight({ scrollDepth: 0, scrollHeight: 2000, scrollTop: 0 })
+    assertCallbackSpyEqual(
+      {
+        maxDepthScrollHeight: 2000,
+        maxDepth: 0,
+        maxDepthScrollTop: 0,
+        maxDepthTime: jasmine.any(Number),
+      },
+      done
+    )
+  })
 
-    it('should update scroll metrics when scrolling the first time, ignoring subsequent values emitted by the observable', () => {
-      newScroll({ scrollHeight: 1000, scrollDepth: 500, scrollTop: 100 })
-      emitObservableValue({
-        scrollHeight: 9999,
-        scrollDepth: 9999,
-        scrollTop: 9999,
-      })
-      expect(scrollMetrics).toEqual({
-        maxDepthScrollHeight: 1000,
+  it('should update scroll depth when scrolling the first time', (done) => {
+    newScroll({ scrollHeight: 1000, scrollDepth: 500, scrollTop: 100 })
+    assertCallbackSpyEqual(
+      {
         maxDepth: 500,
         maxDepthScrollTop: 100,
-        maxDepthTime: 1000 as Duration,
-      })
-    })
-
-    it('should update scroll metrics when scroll depth has increased, still ignoring values emitted by the observable', () => {
-      newScroll({ scrollHeight: 1000, scrollDepth: 500, scrollTop: 100 })
-      newScroll({ scrollHeight: 1000, scrollDepth: 600, scrollTop: 200 })
+        maxDepthScrollHeight: 0,
+        maxDepthTime: 0 as Duration,
+      },
+      done
+    )
+  })
 
-      emitObservableValue({
-        scrollHeight: 9999,
-        scrollDepth: 9999,
-        scrollTop: 9999,
-      })
+  it('should update scroll depth when it has increased', (done) => {
+    newScroll({ scrollHeight: 1000, scrollDepth: 500, scrollTop: 100 })
+    setTimeout(() => newScroll({ scrollHeight: 1000, scrollDepth: 600, scrollTop: 200 }), 100)
 
-      expect(scrollMetrics).toEqual({
-        maxDepthScrollHeight: 1000,
+    assertCallbackSpyEqual(
+      {
         maxDepth: 600,
         maxDepthScrollTop: 200,
-        maxDepthTime: 2000 as Duration,
-      })
-    })
-
-    it('should NOT update scroll metrics when scroll depth has not increased', () => {
-      newScroll({ scrollHeight: 1000, scrollDepth: 600, scrollTop: 200 })
-      newScroll({ scrollHeight: 1000, scrollDepth: 450, scrollTop: 50 })
+        maxDepthScrollHeight: 0,
+        maxDepthTime: 0 as Duration,
+      },
+      done,
+      2
+    )
+  })
 
-      emitObservableValue({
-        scrollHeight: 9999,
-        scrollDepth: 9999,
-        scrollTop: 9999,
-      })
+  it('should update scroll depth and scroll height when both scroll and resize have occured ', (done) => {
+    newScroll({ scrollHeight: 1000, scrollDepth: 600, scrollTop: 200 })
+    setTimeout(() => increaseHeight({ scrollHeight: 2000, scrollDepth: 600, scrollTop: 200 }), 100)
 
-      expect(scrollMetrics).toEqual({
-        maxDepthScrollHeight: 1000,
+    assertCallbackSpyEqual(
+      {
         maxDepth: 600,
         maxDepthScrollTop: 200,
-        maxDepthTime: 1000 as Duration,
-      })
-    })
+        maxDepthScrollHeight: 2000,
+        maxDepthTime: jasmine.any(Number),
+      },
+      done,
+      2
+    )
   })
 })