Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring over DOM helper implementation. #258

Merged
merged 61 commits into from
Dec 15, 2017
Merged

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Dec 6, 2017

TODO:

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

@rwjblue are there any tests for the new helpers?


let rootElement = getContext().element;

return rootElement.querySelector(selectorOrElement);
Copy link
Member

Choose a reason for hiding this comment

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

should we typecheck if selectorOrElement is a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

can a <link> actually be focused?

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable...

Copy link
Member

Choose a reason for hiding this comment

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

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


let element = getElement(selectorOrElement);

if (isFocusable(element)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely

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

/*
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Will update.

* @param selector
* @param type
* @param keyCode
* @param modifiers
Copy link
Member

Choose a reason for hiding this comment

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

the parameters are missing type information

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

return triggerEvent(
selectorOrElement,
type,
merge({ keyCode, which: keyCode, key: keyCode }, modifiers)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that would look much better indeed.

reject(waitUntilTimedOut);
}
}
setTimeout(tick, 10);
Copy link
Member

Choose a reason for hiding this comment

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

should this call tick() directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

TIL 🤔

@Turbo87
Copy link
Member

Turbo87 commented Dec 6, 2017

@cibernox can you have a look at this too?

@rwjblue
Copy link
Member Author

rwjblue commented Dec 6, 2017

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
Copy link
Contributor

cibernox commented Dec 6, 2017

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

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

Choose a reason for hiding this comment

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

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?

import isFocusable from './-is-focusable';
import { nextTickPromise } from '../-utils';

export function _blur(element) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should explicitly mark those functions as @private

element.dispatchEvent(event);
}

/*
Copy link
Member

Choose a reason for hiding this comment

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

/**

return event;
}

/*
Copy link
Member

Choose a reason for hiding this comment

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

/**

/*
@method buildMouseEvent
@param {String} type
@param {Object} (optional) options
Copy link
Member

Choose a reason for hiding this comment

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

usejsdoc.org has some special syntax for declaring optional parameters

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

Choose a reason for hiding this comment

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

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

this.element = document.querySelector('#qunit-fixture');

// create the element and focus in preparation for blur testing
element = buildInstrumentedElement('input');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...


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

Choose a reason for hiding this comment

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

what about "bluring via selector without context set"?

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');
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@cibernox
Copy link
Contributor

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

if (
selectorOrElement instanceof Window ||
selectorOrElement instanceof Document ||
selectorOrElement instanceof HTMLElement ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

import isFormControl from './-is-form-control';
import isContentEditable from './-is-content-editable';

const FOCUSABLE_TAGS = ['LINK', 'A'];
Copy link
Contributor

Choose a reason for hiding this comment

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

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


if (type === 'hidden') {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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


/**
@method click
@param {String|HTMLElement} target
Copy link
Contributor

Choose a reason for hiding this comment

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

{String|Element}

@method __blur__
@param {HTMLElement} element
*/
export function __blur__(element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


/*
@method fillIn
@param {String|HTMLElement} target
Copy link
Contributor

Choose a reason for hiding this comment

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

{String|Element}

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

9 and 10. Probably the same will happen to KeyboardEvent

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@rwjblue
Copy link
Member Author

rwjblue commented Dec 10, 2017

I said I would review but this is pretty massive

Sorry about that....

@rwjblue rwjblue force-pushed the dom-helpers branch 2 times, most recently from e5b13eb to f2feea9 Compare December 10, 2017 21:36
@param {Element} element
@param {String} type
@param {Object} [options]

Copy link
Contributor

Choose a reason for hiding this comment

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

Add @return {Event}


/**
@method triggerEvent
@param {String|HTMLElement} target
Copy link
Contributor

Choose a reason for hiding this comment

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

{String|Element}


/**
@public
@param {String|HTMLElement} target
Copy link
Contributor

Choose a reason for hiding this comment

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

{String|HTMLElement}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #265 to track this.

@rwjblue rwjblue changed the title [WIP] Bring over DOM helper implementation. Bring over DOM helper implementation. Dec 14, 2017
@cibernox
Copy link
Contributor

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
Copy link
Contributor

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.

@rwjblue
Copy link
Member Author

rwjblue commented Dec 15, 2017

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...

@rwjblue rwjblue merged commit 3be6f1b into emberjs:master Dec 15, 2017
@rwjblue rwjblue deleted the dom-helpers branch December 15, 2017 00:44
@Turbo87
Copy link
Member

Turbo87 commented Dec 15, 2017

🎉

@cibernox
Copy link
Contributor

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

@chriskrycho
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants