Skip to content

Preserve SelectableRegion scope across context menu rebuilds - #186553

Merged
auto-submit[bot] merged 4 commits into
flutter:masterfrom
mvincentong:fix-selectionarea-context-menu-rebuild
Aug 7, 2026
Merged

Preserve SelectableRegion scope across context menu rebuilds#186553
auto-submit[bot] merged 4 commits into
flutter:masterfrom
mvincentong:fix-selectionarea-context-menu-rebuild

Conversation

@mvincentong

@mvincentongmvincentong commented May 15, 2026

Copy link
Copy Markdown
Contributor

SelectableRegion conditionally wraps its selection subtree in PlatformSelectableRegionContextMenu on desktop web. When BrowserContextMenu.enabled changes, that wrapper can be removed while the existing root SelectionContainer is still registered, causing a replacement container to register before the old one unregisters.

This keeps the selection status scope keyed from SelectableRegionState, so the selection subtree is reparented across the optional wrapper change instead of recreated. The browser regression covers enable/disable transitions with the widgets-layer test harness and current selection controls.

Fixes#186459.

Tests:

  • ../../bin/flutter test --no-pub --platform chrome test/widgets/selectable_region_context_menu_test.dart
  • ./bin/flutter analyze --no-pub packages/flutter/lib/src/widgets/selectable_region.dart packages/flutter/test/widgets/selectable_region_context_menu_test.dart
  • ./bin/dart format --output=none --set-exit-if-changed packages/flutter/test/widgets/selectable_region_context_menu_test.dart
  • git diff --check

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@github-actionsgithub-actionsBot added the framework flutter/packages/flutter repository. See also f: labels. label May 15, 2026

@gemini-code-assistgemini-code-assistBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a GlobalKey to SelectableRegionState to maintain the SelectableRegionSelectionStatusScope subtree across rebuilds. It also adds a widget test to ensure SelectableRegion can rebuild on the web after disabling the browser context menu. Feedback indicates that the new private GlobalKey should include public-quality documentation and a debugLabel to align with the Flutter style guide.

Comment threadpackages/flutter/lib/src/widgets/selectable_region.dart Outdated
@mvincentong
mvincentongforce-pushed the fix-selectionarea-context-menu-rebuild branch from 01851b9 to 4007a07CompareMay 15, 2026 08:29
@mvincentong
mvincentong changed the base branch from main to masterMay 21, 2026 03:01
@PiinksPiinks added a: text input Entering text in a text field or keyboard related problems f: selection SelectableRegion, SelectionArea, SelectionContainer, Selectable, and related APIs labels May 26, 2026
@Renzo-Olivares
Renzo-Olivares self-requested a review May 28, 2026 20:35
Comment threadpackages/flutter/test/widgets/selectable_region_test.dart Outdated
Comment threadpackages/flutter/test/widgets/selectable_region_test.dart Outdated
Comment threadpackages/flutter/test/widgets/selectable_region_test.dart Outdated
@Renzo-OlivaresRenzo-Olivares added the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Jun 11, 2026
@github-actionsgithub-actionsBot removed a: text input Entering text in a text field or keyboard related problems waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds labels Jun 12, 2026
@Renzo-Olivares
Renzo-Olivares self-requested a review June 18, 2026 19:38
@LongCatIsLooong
LongCatIsLooong self-requested a review June 25, 2026 20:44
Comment threadpackages/flutter/test/widgets/selectable_region_context_menu_test.dart Outdated
Comment threadpackages/flutter/test/widgets/selectable_region_context_menu_test.dart Outdated
expect(tester.takeException(), isNull);
}

await updateContextMenu(BrowserContextMenu.disableContextMenu());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd add a pump between each of these, and end with a pumpAndSettle.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this, I think it would be easier to read if this just calls BrowserContextMenu.disable/enable() directly with a pump in between rather than using this helper method.

@Renzo-OlivaresRenzo-Olivares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few small comments.

@Renzo-OlivaresRenzo-Olivares added the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Jul 9, 2026
@Renzo-Olivares

Copy link
Copy Markdown
Contributor

Hi @mvincentong, friendly ping is this something you still want to work on?

@mvincentong

Copy link
Copy Markdown
ContributorAuthor

Yes—thanks for the ping. I pushed 96b9f77: the regression now uses TestWidgetsApp and testTextSelectionHandleControls, pumps each toggle with a final pumpAndSettle, and links #186459.

@github-actionsgithub-actionsBot removed the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Jul 19, 2026
@Renzo-OlivaresRenzo-Olivares added the CICD Run CI/CD label Jul 20, 2026
Renzo-Olivares
Renzo-Olivares previously approved these changes Jul 20, 2026

@Renzo-OlivaresRenzo-Olivares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo my one comment. This will need a secondary review before landing.

expect(tester.takeException(), isNull);
}

await updateContextMenu(BrowserContextMenu.disableContextMenu());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this, I think it would be easier to read if this just calls BrowserContextMenu.disable/enable() directly with a pump in between rather than using this helper method.

@Piinks

Copy link
Copy Markdown
Contributor

I think this will also need a rebase to resolve the ci failure.

@mvincentong
mvincentongforce-pushed the fix-selectionarea-context-menu-rebuild branch from 96b9f77 to 58ae119CompareAugust 1, 2026 12:21
@flutter-dashboardflutter-dashboardBot removed the CICD Run CI/CD label Aug 1, 2026
@Renzo-Olivares
Renzo-Olivaresforce-pushed the fix-selectionarea-context-menu-rebuild branch from a2e033a to 51ed117CompareAugust 6, 2026 19:03
@flutter-dashboardflutter-dashboardBot removed the CICD Run CI/CD label Aug 6, 2026
@Renzo-OlivaresRenzo-Olivares added the CICD Run CI/CD label Aug 6, 2026
@LongCatIsLooongLongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 7, 2026
@auto-submit
auto-submitBot added this pull request to the merge queueAug 7, 2026
Merged via the queue into flutter:master with commit 2a469b8Aug 7, 2026
19 checks passed
@flutter-dashboardflutter-dashboardBot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 7, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICDRun CI/CDf: selectionSelectableRegion, SelectionArea, SelectionContainer, Selectable, and related APIsframeworkflutter/packages/flutter repository. See also f: labels.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Web] Rebuilding SelectionArea after disabling context menu triggers assertion failure

5 participants

@mvincentong@Renzo-Olivares@Piinks@LongCatIsLooong@alpesh76