-
Notifications
You must be signed in to change notification settings - Fork 58
fix(plugin-autocapture-browser): do not trigger rage click when text being highlighted #1471
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
Open
daniel-graham-amplitude
wants to merge
50
commits into
main
Choose a base branch
from
AMP-146044-fix-highlightable-text-rage-clicks
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
50 commits
Select commit
Hold shift + click to select a range
4a01050
chore: start migration from Yarn -> PNPM (#1443)
daniel-graham-amplitude 5ed0db2
chore: use PNPM workspaces (#1446)
daniel-graham-amplitude 244d9db
chore: migrate publish workflows to PNPM (#1448)
daniel-graham-amplitude 253e268
docs: update documentation to use pnpm over yarn (#1449)
daniel-graham-amplitude bfe323e
merge from main
daniel-graham-amplitude 7b1baaa
fix: action use pnpm over yarn
daniel-graham-amplitude 87c90f1
fix: apply rage clicks to window over viewport
daniel-graham-amplitude afc8b78
fix test
daniel-graham-amplitude 7ff875d
chore: update publish tag
daniel-graham-amplitude 7c1a535
chore: fix -- problem with pnpm (#1462)
daniel-graham-amplitude b6afc2a
chore(release): publish
amplitude-sdk-dev 359ec7c
chore: remove unknown args pnpm publish
daniel-graham-amplitude 2f2e86b
chore: rebase pnpm-migration
daniel-graham-amplitude c2808e9
Revert "chore(release): publish"
daniel-graham-amplitude d36dc92
chore: clean up PR
daniel-graham-amplitude 003570e
chore: clean up PR
daniel-graham-amplitude 771aada
dummy commit
daniel-graham-amplitude 6c9c7d6
chore: fix git-checks version to publish
daniel-graham-amplitude db34376
empty
daniel-graham-amplitude b8a027e
Revert "empty"
daniel-graham-amplitude 935fc68
Merge branch 'main' of github.com:amplitude/Amplitude-TypeScript into…
daniel-graham-amplitude 0a24f2d
chore: fix git-checks version to publish
daniel-graham-amplitude 96defd0
again
daniel-graham-amplitude 3feb114
chore: fix git-checks version to publish
daniel-graham-amplitude cdbcdb2
Merge branch 'main' of github.com:amplitude/Amplitude-TypeScript into…
daniel-graham-amplitude 07b1239
Merge branch 'pnpm-migration' of github.com:amplitude/Amplitude-TypeS…
daniel-graham-amplitude 6749750
fix: implement highlight text fix
daniel-graham-amplitude eba9524
chore(release): publish
amplitude-sdk-dev ce48dcd
fix: add selection observer to cancel out rage clicks
daniel-graham-amplitude e974e48
chore: add selection observable test
daniel-graham-amplitude e8bab23
again
daniel-graham-amplitude 438d897
chore: add "auto" yes to version
daniel-graham-amplitude 66ffd36
chore: clean up PR
daniel-graham-amplitude d8c094d
Merge branch 'main' of github.com:amplitude/Amplitude-TypeScript into…
daniel-graham-amplitude bd7598b
fix: test coverage
daniel-graham-amplitude d0cc2e7
fix: make selection unsubscribable too
daniel-graham-amplitude a6793e0
chore: make unit tests leaner
daniel-graham-amplitude eb2a3ba
again
daniel-graham-amplitude 2e728e5
refactor tests
daniel-graham-amplitude 73f6737
chore: fix deploy script
daniel-graham-amplitude c3e9774
refactor tests
daniel-graham-amplitude 454f675
add back targeting manager
daniel-graham-amplitude 9ef44ba
fix: apply rage clicks to window over viewport
daniel-graham-amplitude a2076e4
fix test
daniel-graham-amplitude 2f9f9a5
chore: cleanup rage click tests
daniel-graham-amplitude fe3dc36
Merge branch 'AMP-146045-rage-click-fix-mobile-scroll-false-positive'…
daniel-graham-amplitude 8a4fbdd
Merge branch 'main' of github.com:amplitude/Amplitude-TypeScript into…
daniel-graham-amplitude 961ca02
Merge branch 'AMP-146045-rage-click-fix-mobile-scroll-false-positive'…
daniel-graham-amplitude eaeb3d1
Merge branch 'main' of github.com:amplitude/Amplitude-TypeScript into…
daniel-graham-amplitude 9eb5742
fix: handle selectionStart exceptions
daniel-graham-amplitude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ export interface AllWindowObservables { | |
| [ObservablesEnum.ClickObservable]: Observable<ElementBasedTimestampedEvent<MouseEvent>>; | ||
| [ObservablesEnum.MutationObservable]: Observable<TimestampedEvent<MutationRecord[]>>; | ||
| [ObservablesEnum.NavigateObservable]?: Observable<TimestampedEvent<NavigateEvent>>; | ||
| [ObservablesEnum.SelectionObservable]?: Observable<void>; | ||
| } | ||
|
|
||
| type BrowserEnrichmentPlugin = EnrichmentPlugin<BrowserClient, BrowserConfig>; | ||
|
|
@@ -91,10 +92,44 @@ export const frustrationPlugin = (options: FrustrationInteractionsOptions = {}): | |
| ); | ||
| } | ||
|
|
||
| const selectionObservable = multicast( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could use more comments here as code to handle selections is not as common. Try to explain high level approach |
||
| new Observable<void>((observer) => { | ||
| const handler = () => { | ||
| // handle input and textarea | ||
| const el: HTMLElement | null = document.activeElement as HTMLElement; | ||
|
|
||
| if (el && (el.tagName === 'TEXTAREA' || el.tagName === 'INPUT')) { | ||
| let start: number | null | undefined; | ||
| let end: number | null | undefined; | ||
| try { | ||
| start = (el as HTMLInputElement | HTMLTextAreaElement).selectionStart; | ||
| end = (el as HTMLInputElement | HTMLTextAreaElement).selectionEnd; | ||
| if (start === end) return; // collapsed | ||
| } catch (error) { | ||
| // input that doesn't support selectionStart/selectionEnd (like checkbox) | ||
| // do nothing here | ||
| return; | ||
| } | ||
| return observer.next(); | ||
daniel-graham-amplitude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
daniel-graham-amplitude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // handle non-input elements | ||
| const selection = window.getSelection(); | ||
| if (!selection || selection.isCollapsed) return; | ||
| return observer.next(); | ||
| }; | ||
| window.document.addEventListener('selectionchange', handler); | ||
| return () => { | ||
| window.document.removeEventListener('selectionchange', handler); | ||
| }; | ||
| }), | ||
| ); | ||
|
|
||
| return { | ||
| [ObservablesEnum.ClickObservable]: clickObservable as Observable<ElementBasedTimestampedEvent<MouseEvent>>, | ||
| [ObservablesEnum.MutationObservable]: enrichedMutationObservable, | ||
| [ObservablesEnum.NavigateObservable]: enrichedNavigateObservable, | ||
| [ObservablesEnum.SelectionObservable]: selectionObservable, | ||
| }; | ||
| }; | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending rage‑click timeout isn’t cleared, so it can fire after teardown or a window reset and emit unexpectedly. Suggest always cancel and null any pending
rageClicktimer during teardown and whenever the click window is reset.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, this was here before. It just means that if it's unsubscribed and a rage click happens within the short window after unsubscribing, it will fire off one last event.