Bring over DOM helper implementation. #258

rwjblue · 2017-12-06T03:25:52Z

TODO:

Turbo87

@rwjblue are there any tests for the new helpers?

Turbo87 · 2017-12-06T09:03:13Z

addon-test-support/@ember/test-helpers/dom/-get-element.js

+
+  let rootElement = getContext().element;
+
+  return rootElement.querySelector(selectorOrElement);


should we typecheck if selectorOrElement is a string?

I thought about that, we can but I was unsure if we could do beat things by allowing Object#toString to be leveraged...

I do not feel strongly though...

My imagination fails here. I don't see many use cases where that would be useful 🤔

Turbo87 · 2017-12-06T09:04:28Z

addon-test-support/@ember/test-helpers/dom/-is-focusable.js

@@ -0,0 +1,14 @@
+const FOCUSABLE_TAGS = ['INPUT', 'BUTTON', 'LINK', 'SELECT', 'A', 'TEXTAREA'];


can a <link> actually be focused?

Turbo87 · 2017-12-06T09:06:42Z

addon-test-support/@ember/test-helpers/dom/blur.js

+  @return {RSVP.Promise}
+  @public
+*/
+export default function blur(selectorOrElement) {


does it make sense that blur() takes a selector? should it default to document.activeElement?

Seems reasonable...

we should check if that works properly with headless browsers and unfocused tabs though

Turbo87 · 2017-12-06T09:09:59Z

addon-test-support/@ember/test-helpers/dom/focus.js

+
+  let element = getElement(selectorOrElement);
+
+  if (isFocusable(element)) {


should this throw an error or warning if the element is not focusable?

Turbo87 · 2017-12-06T09:14:37Z

addon-test-support/@ember/test-helpers/dom/blur.js

+import settled from '../settled';
+import isFocusable from './-is-focusable';
+
+/*


doc comments should use /** instead of /*. this seems to be the case for the majority of the doc comments in this PR.

Good catch. Will update.

Turbo87 · 2017-12-06T09:15:03Z

addon-test-support/@ember/test-helpers/dom/trigger-key-event.js

+ * @param selector
+ * @param type
+ * @param keyCode
+ * @param modifiers


the parameters are missing type information

Turbo87 · 2017-12-06T09:16:01Z

addon-test-support/@ember/test-helpers/dom/trigger-key-event.js

+  return triggerEvent(
+    selectorOrElement,
+    type,
+    merge({ keyCode, which: keyCode, key: keyCode }, modifiers)


can we extract that into an options variable so that the triggerEvent() call stays single-line?

Yep, that would look much better indeed.

Turbo87 · 2017-12-06T09:18:36Z

addon-test-support/@ember/test-helpers/wait-until.js

+        reject(waitUntilTimedOut);
+      }
+    }
+    setTimeout(tick, 10);


should this call tick() directly?

I did think about that, but doing that would allow it to resolve sync if wrapped in a run-loop, and we never want to resolve sync.

Turbo87 · 2017-12-06T09:20:04Z

tests/unit/helpers/wait-until-test.js

+
+      assert.verifySteps(['before invocation', 'after invocation', 'after resolved']);
+    });
+    assert.step('after invocation');


Turbo87 · 2017-12-06T09:22:06Z

@cibernox can you have a look at this too?

rwjblue · 2017-12-06T11:49:55Z

One of the major missing things here are the unit and integration tests for the new helpers. I’m still working on porting those. At the moment, the only tests are the ones that we had previously (in setup rendering context and moduleForComponent) that needed events. This is obviously not enough coverage.

cibernox · 2017-12-06T23:07:26Z

I'll check this more thoroughly tomorrow, but looks good at first glance.

Turbo87 · 2017-12-07T21:58:07Z

addon-test-support/@ember/test-helpers/dom/blur.js

+export default function blur(selectorOrElement = document.activeElement) {
+  let element = getElement(selectorOrElement);
+  if (!element) {
+    throw new Error(`Element not found when calling \`blur('${selectorOrElement}')\`.`);


since nextTickPromise() is making sure that we are always using async shouldn't we also make sure to not throw any errors in sync code? seems like they should be rejections instead 🤔

also what if element doesn't exist anymore by the time _blur() is called?

Turbo87 · 2017-12-07T21:59:51Z

addon-test-support/@ember/test-helpers/dom/blur.js

+import isFocusable from './-is-focusable';
+import { nextTickPromise } from '../-utils';
+
+export function _blur(element) {


I think we should explicitly mark those functions as @private

Turbo87 · 2017-12-07T22:01:43Z

addon-test-support/@ember/test-helpers/dom/fire-event.js

+  element.dispatchEvent(event);
+}
+
+/*


Turbo87 · 2017-12-07T22:01:53Z

addon-test-support/@ember/test-helpers/dom/fire-event.js

+  return event;
+}
+
+/*


Turbo87 · 2017-12-07T22:02:31Z

addon-test-support/@ember/test-helpers/dom/fire-event.js

+/*
+  @method buildMouseEvent
+  @param {String} type
+  @param {Object} (optional) options


usejsdoc.org has some special syntax for declaring optional parameters

Turbo87 · 2017-12-07T22:05:37Z

addon-test-support/@ember/test-helpers/wait-until.js

+      } else if (time < timeout) {
+        // using `setTimeout` directly to allow fake timers
+        // to intercept
+        setTimeout(tick, 10);


we should capture the setTimeout reference so that Timecop et al can do their thing without disturbing us

Turbo87 · 2017-12-07T22:08:40Z

tests/unit/dom/blur-test.js

+    this.element = document.querySelector('#qunit-fixture');
+
+    // create the element and focus in preparation for blur testing
+    element = buildInstrumentedElement('input');


having this.element and element be two different things seems a little confusing 🤔

I'm largely just trying to leverage the normal system of "rooting the selectors" that would happen in an app. In those circumstances this.element (aka getContext().element) is the root element of the app / component test / etc.

I think a good compromise here might be to use an explicit context other than this which has the root element (#qunit-fixture) and to not call the local variable element here.

I'll work towards that...

Turbo87 · 2017-12-07T22:11:47Z

tests/unit/dom/blur-test.js

+
+    assert.verifySteps(['blur', 'focusout']);
+    assert.notEqual(document.activeElement, element, 'activeElement updated');
+  });


what about "bluring via selector without context set"?

Turbo87 · 2017-12-07T22:12:53Z

tests/unit/dom/blur-test.js

+  hooks.beforeEach(async function(assert) {
+    // used to simulate how `setupRenderingTest` (and soon `setupApplicationTest`)
+    // set context.element to the rootElement
+    this.element = document.querySelector('#qunit-fixture');


I'm wondering if we should use an explicit context object here instead of this 🤔

Turbo87 · 2017-12-07T22:15:26Z

tests/unit/legacy-0-6-x/test-module-for-component-test.js

-  assert.equal(this.get('value'), '1');
+  return triggerEvent(input, 'change').then(() => {
+    assert.equal(this.get('value'), '1');
+  });


seems like we should assert.expect(1) here to make sure it actually runs correctly

cibernox · 2017-12-10T17:25:23Z

I said I would review but this is pretty massive 😵
Wish me luck

cibernox · 2017-12-10T17:35:34Z

addon-test-support/@ember/test-helpers/dom/-get-element.js

+  if (
+    selectorOrElement instanceof Window ||
+    selectorOrElement instanceof Document ||
+    selectorOrElement instanceof HTMLElement ||


This can be simplified. At least, HTMLElement and SVGElement are both of subclasses of Element, so that is a simplification.
Also, both Element and Document are subclasses of Node, but I'm not sure we want to be so generic, as there is other things that inherit from it that might not be valid targets, like DocumentFragments.

Confirm. I plan to refactor to use HTMLElement.prototype.nodeType which should “just work”.

cibernox · 2017-12-10T18:06:42Z

addon-test-support/@ember/test-helpers/dom/-is-focusable.js

+import isFormControl from './-is-form-control';
+import isContentEditable from './-is-content-editable';
+
+const FOCUSABLE_TAGS = ['LINK', 'A'];


I really don't know why I listed LINK as a focusable tag originally. I think it was a mindfart, it should be gone.

cibernox · 2017-12-10T18:08:54Z

addon-test-support/@ember/test-helpers/dom/-is-form-control.js

+
+  if (type === 'hidden') {
+    return false;
+  }


I think we could improve the logic here. I don't think that disabled buttons or inputs should be focusable.
I found this nice table we can copy from, even if we're not soooo exhaustive: https://allyjs.io/data-tables/focusable.html

cibernox · 2017-12-10T18:12:30Z

addon-test-support/@ember/test-helpers/dom/blur.js

+
+/**
+  @method blur
+  @param {String|HTMLElement} [target=document.activeElement] the element to blur


Perhaps {String|Element}, as SVG elements might also be focused and blurred.

cibernox · 2017-12-10T18:13:15Z

addon-test-support/@ember/test-helpers/dom/click.js

+
+/**
+  @method click
+  @param {String|HTMLElement} target


{String|Element}

cibernox · 2017-12-10T18:15:10Z

addon-test-support/@ember/test-helpers/dom/blur.js

+  @method __blur__
+  @param {HTMLElement} element
+*/
+export function __blur__(element) {


Why have private methods that are not async and don't return a promise? Perf?

Basically just for reuse where they are used in another async helper already (e.g. focus is used by click).

cibernox · 2017-12-10T18:15:28Z

addon-test-support/@ember/test-helpers/dom/fill-in.js

+
+/*
+  @method fillIn
+  @param {String|HTMLElement} target


{String|Element}

cibernox · 2017-12-10T18:23:16Z

addon-test-support/@ember/test-helpers/dom/fire-event.js

+  try {
+    event = document.createEvent('MouseEvents');
+    let eventOpts = merge(merge({}, DEFAULT_EVENT_OPTIONS), options);
+    event.initMouseEvent(


IDK if this targets Ember 3.0, but if it does, then we don't need this as the MouseEvent constructor works in IE11+

I don’t plan to change browser support as part of this effort, however we should add a comment to indicate it can be removed if IE8/IE9 are not supported.

9 and 10. Probably the same will happen to KeyboardEvent

TODO: Add support for pointer events? That might unify tap/click.

rwjblue · 2017-12-10T20:55:25Z

I said I would review but this is pretty massive

Sorry about that....

cibernox · 2017-12-10T22:22:10Z

addon-test-support/@ember/test-helpers/dom/fire-event.js

+  @param {Element} element
+  @param {String} type
+  @param {Object} [options]
+


Add @return {Event}

cibernox · 2017-12-10T22:25:41Z

addon-test-support/@ember/test-helpers/dom/trigger-event.js

+
+/**
+  @method triggerEvent
+  @param {String|HTMLElement} target


{String|Element}

cibernox · 2017-12-10T22:25:56Z

addon-test-support/@ember/test-helpers/dom/trigger-key-event.js

+
+/**
+  @public
+  @param {String|HTMLElement} target


{String|HTMLElement}

cibernox · 2017-12-10T22:37:12Z

addon-test-support/@ember/test-helpers/dom/trigger-key-event.js

+  @param {Boolean} [modifiers.metaKey=false]
+  @return {Promise<void>}
+*/
+export default function triggerKeyEvent(target, eventType, keyCode, modifiers = DEFAULT_MODIFIERS) {


I've noticed a name change here. In ember-native-dom-helpers it's named just keyEvent, which I copied from the current acceptance helper: https://emberjs.com/api/ember/2.15/classes/Ember.Test/methods/keyEvent?anchor=keyEvent&show=inherited%2Cprotected%2Cprivate%2Cdeprecated

I don't oppose to the name change, but it won't be a drop-in replacement for ember-native-dom-helpers.

Aye, agreed. The change was intentional (keyEvent "seemed" odd to me when what you are doing is triggering a keyboard event).

I think we can mitigate or at least ease some of the migration concerns by adding a reexport to ember-native-dom-helpers...

cibernox · 2017-12-10T22:44:47Z

addon-test-support/@ember/test-helpers/dom/trigger-key-event.js

+  @param {Boolean} [modifiers.metaKey=false]
+  @return {Promise<void>}
+*/
+export default function triggerKeyEvent(target, eventType, keyCode, modifiers = DEFAULT_MODIFIERS) {


When I initially copied keyEvent, I continued to use keyCode as the main way to set the key, but it seems to be deprecated and the new alternatives are KeyboardEvent.code and KeyboardEvent.key, specially the last one.

Perhaps we might want to start using it, or maybe assume that integers mean keyCode and strings mean key.

Agreed, I'll try to review the various docs and update accordingly...

Created #265 to track this.

cibernox · 2017-12-14T09:23:08Z

I do have a question. Should this PR also add shims to methods provided by Ember?
I think that it would feel weird to do import { click } from '@ember/test-helpers' but not do the same for visit, currentURL, currentPath and currentRouteName.

cibernox · 2017-12-14T09:25:39Z

Once this is published out of a beta version I plan to deprecate all the helpers implemented here. If I checked things right, the only two helpers you haven't implemented are scrollTo and selectFiles. Everything else will be deprecated.

These tests previously used a sync version of click that allowed `settled` to be tested with _only_ waiting for some portions of settledness. When the tests were converted to use the new public version of `click` the test began failing (because `click` doesn't kick off until the next tick of the event loop, so `settled` resolves immediately).

rwjblue · 2017-12-15T00:41:17Z

Perhaps adding find and findAll here could be a good idea. I feel that otherwise people will continue to use the built-in find global, and be confused about why find('button:contains("Submit")') works but click('button:contains("Submit")') doesn't.

I'm not opposed to bringing find and findAll over to ease the transition from both ember-native-dom-helpers and Ember's own legacy acceptance testing system, but I definitely do not want them to be the way we teach folks. IMHO, using this.element.querySelector or this.element.querySelectorAll (even though more verbose) is much better than a "magical" find in the long run...

Turbo87 · 2017-12-15T09:20:30Z

🎉

cibernox · 2017-12-15T09:51:14Z

As soon as there is a beta version I'll start testing it.

chriskrycho · 2017-12-17T22:40:13Z

This is awesome.

But, uhhh… we want typings for these, and I'm gonna get 'em somehow. 😏 The question is: do we…

add types by making this actually a TS library?
add types by adding an index.d.ts file here?
add types by putting them on DefinitelyTyped?

My inclination is if we can do the first, that'd be awesomest. I'm happy to help with that process, including doing whatever work to get the build stuff working right.

Turbo87 reviewed Dec 6, 2017

View reviewed changes

Turbo87 added the enhancement label Dec 6, 2017

rwjblue force-pushed the dom-helpers branch from 708b8b8 to 1f5b629 Compare December 6, 2017 23:02

rwjblue force-pushed the dom-helpers branch from 1f5b629 to b77049d Compare December 6, 2017 23:13

Turbo87 reviewed Dec 7, 2017

View reviewed changes

cibernox requested changes Dec 10, 2017

View reviewed changes

cibernox reviewed Dec 10, 2017

View reviewed changes

rwjblue force-pushed the dom-helpers branch 2 times, most recently from e5b13eb to f2feea9 Compare December 10, 2017 21:36

cibernox reviewed Dec 10, 2017

View reviewed changes

rwjblue mentioned this pull request Dec 12, 2017

settled: Ramp up timeout values for isSettled() check #262

Merged

rwjblue force-pushed the dom-helpers branch from 29bcce0 to 526e81c Compare December 14, 2017 04:38

rwjblue changed the title ~~[WIP] Bring over DOM helper implementation.~~ Bring over DOM helper implementation. Dec 14, 2017

rwjblue added 16 commits December 14, 2017 19:00

Remove LINK from FOCUSABLE_TAGS.

62e22c7

Replace HTMLElement || SVGElement with Element.

612c815

Split "click even sequence" from click.

f2b3c35

Add tap implementation.

2cc394e

Add unit tests for tap.

f03eead

Add basic waitFor implementation.

5c23057

More API documentation tweaks.

2a8b97d

Ensure that waitUntil properly handles callbacks with errors.

caf3e13

Return an array of elements for waitFor with count.

3b3d7c1

Add unit tests for waitFor.

326b968

Add basic assert.rejects infrastructure.

95b34ee

Make argument assertions into rejections.

adb006b

Refactor manual rejection assertions to assert.rejects.

7ee206a

Update moduleForAcceptance interop test to use click.

7e41f7f

Add comment RE: eager error creation without throwing.

e6f8668

rwjblue force-pushed the dom-helpers branch from edfe0fa to e6f8668 Compare December 15, 2017 00:08

rwjblue added 3 commits December 14, 2017 19:18

Add helpful errors to helpers for selector|element arg.

0412bbf

Ensure triggerKeyEvent rejects (and doesn't throw eagerly).

d07492b

Validate target in waitFor.

1848f83

This was referenced Dec 15, 2017

Update triggerKeyEvent to handle code and key. #265

Closed

Update fireEvent to use KeyboardEvent and MouseEvent constructors. #266

Closed

Add support for pointer events. #267

Open

rwjblue merged commit 3be6f1b into emberjs:master Dec 15, 2017

rwjblue deleted the dom-helpers branch December 15, 2017 00:44

cibernox mentioned this pull request Dec 22, 2017

Use key instead of keyCode for keyEvent helper? cibernox/ember-native-dom-helpers#136

Closed


		let rootElement = getContext().element;

		return rootElement.querySelector(selectorOrElement);

		@@ -0,0 +1,14 @@
		const FOCUSABLE_TAGS = ['INPUT', 'BUTTON', 'LINK', 'SELECT', 'A', 'TEXTAREA'];


		let element = getElement(selectorOrElement);

		if (isFocusable(element)) {

Bring over DOM helper implementation. #258

Bring over DOM helper implementation. #258

Conversation

rwjblue commented Dec 6, 2017 • edited

Turbo87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Turbo87 commented Dec 6, 2017

rwjblue commented Dec 6, 2017

cibernox commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cibernox commented Dec 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Dec 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cibernox commented Dec 14, 2017

cibernox commented Dec 14, 2017

rwjblue commented Dec 15, 2017

Turbo87 commented Dec 15, 2017

cibernox commented Dec 15, 2017

chriskrycho commented Dec 17, 2017

rwjblue commented Dec 6, 2017 •

edited