From a38665807527b1088516886a02b007bd27be26b2 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Thu, 10 Aug 2017 16:44:39 -0700 Subject: [PATCH 1/6] Fix text selection tests in Chrome, update test running logic --- test/over_react/util/dom_util_test.dart | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/test/over_react/util/dom_util_test.dart b/test/over_react/util/dom_util_test.dart index e5e35027c..4e2805c19 100644 --- a/test/over_react/util/dom_util_test.dart +++ b/test/over_react/util/dom_util_test.dart @@ -214,7 +214,7 @@ main() { // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360 test(type, () { sharedInputSetSelectionRangeTest(type); - }, testOn: 'js && !chrome'); + }, testOn: '!blink'); } else { test(type, () { sharedInputSetSelectionRangeTest(type); }); } @@ -301,23 +301,29 @@ main() { }); group('for an `` of type:', () { - void sharedInputGetSelectionStartTest(String type) { + void sharedInputGetSelectionStartTest(String type, {bool shouldReturnNull: false}) { renderedInstance = renderAttachedToDocument((Dom.input() ..defaultValue = testValue ..type = type )()); inputElement = findDomNode(renderedInstance); + setSelectionRange(inputElement, testValue.length, testValue.length); + var selectionStart = getSelectionStart(inputElement); - expect(selectionStart, testValue.length); + expect(selectionStart, shouldReturnNull ? isNull : testValue.length); } for (var type in inputTypesWithSelectionRangeSupport) { if (type == 'email' || type == 'number') { // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360 + test('$type, returning null in Chrome/Dartium/content-shell', () { + sharedInputGetSelectionStartTest(type, shouldReturnNull: true); + }, testOn: 'blink'); + test(type, () { - sharedInputGetSelectionStartTest(type); - }, testOn: 'js && !chrome'); + sharedInputGetSelectionStartTest(type, shouldReturnNull: false); + }, testOn: '!blink'); } else { test(type, () { sharedInputGetSelectionStartTest(type); }); } From 26e61a198487a7358aa4b51cf2aad54749e6eb0c Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Thu, 10 Aug 2017 16:58:41 -0700 Subject: [PATCH 2/6] Update get/setSelectionRange with Firefox logic UIP-2322 --- lib/src/util/dom_util.dart | 4 ++-- test/over_react/util/dom_util_test.dart | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/src/util/dom_util.dart b/lib/src/util/dom_util.dart index 340e60b58..d76f4a677 100644 --- a/lib/src/util/dom_util.dart +++ b/lib/src/util/dom_util.dart @@ -128,7 +128,7 @@ void setSelectionRange(/* TextInputElement | TextAreaElement */Element input, in if (input is TextAreaElement) { input.setSelectionRange(start, end, direction); } else if (input is InputElement && supportsSelectionRange(input)) { - if (browser.isChrome) { + if (browser.isChrome || browser.isFirefox) { final inputType = input.getAttribute('type'); if (inputType == 'email' || inputType == 'number') { @@ -161,7 +161,7 @@ int getSelectionStart(Element input) { } else if (input is TextInputElement && supportsSelectionRange(input)) { final inputType = input.getAttribute('type'); - if (browser.isChrome) { + if (browser.isChrome || browser.isFirefox) { if (inputType == 'email' || inputType == 'number') { return null; } diff --git a/test/over_react/util/dom_util_test.dart b/test/over_react/util/dom_util_test.dart index 4e2805c19..6514bfd58 100644 --- a/test/over_react/util/dom_util_test.dart +++ b/test/over_react/util/dom_util_test.dart @@ -214,7 +214,7 @@ main() { // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360 test(type, () { sharedInputSetSelectionRangeTest(type); - }, testOn: '!blink'); + }, testOn: '!(blink || firefox)'); } else { test(type, () { sharedInputSetSelectionRangeTest(type); }); } @@ -233,7 +233,7 @@ main() { }); // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360 - group('without throwing an error in Google Chrome when `props.type` is', () { + group('without throwing an error in Google Chrome (or Dartium/content_shell) or Firefox when `props.type` is', () { void verifyLackOfException() { expect(renderedInstance, isNotNull, reason: 'test setup sanity check'); expect(inputElement, isNotNull, reason: 'test setup sanity check'); @@ -266,7 +266,7 @@ main() { inputElement = findDomNode(renderedInstance); verifyLackOfException(); }); - }, testOn: 'chrome'); + }, testOn: 'blink || firefox'); }); }); @@ -317,13 +317,13 @@ main() { for (var type in inputTypesWithSelectionRangeSupport) { if (type == 'email' || type == 'number') { // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360 - test('$type, returning null in Chrome/Dartium/content-shell', () { + test('$type, returning null in Google Chrome (or Dartium/content_shell) or Firefox', () { sharedInputGetSelectionStartTest(type, shouldReturnNull: true); - }, testOn: 'blink'); + }, testOn: 'blink || firefox'); test(type, () { sharedInputGetSelectionStartTest(type, shouldReturnNull: false); - }, testOn: '!blink'); + }, testOn: '!(blink || firefox)'); } else { test(type, () { sharedInputGetSelectionStartTest(type); }); } @@ -343,7 +343,7 @@ main() { }); // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360 - group('by returning null and not throwing an error in Google Chrome when `props.type` is', () { + group('by returning null and not throwing an error in Google Chrome (or Dartium/content_shell) or Firefox when `props.type` is', () { void verifyReturnsNullWithoutException() { expect(renderedInstance, isNotNull, reason: 'test setup sanity check'); expect(inputElement, isNotNull, reason: 'test setup sanity check'); @@ -372,7 +372,7 @@ main() { inputElement = findDomNode(renderedInstance); verifyReturnsNullWithoutException(); }); - }, testOn: 'chrome'); + }, testOn: 'blink || firefox'); }); }); } From 1a607396a4a65293db7325254d122c40cb1f8f46 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Thu, 10 Aug 2017 17:04:31 -0700 Subject: [PATCH 3/6] Print warning when getDartComponent is used incorrectly --- lib/src/util/react_wrappers.dart | 37 +++++++++++++++++++ test/over_react/util/react_wrappers_test.dart | 25 +++++++++++++ 2 files changed, 62 insertions(+) diff --git a/lib/src/util/react_wrappers.dart b/lib/src/util/react_wrappers.dart index 8f29355fa..d2264b9e0 100644 --- a/lib/src/util/react_wrappers.dart +++ b/lib/src/util/react_wrappers.dart @@ -280,6 +280,43 @@ react.Component getDartComponent(/* [1] */ instance) { return null; } + // Split out into a separate function since the DDC doesn't support + // functions as assert params. + bool warn() { + if (isValidElement(instance)) { + // `print` instead of `ValidationUtil.warn` so that this message shows up + // in the test output when running `ddev test`. + print(unindent( + ''' + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + * WARNING: `getDartComponent` + * + * react-dart 4.0 will no longer support retrieving Dart components from + * `ReactElement` (pre-mounted VDOM instances) in order to prevent memory leaks. + * + * In react-dart 4.0, this call to `getDartComponent` will return `null`. + * + * Please switch over to using the mounted JS component instance. + * + * Example: + * // Before + * var vdom = Button()('Click me'); + * react_dom.render(vdom, mountNode); + * var dartInstance = getDartComponent(vdom); + * + * // Fixed + * var vdom = Button()('Click me'); + * var jsInstance = react_dom.render(vdom, mountNode); + * var dartInstance = getDartComponent(jsInstance); + * + * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * + ''' + )); + } + return true; + } + assert(warn()); + return _getInternal(instance)?.component; } diff --git a/test/over_react/util/react_wrappers_test.dart b/test/over_react/util/react_wrappers_test.dart index 553dcf57c..03e030a40 100644 --- a/test/over_react/util/react_wrappers_test.dart +++ b/test/over_react/util/react_wrappers_test.dart @@ -15,6 +15,7 @@ @JS() library react_wrappers_test; +import 'dart:async'; import 'dart:html'; import 'package:js/js.dart'; @@ -439,6 +440,30 @@ main() { var renderedInstance = render(Dom.div()); expect(getDartComponent(renderedInstance), isNull); }); + + group('', () { + final messageMatcher = contains('react-dart 4.0 will no longer support retrieving Dart components from'); + + test('warns when passed a ReactElement', () { + ReactElement instance = Wrapper()(); + expect(() => getDartComponent(instance), prints(messageMatcher)); + }, testOn: 'dart-vm'); + + test('does not when passed a ReactElement in JS', () { + ReactElement instance = Wrapper()(); + expect(() => getDartComponent(instance), isNot(prints(messageMatcher))); + }, testOn: 'js'); + + test('does not warn when passed a ReactComponent', () { + var renderedInstance = render(Wrapper()); + expect(() => getDartComponent(renderedInstance), isNot(prints(messageMatcher))); + }); + + test('does not warn when passed a DOM component', () { + var renderedInstance = render(Dom.div()); + expect(() => getDartComponent(renderedInstance), isNot(prints(messageMatcher))); + }); + }); }); group('getProps', () { From 473b04360d2347fc9f68f18c1bd22ca527fea436 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Fri, 11 Aug 2017 10:54:34 -0700 Subject: [PATCH 4/6] Skip JS-only test in the DDC --- test/over_react/util/react_wrappers_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/over_react/util/react_wrappers_test.dart b/test/over_react/util/react_wrappers_test.dart index 03e030a40..ad8b31599 100644 --- a/test/over_react/util/react_wrappers_test.dart +++ b/test/over_react/util/react_wrappers_test.dart @@ -452,7 +452,7 @@ main() { test('does not when passed a ReactElement in JS', () { ReactElement instance = Wrapper()(); expect(() => getDartComponent(instance), isNot(prints(messageMatcher))); - }, testOn: 'js'); + }, testOn: 'js', tags: 'no-ddc'); test('does not warn when passed a ReactComponent', () { var renderedInstance = render(Wrapper()); From 0be65090527b78fbbaba6fd21e6923a420517e7a Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Fri, 11 Aug 2017 14:04:03 -0700 Subject: [PATCH 5/6] Fix `ddev coverage` failures --- test/over_react/util/dom_util_test.dart | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/over_react/util/dom_util_test.dart b/test/over_react/util/dom_util_test.dart index 6514bfd58..184f93954 100644 --- a/test/over_react/util/dom_util_test.dart +++ b/test/over_react/util/dom_util_test.dart @@ -214,7 +214,11 @@ main() { // See: https://bugs.chromium.org/p/chromium/issues/detail?id=324360 test(type, () { sharedInputSetSelectionRangeTest(type); - }, testOn: '!(blink || firefox)'); + + // Tests run in `ddev coverage` don't respect tags and show up as the 'vm' platform + // so we can use this to disable certain browser tests during coverage. + // Workaround for https://github.com/Workiva/dart_dev/issues/200 + }, testOn: '!(blink || firefox || vm)'); } else { test(type, () { sharedInputSetSelectionRangeTest(type); }); } @@ -323,7 +327,11 @@ main() { test(type, () { sharedInputGetSelectionStartTest(type, shouldReturnNull: false); - }, testOn: '!(blink || firefox)'); + + // Tests run in `ddev coverage` don't respect tags and show up as the 'vm' platform + // so we can use this to disable certain browser tests during coverage. + // Workaround for https://github.com/Workiva/dart_dev/issues/200 + }, testOn: '!(blink || firefox || vm)'); } else { test(type, () { sharedInputGetSelectionStartTest(type); }); } From b299df57a70cf5d82f04fba5b6b559fcf9796229 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Fri, 11 Aug 2017 14:15:55 -0700 Subject: [PATCH 6/6] Fix misinformed DDC assert workaround See: https://github.com/dart-lang/sdk/issues/30426 --- lib/src/util/react_wrappers.dart | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/src/util/react_wrappers.dart b/lib/src/util/react_wrappers.dart index d2264b9e0..12dfc74cd 100644 --- a/lib/src/util/react_wrappers.dart +++ b/lib/src/util/react_wrappers.dart @@ -280,9 +280,7 @@ react.Component getDartComponent(/* [1] */ instance) { return null; } - // Split out into a separate function since the DDC doesn't support - // functions as assert params. - bool warn() { + assert(() { if (isValidElement(instance)) { // `print` instead of `ValidationUtil.warn` so that this message shows up // in the test output when running `ddev test`. @@ -314,8 +312,7 @@ react.Component getDartComponent(/* [1] */ instance) { )); } return true; - } - assert(warn()); + }); return _getInternal(instance)?.component; }