Skip to content

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Jan 30, 2024

fixes DropdownMenu doesn't have a focusNode

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

enum TShirtSize {
  s('S'),
  m('M'),
  l('L'),
  xl('XL'),
  xxl('XXL'),
  ;

  const TShirtSize(this.label);
  final String label;
}

void main() => runApp(const MyApp());

class MyApp extends StatefulWidget {
  const MyApp({super.key});

  @override
  State<MyApp> createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  final FocusNode _focusNode = FocusNode();

  @override
  void dispose() {
    _focusNode.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Scaffold(
        appBar: AppBar(
          title: const Text('DropdownMenu Sample'),
        ),
        body: Center(
          child: DropdownMenu<TShirtSize>(
            focusNode: _focusNode,
            initialSelection: TShirtSize.m,
            label: const Text('T-Shirt Size'),
            dropdownMenuEntries: TShirtSize.values.map((e) {
              return DropdownMenuEntry<TShirtSize>(
                value: e,
                label: e.label,
              );
            }).toList(),
          ),
        ),
        floatingActionButton: FloatingActionButton.extended(
          onPressed: () {
            _focusNode.requestFocus();
          },
          label: const Text('Request Focus on DropdownMenu'),
        ),
      ),
    );
  }
}

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jan 30, 2024
@Hixie Hixie requested a review from TahaTesser January 30, 2024 09:46
@Hixie
Copy link
Contributor Author

Hixie commented Jan 30, 2024

Still needs a test, see discussion in #142384.

@TahaTesser TahaTesser changed the title DropDownMenu.focusNode Add DropdownMenu.focusNode Jan 30, 2024
Comment on lines 463 to 477
Copy link
Member

@TahaTesser TahaTesser Jan 30, 2024

Choose a reason for hiding this comment

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

canRequestFocus allows the text input field to be focused only on desktop unless DropdownMenu.requestFocusOnTap is set to true.

This can be handled two ways:

  1. A. focusable FocusNode overrides the requestFocusOnTap value. (I added this)
  2. Explicitly set requestFocusOnTap to true when providing a FocusNode.

cc: @QuncCccccc if you've any input here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this! I think the first option makes more sense:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should update the docs as well right? (To say which one wins.)

Copy link
Member

Choose a reason for hiding this comment

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

Yep!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed an update to the docs

Comment on lines +288 to +294
Copy link
Member

Choose a reason for hiding this comment

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

The analyzer is probably failing due to missing {@tool snippet}.

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe we can create a {@macro ...} for this documentation. Seems it is the same as the TextField.focusNode:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I think @macro is a bit of a mistake, because it makes it hard to read the documentation inline, and makes it very easy for us to write incorrect documentation (e.g. when we're updating docs in a template, it's easy to add changes that don't apply to all the places that use that template). I'm currently in the middle of rewriting a bunch of documentation around text fields and the magnifier in particular that are suboptimal because of over-eager use of macros.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. That makes sense:)

@TahaTesser
Copy link
Member

TahaTesser commented Jan 30, 2024

Added a fix for the widget test not producing expected results and added a test for the changes. b92e7c4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, of course, i forgot to advance the animation to see the changed rendering. d'oh.

Copy link
Member

Choose a reason for hiding this comment

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

Minor detail, easy to forget since a lot of tests simply use pumpAndSettle.

This and widget.focusNode!.canRequestFocus gets the expected test results for all target platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I regret ever creating pumpAndSettle. :-)

@Hixie
Copy link
Contributor Author

Hixie commented Jan 30, 2024

Thanks @TahaTesser!

@Hixie Hixie force-pushed the dropDownMenuFocusNode branch from b92e7c4 to aa6137d Compare January 30, 2024 22:33
@Hixie Hixie marked this pull request as ready for review January 30, 2024 22:33
@Hixie Hixie force-pushed the dropDownMenuFocusNode branch from aa6137d to a75d246 Compare January 30, 2024 22:39
@Hixie
Copy link
Contributor Author

Hixie commented Jan 30, 2024

@TahaTesser's changes LGTM, but someone will have to review my part of the changes before we can land it. :-)

@QuncCccccc
Copy link
Contributor

@TahaTesser's changes LGTM, but someone will have to review my part of the changes before we can land it. :-)

Taking a look!

Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM:) Thanks for adding this.

@Hixie
Copy link
Contributor Author

Hixie commented Jan 31, 2024

Thanks!

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2024
@auto-submit

This comment was marked as outdated.

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2024
@auto-submit auto-submit bot merged commit 16e014e into flutter:master Jan 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 1, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Feb 1, 2024
Manual roll requested by [email protected]

flutter/flutter@75a2e5b...c65ab4d

2024-01-31 [email protected] Roll Flutter Engine from 2d2d88068a23 to 8e7df85f7d11 (2 revisions) (flutter/flutter#142583)
2024-01-31 [email protected] Add `DropdownMenu.focusNode` (flutter/flutter#142516)
2024-01-31 [email protected] Marks Windows_android native_assets_android to be unflaky (flutter/flutter#140042)
2024-01-31 [email protected] Marks Linux_pixel_7pro complex_layout_scroll_perf_impeller_gles__timeline_summary to be unflaky (flutter/flutter#140039)
2024-01-31 [email protected] Marks Linux_android native_assets_android to be unflaky (flutter/flutter#140040)
2024-01-31 [email protected] Marks Mac_android native_assets_android to be unflaky (flutter/flutter#140041)
2024-01-31 [email protected] Marks Mac flavors_test_macos to be unflaky (flutter/flutter#140647)
2024-01-31 [email protected] Roll Flutter Engine from 588f06b02664 to 2d2d88068a23 (1 revision) (flutter/flutter#142569)
2024-01-31 [email protected] Roll Flutter Engine from e6e1d6b3d035 to 588f06b02664 (1 revision) (flutter/flutter#142566)
2024-01-31 [email protected] M3 - Fix Chip icon and label colors (flutter/flutter#140573)
2024-01-31 [email protected] Roll Flutter Engine from d20ed240ace9 to e6e1d6b3d035 (3 revisions) (flutter/flutter#142564)
2024-01-30 [email protected] Be less verbose in CI for customer_testing, and revert old timeout hack (flutter/flutter#139611)
2024-01-30 [email protected] use PDI to end the isolated scope for RLI (flutter/flutter#141345)
2024-01-30 [email protected] Style correctness improvements for toStrings and related fixes (flutter/flutter#142485)
2024-01-30 [email protected] Roll Flutter Engine from 500ae6c4d947 to d20ed240ace9 (1 revision) (flutter/flutter#142554)
2024-01-30 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 3.1.5 to 3.1.6 (flutter/flutter#142553)
2024-01-30 [email protected] Roll Flutter Engine from cd12316a0883 to 500ae6c4d947 (1 revision) (flutter/flutter#142551)
2024-01-30 [email protected] Roll Flutter Engine from 0e4342c5a1f2 to cd12316a0883 (6 revisions) (flutter/flutter#142548)
2024-01-30 [email protected] Roll Flutter Engine from 438e9b4d7d4e to 0e4342c5a1f2 (11 revisions) (flutter/flutter#142543)
2024-01-30 [email protected] Fix: selection handles do not inherit color from local `Theme` widget (flutter/flutter#142476)
2024-01-30 [email protected] Marks Windows_arm64 plugin_test_windows to be unflaky (flutter/flutter#137675)
2024-01-30 [email protected] Revert "Roll Flutter Engine from 0e586d1c28c8 to f02a4a80a77e (3 revisions)" (flutter/flutter#142533)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DropdownMenu doesn't have a focusNode

3 participants