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.

Sign up for GitHub

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

Jump to bottom

[HIG-2959] Fix: scrolling on elements being is ignored #93

Merged
merged 1 commit into from
Oct 17, 2022
Merged

[HIG-2959] Fix: scrolling on elements being is ignored #93

merged 1 commit into from
Oct 17, 2022

Conversation

Vadman97
Copy link
Member

In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the scrollTo api as it doesn't have that issue.

@Vadman97 Vadman97 marked this pull request as ready for review October 17, 2022 21:54
@Vadman97 Vadman97 requested review from a team, ccschmitz and mayberryzane and removed request for a team October 17, 2022 21:55
aptlin
aptlin approved these changes Oct 17, 2022
View reviewed changes
Copy link
Contributor

@aptlin aptlin left a comment

Choose a reason for hiding this comment

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

🚢

@Vadman97 Vadman97 changed the title Fix: scrolling on elements being is ignored [HIG-2959] Fix: scrolling on elements being is ignored Oct 17, 2022
@linear
Copy link

linear bot commented Oct 17, 2022

HIG-2959 rrweb scrolling conflicts with css body scrolling disabled

I think Highlight's session recording or playback might not be accounting for sites that disable body scrolling in favor of scrolling elements within the page with 100% height, using CSS that looks like this:
body,html,#root { height: 100%; }
https://reflame.app does this in order to be able to position elements relative to the full viewport as an alternative to using vh units, which has known issues with auto-hiding mobile address bars: Avoid 100vh On Mobile Web | chanind.github.io
The symptom in Highlight is I see a bunch of sessions (mostly on mobile?) where users are scrolling around on the screen but the viewport in the recording doesn't move (and actual scrolling has to be working since they're navigating around and clicking buttons and whatnot). Here's one example: Sessions: alex.mcgonagle.dev@gmail.com (highlight.run)

@aptlin
Copy link
Contributor

aptlin commented Oct 17, 2022

Should this go to #94 instead?

@Vadman97
Copy link
Member Author

Should this go to #94 instead?

@aptlin i'm keeping this commit as a separate PR on top of the newly-rebased master since this is a new change

@Juice10 @Vadman97
Fix: scrolling on elements being is ignored
7ec25a3 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed.
In this case we use the `scrollTo` api as it doesn't have that issue.
@Vadman97 Vadman97 merged commit 196a133 into master Oct 17, 2022
@Vadman97 Vadman97 deleted the vadim/reflame-fixes branch October 17, 2022 22:24
Vadman97 added a commit to highlight/highlight that referenced this pull request Oct 17, 2022
@Vadman97
update rrweb
5906566 
update to include HIG-2959 fix as well as updating rrweb fork.
see highlight/rrweb#94 and highlight/rrweb#93
@Vadman97 Vadman97 mentioned this pull request Oct 17, 2022
Merged
Vadman97 added a commit to highlight/highlight that referenced this pull request Oct 18, 2022
@Vadman97
[HIG-2959] update rrweb (#3200)
d35b22c 
## Summary

update to include HIG-2959 fix as well as updating rrweb fork. 
see highlight/rrweb#94 and highlight/rrweb#93

## How did you test this change?

Recording and replaying my own session with the new rrweb in client and frontend.
Canvas recording, obfuscation, normal record/replay look good.
https://frontend-pr-3200.onrender.com/1/sessions/ZvuQEPT1Hqrsmiqgb9sMrJlOheY0?page=1&query=and%7C%7Ccustom_created_at%2Cbetween_date%2C30%20days

## Are there any deployment considerations?

Bumped client version to keep track of new sessions.
Using https://static.highlight.run/beta/index.js to test new client for app.highlight.run recordings.
Will remove beta build after this is merged.
@Vadman97 Vadman97 mentioned this pull request Oct 18, 2022
Merged
Vadman97 added a commit to highlight/highlight that referenced this pull request Oct 18, 2022
@Vadman97
[HIG-2959] update rrweb with fix for stopping recording (#3211)
1c85bf8 
## Summary

update to include HIG-2959 fix as well as updating rrweb fork. 
see highlight/rrweb#94 and highlight/rrweb#93

See #3200 

Fixed by reverting rrweb-io/rrweb#962 which causes problems. Will be pointing out the observers issue there.

This also cleans up some client state logic which should make sure we do not call `rrweb.addCustomEvent` which we are not recording

## How did you test this change?

Before: 
![image](https://user-images.githubusercontent.com/1351531/196545769-12c0ff1f-44a6-40a5-8457-9f224785a7e3.png)

After:
![image](https://user-images.githubusercontent.com/1351531/196560173-8b550e2e-1b4d-48c8-a00e-04f69f62197f.png)


## Are there any deployment considerations?

Bumped client version.
Vadman97 added a commit that referenced this pull request Nov 1, 2022
@Vadman97
[HIG-2959] Fix: scrolling on elements being is ignored (#93)
c450d34 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the `scrollTo` api as it doesn't have that issue.
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
Vadman97 added a commit that referenced this pull request Nov 1, 2022
@Vadman97
[HIG-2959] Fix: scrolling on elements being is ignored (#93)
dabc50a 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the `scrollTo` api as it doesn't have that issue.
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
Vadman97 added a commit that referenced this pull request Nov 4, 2022
@Vadman97
[HIG-2959] Fix: scrolling on elements being is ignored (#93)
a915242 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the `scrollTo` api as it doesn't have that issue.
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
Vadman97 added a commit that referenced this pull request Nov 8, 2022
@Vadman97
[HIG-2959] Fix: scrolling on elements being is ignored (#93)
408da7d 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the `scrollTo` api as it doesn't have that issue.
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
Vadman97 added a commit that referenced this pull request Nov 8, 2022
@Vadman97
[HIG-2959] Fix: scrolling on elements being is ignored (#93)
0e89ef6 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the `scrollTo` api as it doesn't have that issue.
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
Vadman97 added a commit that referenced this pull request Nov 9, 2022
@Vadman97
[HIG-2959] Fix: scrolling on elements being is ignored (#93)
0d0a658 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the `scrollTo` api as it doesn't have that issue.
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
Vadman97 added a commit that referenced this pull request Nov 11, 2022
@Vadman97
[HIG-2959] Fix: scrolling on elements being is ignored (#93)
db45de2 
In certain cases when scrollLeft is being set, but the value doesn't change. Then scrollTop is also ignored, even if that value was changed. In this case we use the `scrollTo` api as it doesn't have that issue.
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aptlin aptlin aptlin approved these changes

@ccschmitz ccschmitz Awaiting requested review from ccschmitz ccschmitz was automatically assigned from highlight/full-stack-devs

@mayberryzane mayberryzane Awaiting requested review from mayberryzane mayberryzane was automatically assigned from highlight/full-stack-devs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vadman97 @aptlin @Juice10