Fix #5067: InteractionDialog dispose leaves dialog visible without animateShow#5068
Merged
Merged
Conversation
…imateShow dispose() called remove() then pp.revalidate() on the layered pane wrapper to schedule the form repaint that hides the old dialog pixels. But remove() triggers the recursive deinitialize() path which itself runs cleanupLayer() and detaches that wrapper from the form first -- so by the time pp.revalidate() runs, pp has no Form in its parent chain and the repaint never bubbles up. setAnimateShow(true) masked the bug because the animation drives its own paint cycle. Without it, the old dialog pixels stayed on screen until something else (scroll, hover) forced a redraw. Force a form-level revalidate after cleanupLayer so the next paint cycle clears the pixels deterministically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 115 screenshots: 115 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 116 screenshots: 116 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 115 screenshots: 115 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
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
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.
Summary
InteractionDialog.dispose()left the dialog visible on screen whensetAnimateShow(false)+setFormMode(true). Scrolling or hovering would make it disappear, but the dialog otherwise stayed painted until the next external repaint trigger.dispose()so the next paint cycle reliably clears the old dialog pixels.disposeWithoutAnimationSchedulesFormRepaint) inInteractionDialogTest.Root cause
dispose()ends with:The
remove()invokesComponent.deinitializeImpl()→InteractionDialog.deinitialize(), which already runs its owncleanupLayer(f)and detaches the layered-pane wrapper (pp) from the form. By the time the outerpp.revalidate()runs,pp.getComponentForm()returnsnull, sorevalidateInternalfalls throughContainer.java:1606and never callsForm.repaint().setAnimateShow(true)masked the bug becauseanimateUnlayoutAndWait(...)drives its own paint cycle.Fix
After
cleanupLayer(f), callf.revalidateWithAnimationSafety(). This guarantees a form revalidate + repaint regardless of whether the layered-pane wrapper is still attached when the earlierpp.revalidate()runs.Regression test
The test instantiates a
RepaintCountingForm, shows it, opens the dialog with the exactanimateShow=false+formMode=trueflags from the bug report, resets the counter, callsdispose(), and assertsrepaint()was invoked at least once. The test fails on master (0 calls) and passes with this fix (1 call).Note: the test requires
form.show()(rather than justimplementation.setCurrentForm(form)) so the dialog actually initialises -- without initialisation, the recursivedeinitialize()path doesn't fire anddispose()hits a different (working) code path that masks the bug.Test plan
mvn -pl core-unittests -am test -Dtest=InteractionDialogTest -DunitTests— all 10 tests passmvn -pl core-unittests -am test -DunitTests) — 2634 tests passobserved 0 calls) and passes with the fix🤖 Generated with Claude Code