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

⚗[REPLAY-341] Add VisualViewport tracking (Pinch Zoom) #1118

Merged
merged 40 commits into from
Nov 9, 2021

Conversation

jagracey
Copy link
Contributor

@jagracey jagracey commented Oct 6, 2021

Motivation

Support pinch zoom and pinch scroll by tracking the "visual viewport".

As mentioned in the internal RFC "Supporting Pinch Zoom Tracking", unfortunately, the dimensions we currently record are not consistent across devices and browsers as some are referenced. On certain devices (primarily mobile), when users pinch zoom, the replay will replay incorrectly- displaying the layout viewport but not the visual viewport.

Unfortunately, browsers have not standardised various dimension properties. Mobile devices typically report dimensions in reference to the visual viewport, while desktop uses the layout viewport. For example, mobile Chrome will change innerWidth when a pinch zoom takes place, while Chrome desktop (mac) will not

With the new Viewport API, we now calculate and normalise dimension properties to the layout viewport.

Note: If the VisualViewport API is not supported by a browser, it isn't reasonably possible to detect or normalise which viewport is being measured. Therefore these exported functions will fallback to assuming that the layout viewport is being measured by the browser

Changes

  1. Adds VisualViewport record as incremental record.
  2. Changes innerWidth/innerHeight, scrollX/scrollY, and mouse positions to a normalised value that is referenced to the layout viewport (not pinch zoom's visual viewport) so that we do not factor in the visual viewport (pinch zoom) twice for mobile devices.

Testing

To support pinch zoom and pinch scroll unit tests, we use utilise our E2E testing framework to use Appium's gesture commands.

Unfortunately there are many pitfalls to be aware of that required work arounds:

  • Scrolling behaviour may disappear the address bar changing screen height
  • Address bar height can persist across page changes (record flushing)
  • FullScreen mode isn't consistent with regard to pinch zoom
  • Pinch/touch gestures must not exceed the bounds of the page, so are commonly repeated together.
  • Pinch scroll is inverted on iOS vs Android
  • Click() method will trigger a scroll into view, so testing the coordinate system of layout vs visual viewports for clicks doesn't appear possible
  • Chrome Linux doesn't support the meta viewport tag to limit max scale
  • Chrome Linux innerWidth vs visual.width*scale appears to differ by 15 pixels likely due to scroll bars.

I have gone over the contributing documentation.

@jagracey jagracey requested review from a team as code owners October 6, 2021 14:58
@jagracey
Copy link
Contributor Author

jagracey commented Oct 6, 2021

Reopened from #1118

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #1118 (a91e14d) into main (514c1a2) will decrease coverage by 0.70%.
The diff coverage is 36.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1118      +/-   ##
==========================================
- Coverage   89.13%   88.43%   -0.71%     
==========================================
  Files          93       94       +1     
  Lines        4197     4253      +56     
  Branches      964      969       +5     
==========================================
+ Hits         3741     3761      +20     
- Misses        456      492      +36     
Impacted Files Coverage Δ
packages/rum/src/domain/record/types.ts 100.00% <ø> (ø)
packages/rum/src/domain/record/utils.ts 100.00% <ø> (+16.66%) ⬆️
packages/rum/src/types.ts 100.00% <ø> (ø)
packages/rum/test/utils.ts 83.22% <0.00%> (-1.05%) ⬇️
packages/rum/src/domain/record/observer.ts 64.70% <23.33%> (-7.52%) ⬇️
packages/rum/src/domain/record/record.ts 66.66% <25.00%> (+7.14%) ⬆️
packages/rum/src/domain/record/viewports.ts 47.72% <47.72%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 514c1a2...a91e14d. Read the comment docs.

packages/rum/src/domain/record/observer.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/observer.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/viewports.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/viewports.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/viewports.spec.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/observer.ts Show resolved Hide resolved
packages/rum/src/domain/record/viewports.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/viewports.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/viewports.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/viewports.ts Outdated Show resolved Hide resolved
@jagracey jagracey force-pushed the john.gracey/pinch-zoom-support branch from 5005e46 to 50786d9 Compare October 8, 2021 09:08
return data

support old browsers

add debug console

Debug cross-browser unit test
PR review updates

`window.viewport` breaks build

lint
PR review fix
Bump Safari Mobile version to v13

BrowserStack device

Block Safari 12 unit test: doesnt support scrollTo within iframes
@jagracey jagracey force-pushed the john.gracey/pinch-zoom-support branch from 50786d9 to e3d75ac Compare October 8, 2021 10:48
jagracey and others added 8 commits October 8, 2021 14:12
return data

support old browsers

add debug console

Debug cross-browser unit test
PR review updates

`window.viewport` breaks build

lint
PR review fix
Bump Safari Mobile version to v13

BrowserStack device

Block Safari 12 unit test: doesnt support scrollTo within iframes
@jagracey jagracey marked this pull request as draft October 11, 2021 08:36
@jagracey
Copy link
Contributor Author

jagracey commented Oct 11, 2021

As per IRL discussions: PR drafted while we reprioritise unit testing synthetic gestures (pinch zoom + pinch scroll) via the exposed browser automation hooks in our E2E test suite.

jagracey and others added 5 commits October 11, 2021 11:05
@jagracey jagracey marked this pull request as ready for review October 19, 2021 08:39
@bcaudan bcaudan requested review from a team October 20, 2021 08:01
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
packages/rum/test/utils.ts Outdated Show resolved Hide resolved
test/e2e/lib/helpers/browser.ts Outdated Show resolved Hide resolved
@jagracey jagracey requested a review from bcaudan October 22, 2021 13:22
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/viewports.scenario.ts Outdated Show resolved Hide resolved
@jagracey jagracey requested a review from bcaudan November 5, 2021 09:30
Copy link
Contributor

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

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

LGTM

packages/rum/src/domain/record/record.ts Outdated Show resolved Hide resolved
Comment on lines +37 to +39
await browserExecute(() => {
window.dispatchEvent(new Event('resize'))
})
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member

Choose a reason for hiding this comment

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

For the record: this is to generate a ViewportResize event without having to explicitly resize the browser (which may be more complex/flaky). LGTM

test/e2e/scenario/recorder/viewports.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder/viewports.scenario.ts Outdated Show resolved Hide resolved
@jagracey jagracey merged commit 3e790f2 into main Nov 9, 2021
@jagracey jagracey deleted the john.gracey/pinch-zoom-support branch November 9, 2021 08:39
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.

4 participants