Fix web PaymentElement scrolling in constrained layouts#2423
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR blocks ChangesWeb Platform Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/stripe_web/lib/src/widgets/payment_element.dart (2)
119-121: ⚡ Quick winRemove redundant style assignments.
Lines 119-120 manually set
heightandminHeight, but line 121 immediately calls_applyHostHeight()which sets these same properties (along withmaxHeightand overflow). The manual assignments are redundant.♻️ Simplify by removing redundant assignments
_divElement = web.HTMLDivElement() ..id = 'payment-element' ..style.border = 'none' - ..style.width = '100%' - ..style.height = '${_effectiveHeight}px' - ..style.minHeight = '${_effectiveHeight}px'; + ..style.width = '100%'; _applyHostHeight();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stripe_web/lib/src/widgets/payment_element.dart` around lines 119 - 121, Remove the redundant manual style assignments of height and minHeight that use _effectiveHeight and rely on the existing _applyHostHeight() call to set height, minHeight, maxHeight and overflow; specifically, in the PaymentElement widget remove the two lines that set ..style.height = '${_effectiveHeight}px' and ..style.minHeight = '${_effectiveHeight}px' so that only _applyHostHeight() configures these properties (keep _effectiveHeight and _applyHostHeight() as-is).
185-195: ⚡ Quick winConsider allowing horizontal overflow when capped.
When the element is height-capped, line 193 sets
overflowX = 'hidden', which prevents horizontal scrolling. If the PaymentElement contains wide form fields, labels, or validation messages that exceed the container width, they would be clipped without any way to view them.Consider using
overflowX = 'auto'to enable horizontal scrolling when needed, or omit setting it entirely to inherit the default behavior.📜 Proposed change
_divElement.style ..height = heightCss ..minHeight = heightCss ..maxHeight = _isHeightCapped ? heightCss : '' - ..overflowX = _isHeightCapped ? 'hidden' : '' + ..overflowX = '' ..overflowY = _isHeightCapped ? 'auto' : '';Or if you want to enable horizontal scrolling when needed:
_divElement.style ..height = heightCss ..minHeight = heightCss ..maxHeight = _isHeightCapped ? heightCss : '' - ..overflowX = _isHeightCapped ? 'hidden' : '' + ..overflowX = _isHeightCapped ? 'auto' : '' ..overflowY = _isHeightCapped ? 'auto' : '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stripe_web/lib/src/widgets/payment_element.dart` around lines 185 - 195, In _applyHostHeight(), when _isHeightCapped is true the code sets _divElement.style.overflowX = 'hidden' which can clip wide content; change that assignment to 'auto' (or remove the overflowX assignment entirely) so horizontal scrolling is allowed when needed; update the branch that references _isHeightCapped in _applyHostHeight to use overflowX = 'auto' instead of 'hidden' to ensure wide form fields/labels remain accessible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/stripe_web/lib/src/widgets/payment_element.dart`:
- Around line 119-121: Remove the redundant manual style assignments of height
and minHeight that use _effectiveHeight and rely on the existing
_applyHostHeight() call to set height, minHeight, maxHeight and overflow;
specifically, in the PaymentElement widget remove the two lines that set
..style.height = '${_effectiveHeight}px' and ..style.minHeight =
'${_effectiveHeight}px' so that only _applyHostHeight() configures these
properties (keep _effectiveHeight and _applyHostHeight() as-is).
- Around line 185-195: In _applyHostHeight(), when _isHeightCapped is true the
code sets _divElement.style.overflowX = 'hidden' which can clip wide content;
change that assignment to 'auto' (or remove the overflowX assignment entirely)
so horizontal scrolling is allowed when needed; update the branch that
references _isHeightCapped in _applyHostHeight to use overflowX = 'auto' instead
of 'hidden' to ensure wide form fields/labels remain accessible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4418c17-56cb-4370-bc44-01651db9d839
📒 Files selected for processing (2)
packages/stripe_web/lib/src/web_stripe.dartpackages/stripe_web/lib/src/widgets/payment_element.dart
remonh87
left a comment
remonh87
left a comment
There was a problem hiding this comment.
Thanks for the PR I left a couple of remarks here
| // layout height instead of Stripe's natural content height. Keep the | ||
| // previous natural measurement unless Stripe reports a larger height, | ||
| // otherwise the element can repeatedly cap and uncap while resizing. | ||
| if (!_isHeightCapped || |
There was a problem hiding this comment.
Once the content grows past maxHeight one time, this if only ever lets _measuredContentHeight go up never back down. So if the user switches to a shorter payment method (or some validation error happens) the element stays stuck at maxheight with a scrollbar even though the content would now fit. Best fix: measure stripeEl.scrollHeight instead of the contentRect.height. If you do this this guard can be removed entirely
| _divElement.style.height = '${height}px'; | ||
| }); | ||
| final observedHeight = entry.contentRect.height.toDouble(); | ||
| // Once the host is capped, ResizeObserver may report the capped |
There was a problem hiding this comment.
Also the guard below does not claim what the comment says. I would implement below suggestion and then we can get rid of it
| ..style.width = '100%' | ||
| ..style.height = '${height}px' | ||
| ..style.minHeight = '${height}px'; | ||
| ..style.height = '${_effectiveHeight}px' |
There was a problem hiding this comment.
I think these assignments are not needed since we override them in _applyHostHeight
| ..height = heightCss | ||
| ..minHeight = heightCss | ||
| ..maxHeight = _isHeightCapped ? heightCss : '' | ||
| ..overflowX = _isHeightCapped ? 'hidden' : '' |
There was a problem hiding this comment.
I am not sure about this oberflow we need to double check that some focus rings or dropdowns from the stripe element do not clip.
2 participants
Fixes
PaymentElementon web when it is placed inside a constrained layout. Fix #2045Add an optional
maxHeight. If Stripe reports content taller than that, the host div is capped and scrolls instead of growing past its parent.Also adds the missing web stub for
setConfirmHandlerto make the package compile.I tested the changes on our project, it is running on the old Flutter version (3.32.8), haven't verify on stable Flutter yet.
AI was used to help inspect the existing web implementation and draft the change. I manually reviewed the changes, tested the patched package in a Flutter web app using a constrained payment sheet, and ran package-level analysis/tests locally.
Summary by CodeRabbit
New Features
maxHeightparameter.Improvements
Compatibility