Fix DropdownButton regression (#6353)

When I changed how routes complete their futures, I broke the Dropdown
button because it was still waiting for its own Completer to complete
instead of using the Future returned by push. This patch fixes that
issue.

I've also removed the previous behavior of the DropdownButton forwarding
its text style to its route. The mechansim that we were using doesn't
work properly in all cases. For example, if the DropdownButton is a
child of a LayoutBuilder, then the route will have already built by the
time the DropdownButton gets a chance to forward its text style, causing
an assert in setState.

Finally, I've tweaked PopupMenuButton to work the same way as
DropdownButton in a couple corner cases (e.g., not calling the changed
callback if the button was removed from the tree before the menu
completed its Future).

Fixes #6352

packages/flutter/lib/src/material/drop_down.dart

@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'dart:async';
 import 'dart:math' as math;
 
 import 'package:flutter/scheduler.dart';
@@ -293,8 +292,8 @@ class _DropdownRoute<T> extends PopupRoute<_DropdownRouteResult<T>> {
     this.selectedIndex,
     this.elevation: 8,
     this.theme,
-    TextStyle style,
-  }) : _style = style {
+    this.style,
+  }) {
     assert(style != null);
   }
 
@@ -304,21 +303,12 @@ class _DropdownRoute<T> extends PopupRoute<_DropdownRouteResult<T>> {
   final int selectedIndex;
   final int elevation;
   final ThemeData theme;
+  final TextStyle style;
+
   // The layout gets this route's scrollableKey so that it can scroll the
   /// selected item into position, but only on the initial layout.
   bool initialLayout = true;
 
-  TextStyle get style => _style;
-  TextStyle _style;
-  set style (TextStyle value) {
-    assert(value != null);
-    if (_style == value)
-      return;
-    setState(() {
-      _style = value;
-    });
-  }
-
   @override
   Duration get transitionDuration => _kDropdownMenuDuration;
 
@@ -493,24 +483,17 @@ class _DropdownButtonState<T> extends State<DropdownButton<T>> {
 
   TextStyle get _textStyle => config.style ?? Theme.of(context).textTheme.subhead;
 
-  _DropdownRoute<T> _currentRoute;
-
   void _handleTap() {
-    assert(_currentRoute == null);
     final RenderBox itemBox = context.findRenderObject();
     final Rect itemRect = itemBox.localToGlobal(Point.origin) & itemBox.size;
-    final Completer<_DropdownRouteResult<T>> completer = new Completer<_DropdownRouteResult<T>>();
-    _currentRoute = new _DropdownRoute<T>(
+    Navigator.push(context, new _DropdownRoute<T>(
       items: config.items,
       buttonRect: _kMenuHorizontalPadding.inflateRect(itemRect),
       selectedIndex: _selectedIndex,
       elevation: config.elevation,
       theme: Theme.of(context, shadowThemeOnly: true),
       style: _textStyle,
-    );
-    Navigator.push(context, _currentRoute);
-    completer.future.then((_DropdownRouteResult<T> newValue) {
-      _currentRoute = null;
+    )).then((_DropdownRouteResult<T> newValue) {
       if (!mounted || newValue == null)
         return;
       if (config.onChanged != null)
@@ -521,10 +504,8 @@ class _DropdownButtonState<T> extends State<DropdownButton<T>> {
   @override
   Widget build(BuildContext context) {
     assert(debugCheckHasMaterial(context));
-    final TextStyle style = _textStyle;
-    _currentRoute?.style = style;
     Widget result = new DefaultTextStyle(
-      style: style,
+      style: _textStyle,
       child: new Row(
         mainAxisAlignment: MainAxisAlignment.spaceBetween,
         mainAxisSize: MainAxisSize.min,

packages/flutter/lib/src/material/popup_menu.dart

@@ -508,7 +508,7 @@ class PopupMenuButton<T> extends StatefulWidget {
 }
 
 class _PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
-  void showButtonMenu(BuildContext context) {
+  void showButtonMenu() {
     final RenderBox renderBox = context.findRenderObject();
     final Point topLeft = renderBox.localToGlobal(Point.origin);
     showMenu/*<T>*/(
@@ -521,9 +521,11 @@ class _PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
         0.0, 0.0
       )
     )
-    .then((T value) {
-      if (value != null && config.onSelected != null)
-        config.onSelected(value);
+    .then((T newValue) {
+      if (!mounted || newValue == null)
+        return;
+      if (config.onSelected != null)
+        config.onSelected(newValue);
     });
   }
 
@@ -534,12 +536,12 @@ class _PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
         icon: new Icon(Icons.more_vert),
         padding: config.padding,
         tooltip: config.tooltip,
-        onPressed: () { showButtonMenu(context); }
+        onPressed: showButtonMenu,
       );
     }
     return new InkWell(
-      onTap: () { showButtonMenu(context); },
-      child: config.child
+      onTap: showButtonMenu,
+      child: config.child,
     );
   }
 }

packages/flutter/test/material/drop_down_test.dart

@@ -6,6 +6,64 @@ import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/material.dart';
 
 void main() {
+  testWidgets('Drop down button control test', (WidgetTester tester) async {
+    List<String> items = <String>['one', 'two', 'three', 'four'];
+    String value = items.first;
+
+    void didChangeValue(String newValue) {
+      value = newValue;
+    }
+
+    Widget build() {
+      return new MaterialApp(
+        home: new Material(
+          child: new Center(
+            child: new DropdownButton<String>(
+              value: value,
+              items: items.map((String item) {
+                return new DropdownMenuItem<String>(
+                  value: item,
+                  child: new Text(item),
+                );
+              }).toList(),
+              onChanged: didChangeValue,
+            ),
+          ),
+        ),
+      );
+    }
+
+    await tester.pumpWidget(build());
+
+    await tester.tap(find.text('one'));
+    await tester.pump();
+    await tester.pump(const Duration(seconds: 1)); // finish the menu animation
+
+    expect(value, equals('one'));
+
+    await tester.tap(find.text('three').last);
+
+    await tester.pump();
+    await tester.pump(const Duration(seconds: 1)); // finish the menu animation
+
+    expect(value, equals('three'));
+
+    await tester.tap(find.text('three'));
+    await tester.pump();
+    await tester.pump(const Duration(seconds: 1)); // finish the menu animation
+
+    expect(value, equals('three'));
+
+    await tester.pumpWidget(build());
+
+    await tester.tap(find.text('two').last);
+
+    await tester.pump();
+    await tester.pump(const Duration(seconds: 1)); // finish the menu animation
+
+    expect(value, equals('two'));
+  });
+
   testWidgets('Drop down screen edges', (WidgetTester tester) async {
     int value = 4;
     List<DropdownMenuItem<int>> items = <DropdownMenuItem<int>>[];
@@ -19,7 +77,7 @@ void main() {
     DropdownButton<int> button = new DropdownButton<int>(
       value: value,
       onChanged: handleChanged,
-      items: items
+      items: items,
     );
 
     await tester.pumpWidget(
@@ -27,10 +85,10 @@ void main() {
         home: new Material(
           child: new Align(
             alignment: FractionalOffset.topCenter,
-            child: button
-          )
-        )
-      )
+            child: button,
+          ),
+        ),
+      ),
     );
 
     await tester.tap(find.text('4'));
@@ -48,13 +106,13 @@ void main() {
     expect(value, 4);
     await tester.tap(find.byConfig(button));
     expect(value, 4);
-    await tester.idle(); // this waits for the route's completer to complete, which calls handleChanged
+    // this waits for the route's completer to complete, which calls handleChanged
+    await tester.idle();
     expect(value, 4);
 
     // TODO(abarth): Remove these calls to pump once navigator cleans up its
     // pop transitions.
     await tester.pump();
     await tester.pump(const Duration(seconds: 1)); // finish the menu animation
-
   });
 }