Scrolling scroll view to bottom in BasePersonalIdFragment when view r…#3570
Scrolling scroll view to bottom in BasePersonalIdFragment when view r…#3570OrangeAndGreen wants to merge 1 commit intomasterfrom
Conversation
…esizes (to fix issue with scrolling when keyboard appears).
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis change refactors keyboard scroll handling in BasePersonalIdFragment from a window-insets-based approach to a global-layout-listener model. A private globalLayoutListener field is introduced to store a layout listener that triggers scrolling to the bottom when layout changes occur. The listener is registered during setup and unregistered during destruction. Related imports for window insets handling are removed, with no changes to public method signatures. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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)
app/src/org/commcare/fragments/personalId/BasePersonalIdFragment.kt (2)
58-59:child.bottomas scroll target relies on ScrollView clamping — considerfullScrollas a cleaner alternative.
child.bottomtypically equalschild.height(since the child's top is 0), so passing it tosmoothScrollTointentionally over-shoots and letsScrollViewclamp to its max scroll extent. This works but is semantically indirect. A slightly cleaner idiom is:♻️ Alternative using fullScroll
globalLayoutListener = ViewTreeObserver.OnGlobalLayoutListener { - val contentHeight = scrollView.getChildAt(0)?.bottom ?: 0 - scrollView.smoothScrollTo(0, contentHeight) + scrollView.post { scrollView.fullScroll(android.view.View.FOCUS_DOWN) } }
postdefers the scroll until after the current layout pass completes, which is a well-established idiom and avoids triggering the scroller mid-layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/fragments/personalId/BasePersonalIdFragment.kt` around lines 58 - 59, Replace the indirect scroll-to-bottom logic that uses scrollView.getChildAt(0)?.bottom and smoothScrollTo with a post-deferred fullScroll call: locate the scrollView usage in BasePersonalIdFragment.kt (the scrollView variable) and change the behavior to call scrollView.post and then scrollView.fullScroll(View.FOCUS_DOWN) so the scroll happens after layout and uses the cleaner fullScroll API rather than relying on child.bottom clamping.
56-71: Store theScrollViewreference at registration time to avoid silent listener leaks.
setupregisters the listener on a specificScrollView'sViewTreeObserver, butdestroyre-fetchesscrollView.viewTreeObserverfrom its own parameter. If a caller ever passes a differentScrollViewinstance (e.g., a stale binding reference vs. a recreated one),removeOnGlobalLayoutListenerruns on the wrong observer and the listener is never removed.The simplest fix is to store a weak or strong reference to the
ScrollView(or itsViewTreeObserver) at setup time and reuse it indestroyKeyboardScrollListener, eliminating the parameter entirely:♻️ Proposed refactor — remove the mismatch risk
+ private var registeredScrollView: ScrollView? = null private var globalLayoutListener: ViewTreeObserver.OnGlobalLayoutListener? = null protected fun setupKeyboardScrollListener(scrollView: ScrollView) { + registeredScrollView = scrollView globalLayoutListener = ViewTreeObserver.OnGlobalLayoutListener { val contentHeight = scrollView.getChildAt(0)?.bottom ?: 0 scrollView.smoothScrollTo(0, contentHeight) } scrollView.viewTreeObserver.addOnGlobalLayoutListener(globalLayoutListener) } - protected fun destroyKeyboardScrollListener(scrollView: ScrollView) { + protected fun destroyKeyboardScrollListener() { + val scrollView = registeredScrollView ?: return globalLayoutListener?.let { listener -> if (scrollView.viewTreeObserver.isAlive) { scrollView.viewTreeObserver.removeOnGlobalLayoutListener(listener) } } globalLayoutListener = null + registeredScrollView = null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/fragments/personalId/BasePersonalIdFragment.kt` around lines 56 - 71, The listener is registered on a specific ScrollView's ViewTreeObserver in setupKeyboardScrollListener but destroyKeyboardScrollListener re-fetches the observer from its ScrollView parameter, which can cause silent leaks if a different ScrollView is passed; fix by storing the ScrollView (or its viewTreeObserver) when you create globalLayoutListener in setupKeyboardScrollListener (e.g., a private var registeredScrollView or registeredViewTreeObserver) and update destroyKeyboardScrollListener to remove the listener from that stored reference and drop the ScrollView parameter, then clear both the stored reference and globalLayoutListener after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/org/commcare/fragments/personalId/BasePersonalIdFragment.kt`:
- Around line 58-59: Replace the indirect scroll-to-bottom logic that uses
scrollView.getChildAt(0)?.bottom and smoothScrollTo with a post-deferred
fullScroll call: locate the scrollView usage in BasePersonalIdFragment.kt (the
scrollView variable) and change the behavior to call scrollView.post and then
scrollView.fullScroll(View.FOCUS_DOWN) so the scroll happens after layout and
uses the cleaner fullScroll API rather than relying on child.bottom clamping.
- Around line 56-71: The listener is registered on a specific ScrollView's
ViewTreeObserver in setupKeyboardScrollListener but
destroyKeyboardScrollListener re-fetches the observer from its ScrollView
parameter, which can cause silent leaks if a different ScrollView is passed; fix
by storing the ScrollView (or its viewTreeObserver) when you create
globalLayoutListener in setupKeyboardScrollListener (e.g., a private var
registeredScrollView or registeredViewTreeObserver) and update
destroyKeyboardScrollListener to remove the listener from that stored reference
and drop the ScrollView parameter, then clear both the stored reference and
globalLayoutListener after removal.
| protected fun setupKeyboardScrollListener(scrollView: ScrollView) { | ||
| globalLayoutListener = ViewTreeObserver.OnGlobalLayoutListener { | ||
| val contentHeight = scrollView.getChildAt(0)?.bottom ?: 0 | ||
| scrollView.smoothScrollTo(0, contentHeight) |
There was a problem hiding this comment.
As I was the one who was able to reproduce the issue, I have given it a try and found out the following is also working: (Not using ViewTreeObserver as it's discouraged)
protected fun setupKeyboardScrollListener(scrollView: ScrollView) {
ViewCompat.setOnApplyWindowInsetsListener(scrollView) { _, insets ->
if (insets.isVisible(WindowInsetsCompat.Type.ime())) {
scrollView.post {
scrollView.smoothScrollTo(0, scrollView.getChildAt(0)?.bottom ?: 0)
}
}
insets
}
}
protected fun destroyKeyboardScrollListener(scrollView: ScrollView) {
ViewCompat.setOnApplyWindowInsetsListener(scrollView, null)
}
Your solution is also working.
But just to let you know that both the solutions will work for only one EditText, as getChildAt(0)?.bottom equals the total height of the scroll content. So if it's used with multiple EditTexts, it might start giving other issues, but yeah, it works for this case.
There was a problem hiding this comment.
I tested the code you pasted but it didn't consistently solve the task:
- On my tablet (where I was getting the squished scroll view reported in the ticket), the scroll view no longer gets squished. The screen is large enough to show the entire UI as well as the keyboard, so scrolling isn't possible (and shouldn't be).
- But on my phone the screen is small enough that the keyboard covers the Continue button, and the UI does not scroll down to show the Continue button with the suggested code.
The current solution works correctly on both my tablet and my phone.
| protected fun setupKeyboardScrollListener(scrollView: ScrollView) { | ||
| globalLayoutListener = ViewTreeObserver.OnGlobalLayoutListener { | ||
| val contentHeight = scrollView.getChildAt(0)?.bottom ?: 0 | ||
| scrollView.smoothScrollTo(0, contentHeight) |
There was a problem hiding this comment.
Do we really need scroll view in this layout, I am concerned here the solution here is quite risky and hardcoded without us realising the impact this can cause -
-
Seems like we are adding a global layout listener which will get triggered on all sorts of layout changes and not just keyboard open/close and may lead to the unwated behaviour here.
-
scrollView.getChildAt(0)?.bottomseems to assume a speciifc child view positioning and is not resilient with xml layout changes
Think if this is becoming hard to manage, I would advocate use of the deprecated Android methods here instead to adjust the layout and make a separate effort to get rid of Scroll view from this layout (assuming there is not a good reason for it to be there)
There was a problem hiding this comment.
I think the ScrollViews on every page of the PersonalID configuration workflow are necessary since the UIs are often large enough that the Continue button gets hidden behind the keyboard. Otherwise users must know to dismiss the keyboard in order to find the button. These pages didn't originally employ ScrollViews, but we added them because users were getting stuck. I think removing them safely would be a large task.
I agree with both of your points about how this could be dangerous though, and also Jignesh's point that ViewTreeObserver is no longer recommended. Thoughts:
- We could add some code to restrict the scrolling to only when they keyboard appears and ignore other callbacks
- In practice all of the pages in the configuration workflow have the Continue button as the last element in the UI, and as that's always a pretty important UI element it currently works out that always scrolling to it works well.
- As a workaround, we could have each fragment do something to specify the button explicitly so we always scroll to that rather than assuming we need to scroll to the last element
If we aren't happy with any of the solutions so far, I'd suggest just leaving it as is might be better than taking on big changes like trying to remove all the ScrollViews right now. Although I'll point out that the behavior today is for the button to get hidden behind the keyboard and the user must either scroll it into view manually or dismiss the keyboard. The code proposed here works for me on every device I've tested and makes the Continue button visible while the keyboard is up. That seems like a noticeable improvement to the UX.
There was a problem hiding this comment.
@OrangeAndGreen makes sense, think if we can only do the scrolling in response to keyboard opening/closing that will elevate the large part of "unknown" safety concerns on this PR for me.
I think the ScrollViews on every page of the PersonalID configuration workflow are necessary since the UIs are often large enough that the Continue button gets hidden behind the keyboard. Otherwise users must know to dismiss the keyboard in order to find the button.
I was imagining that with this change to adjust the layout, we might not need scrollview anymore but since the solution here depends on scrolling itself, think my assimption there might not be correct.
On 2 and 3 -> it makes sense to me to scroll to the bottom with the info you added.
There was a problem hiding this comment.
Sounds good, I'll look into that
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
LGTM if we keep using the scroll view
4 participants
https://dimagi.atlassian.net/browse/CCCT-2197
Product Description
Fixes a bug with scrolling on in PersonalID configuration
On some devices:
Note CommCare logo unnecessarily scrolled away in the "before" shot below
Before (showing the error):
After (fixed):
Technical Summary
Scrolling scroll view to bottom in BasePersonalIdFragment when view resizes.
Not taking over edge-to-edge and trying to manually handle IME offsets reliably.
The simpler solution to this issue turned out to just be letting the ScrollView resize naturally when the keyboard appears, and then scroll to the bottom to ensure the last element (always the Continue button) is showing.
Feature Flag
None
Safety Assurance
Safety story
I was eventually able to reproduce this on a Google Pixel Tablet
See screenshots above showing before and after views of the fix.
I also verified that the scrolling works as desired on my phone when the keyboard appears and disappears.
Automated test coverage
None
QA Plan
Repeat steps described in the Safety Story above.