fix(forms): floating form offset clamping + data-klaviyo-device device info#443
Draft
evan-masseau wants to merge 8 commits intorel/4.4.0from
Draft
fix(forms): floating form offset clamping + data-klaviyo-device device info#443evan-masseau wants to merge 8 commits intorel/4.4.0from
evan-masseau wants to merge 8 commits intorel/4.4.0from
Conversation
Bring Android floating-form offset handling in line with iOS. Offsets (plus safe-area insets) are treated as the margin from the screen edge and take priority over the requested form size: if form dimensions plus margins would exceed the screen, the form now shrinks to fit. If it already fits, margins anchor it away from the edge without shrinking. - Introduce calculateLayoutParams() that folds safe-area + user offsets into available width AND height, clamps requested dims via min(), and returns gravity-relative x/y consistent with Android's WindowManager model. - Respect left/right offsets for horizontally-centered positions (TOP, BOTTOM, CENTER) and top/bottom offsets for CENTER by shifting toward the smaller margin (matches iOS calculateFrame semantics). - FULLSCREEN continues to ignore offsets and fill the screen. - Update calculateFormBottomGap() to account for the asymmetric-margin shift on CENTER and for the clamped form height, keeping the keyboard-shift overlap math correct. - Remove the now-dead FormPosition.isHorizontallyCentered() helper. - Expand FloatingFormWindowTest coverage for clamping, asymmetric centering, safe-area + offset interaction, and the FULLSCREEN short-circuit.
Forms taller than the available screen area (or CENTER with heavily asymmetric margins) could produce a negative calculateFormBottomGap value, which in turn over-shifted the flyout when the keyboard opened. Floor the gap at zero so keyboard-shift math stays well-defined.
Adds a `data-klaviyo-device` attribute on the in-app forms template head that publishes screen dimensions, safe-area insets, orientation, and device pixel ratio using CSSOM conventions. This lets onsite-in-app compute flexible-form dimensions at HTML parse time without querying potentially-stale `window.screen.*` before view-hierarchy attachment. The attribute is re-published whenever insets change (via the existing `setOnApplyWindowInsetsListener`) and on orientation changes (via the presentation manager's `ConfigurationChanged` handler), so onsite stays in sync with the device state.
…sets flag Rename the formWillAppear wire key `margin` to `offsets` to match the Android internal naming. Read `offsets` first and fall back to `margin` for backward compatibility with older onsite payloads, emitting a one-time verbose deprecation log the first time the fallback is hit. Add a new optional `addSafeAreaInsetsToOffsets` flag (default true) that lets onsite opt out of the SDK's safe-area handling. When false, the SDK treats offsets as absolute distances from the screen edge and skips safe area entirely for both position math and available-space clamping, so onsite is fully responsible for any safe-area inset it wants to apply. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 178ebb6. Configure here.
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 178ebb6. Configure here.
…rotation lag ConfigurationChanged fires before the old activity is destroyed, so Display.rotation/rootWindowInsets still reflect the prior orientation when read from Registry.lifecycleMonitor.currentActivity at that moment. Pushing device info then left `data-klaviyo-device` one rotation behind on every subsequent rotation. Replace the immediate pushDeviceInfo() call in onConfigurationChanged with a one-shot ActivityObserver that waits for the next Resumed event (when the new activity's Display metrics are fresh), with a safety timeout to unregister if the new activity never resumes. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Rotation-to-Resumed is typically sub-300ms. 5s was overspecified and meant stale attribute data could linger for 5s in the pathological destroyed- without-replacement path. 1s comfortably exceeds real recreation time. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…ally BugBot flagged that clearTimers() does not reset deviceInfoPushObserver / deviceInfoPushTimeout. That is deliberate — the data-klaviyo-device attribute should stay fresh on the preloaded webview whether or not a form is presenting. dismiss() does not destroy the webview, and pushDeviceInfo already guards against a null webview. Also remove the diagnostic console-log snippet from the HTML template. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit cb57acc. Configure here.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Floating-form rendering tweaks: clamp offsets so they can't push the form off-screen, expose a
data-klaviyo-deviceattribute on<head>for onsite JS, harden the device-info push path for thread-safety, and rename themarginlayout key tooffsetsplus a new opt-out flag for SDK-managed safe-area insets.Due Diligence
Release/Versioning Considerations
PatchContains internal changes or backwards-compatible bug fixes.MinorContains changes to the public API.MajorContains breaking changes.Changelog / Code Overview
Four closely-related areas of work on floating-form rendering:
Offset clamping (
65152a74a,3cefafc0a)offsetscan never drive the computed size negative.>= 0so we don't spuriously report overlap when the form sits above the keyboard.Device info attribute (
54472e68b)data-klaviyo-deviceattribute on<head>carrying JSON-encoded device info (DPR, viewport, orientation, safe-area insets) so onsite JS can read it directly.Device-info push hardening (
fb3a0bf96)pushDeviceInfonow dispatches the entire snapshot/serialize/escape/evaluate chain onto the UI thread.DeviceInfoProvider.current()reads UI-thread-only APIs (Display.rotation,decorView.rootWindowInsets) and silently degrades off-thread.toJsondeterminism.Wire rename + safe-area opt-out (
178ebb6f5)margin→offsetsto match Android internal naming.offsetsfirst; fall back tomarginfor backward compatibility with older onsite payloads. Emits a one-time verbose deprecation log the first time the fallback is hit.addSafeAreaInsetsToOffsetsflag (defaulttrue) lets onsite opt out of SDK-managed safe-area insets entirely. Whenfalse, the SDK treatsoffsetsas absolute distances from the screen edge and skips safe-area in both position math and available-space clamping. Keyboard-overlap math mirrors the same semantic.Wire-contract changes for Fender
margin→offsets(with backward-compat fallback and one-time deprecation log)addSafeAreaInsetsToOffsets: Booleanoptional, defaults totrue(preserves existing behavior)Test Plan
./gradlew :sdk:forms:testDebugUnitTestpasses (new coverage inFormLayoutTest,FloatingFormWindowTest,DeviceInfoTest)./gradlew ktlintCheckcleanoffsetsclamps to screen bounds instead of overflowingdata-klaviyo-deviceattribute appears on<head>and updates on rotationaddSafeAreaInsetsToOffsets: falsepayload from Fender positions the form at absolute edge-relative offsets (no safe-area padding added)marginpayload still renders correctly and emits the deprecation log once per sessionRelated Issues/Tickets
Related to MAGE-541