Treat selection hits on DOM-detached widgets as misses#6645
Open
uttam12331 wants to merge 1 commit into
Open
Conversation
The compositor can briefly return a widget that has been removed from the DOM (e.g. while Markdown.update swaps children). Starting or updating a text selection on such a widget crashed with AttributeError: 'NoneType' object has no attribute 'region' because the detached widget's parent is None. Guard both mouse selection paths in Screen._forward_event: if the hit widget is no longer attached, treat the hit as a miss. Fixes Textualize#6643
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.
Fixes #6643.
Problem
As diagnosed by @abs1u in the issue: during
Markdown.update(), an oldMarkdownParagraphcan be detached from the DOM while the compositor still returns it fromget_widget_and_offset_at(). AMouseDownduring that window crashes:because
container = content_widget.parentisNonefor the detached widget.Fix
Following the direction the reporter verified locally ("ignoring a hit when
widget.is_attachedis false prevents the crash while preserving normal text selection"): in both mouse selection paths ofScreen._forward_event(MouseDownstart-selection andMouseMoveupdate-selection), if the hit widget is no longer attached to the DOM, treat the hit as a miss.Verification
main: crashes atscreen.py:1914exactly as reported. With this change: the race window is still observed (detached at step: 3) and there is no crash.test_mouse_down_on_detached_widget_does_not_crash, a deterministic regression test (removes a widget, then simulates the compositor still mapping it). It fails onmainwith the reportedAttributeErrorand passes with this change.tests/test_selection.pysuite passes (15 passed);black --checkclean; CHANGELOG entry added.