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.

Sign up for GitHub

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

Jump to bottom

Allow typeIn for contenteditable elements #869

Merged
merged 3 commits into from
May 23, 2020
Merged

Allow typeIn for contenteditable elements #869

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
663c9d3
narrow contenteditabletype
jeffhertzler May 21, 2020
55bbb76
allow typeIn for contenteditable
jeffhertzler May 21, 2020
80786e8
better guards, remove casting
jeffhertzler May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion addon-test-support/@ember/test-helpers/dom/-target.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ type Target = string | Element | Document | Window;

export default Target;

export interface HTMLElementContentEditable extends HTMLElement {
isContentEditable: true;
}

// eslint-disable-next-line require-jsdoc
export function isElement(target: any): target is Element {
return target.nodeType === Node.ELEMENT_NODE;
Expand All @@ -13,6 +17,6 @@ export function isDocument(target: any): target is Document {
}

// eslint-disable-next-line require-jsdoc
export function isContentEditable(element: Element): element is HTMLElement {
export function isContentEditable(element: Element): element is HTMLElementContentEditable {
return 'isContentEditable' in element && (element as HTMLElement).isContentEditable;
}
47 changes: 29 additions & 18 deletions addon-test-support/@ember/test-helpers/dom/type-in.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { __focus__ } from './focus';
import { Promise } from 'rsvp';
import fireEvent from './fire-event';
import guardForMaxlength from './-guard-for-maxlength';
import Target from './-target';
import Target, { isContentEditable, HTMLElementContentEditable } from './-target';
import { __triggerKeyEvent__ } from './trigger-key-event';
import { log } from '@ember/test-helpers/dom/-logging';

Expand Down Expand Up @@ -46,27 +46,30 @@ export default function typeIn(target: Target, text: string, options: Options =
throw new Error('Must pass an element or selector to `typeIn`.');
}

const element = getElement(target);
const element = getElement(target) as Element | HTMLElement;
Copy link
Member

Choose a reason for hiding this comment

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

Curious why we need to cast this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partially because that's what fillIn was doing. There was a type error that I don't recall. I'll go check on it.

Copy link
Member

Choose a reason for hiding this comment

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

kk, doesn't seem like a major issue to me, but if we have a type misalignment in getElement we can try to fix it instead of forcing all consumers to cast. Alternatively, if specific helper methods can only work on HTMLElement (vs the more generic Element) maybe we should make an assertion method that narrows the types for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a commit to this branch that alters the type guards and removes the casting but it's not reflecting in the PR 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One bit to note here about how both fillIn and typeIn work is they're assuming FormControl is ok, but that includes selects and buttons which don't seem like they should be allowed. Might be nice to have some sort of fillable type guard that narrows down to input/textarea and maybe content editable?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, totally agree. Seems like a great candidate for a follow up PR if you have the time.

if (!element) {
throw new Error(`Element not found when calling \`typeIn('${target}')\``);
}
if (!isFormControl(element)) {
throw new Error('`typeIn` is only usable on form controls.');
}

if (typeof text === 'undefined' || text === null) {
throw new Error('Must provide `text` when calling `typeIn`.');
}

if (element.disabled) {
throw new Error(`Can not \`typeIn\` disabled '${target}'.`);
}
if (isFormControl(element)) {
if (element.disabled) {
throw new Error(`Can not \`typeIn\` disabled '${target}'.`);
}

if ('readOnly' in element && element.readOnly) {
throw new Error(`Can not \`typeIn\` readonly '${target}'.`);
}
if ('readOnly' in element && element.readOnly) {
throw new Error(`Can not \`typeIn\` readonly '${target}'.`);
}

__focus__(element);
__focus__(element);
} else if (isContentEditable(element)) {
__focus__(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 would generally prefer to avoid having these two branches that each call __focus__, think there is a way to unify a bit to avoid the extra branching?

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! That's definitely doable. Mostly just mirroring the structure of fillIn

Copy link
Member

Choose a reason for hiding this comment

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

Yep, makes sense, thanks for working on it!

} else {
throw new Error('`typeIn` is only usable on form controls or contenteditable elements.');
}

let { delay = 50 } = options;

Expand All @@ -77,15 +80,18 @@ export default function typeIn(target: Target, text: string, options: Options =
}

// eslint-disable-next-line require-jsdoc
function fillOut(element: FormControl, text: string, delay: number) {
function fillOut(element: FormControl | HTMLElementContentEditable, text: string, delay: number) {
const inputFunctions = text.split('').map(character => keyEntry(element, character));
return inputFunctions.reduce((currentPromise, func) => {
return currentPromise.then(() => delayedExecute(delay)).then(func);
}, Promise.resolve(undefined));
}

// eslint-disable-next-line require-jsdoc
function keyEntry(element: FormControl, character: string): () => void {
function keyEntry(
element: FormControl | HTMLElementContentEditable,
character: string
): () => void {
let shiftKey = character === character.toUpperCase() && character !== character.toLowerCase();
let options = { shiftKey };
let characterKey = character.toUpperCase();
Expand All @@ -95,10 +101,15 @@ function keyEntry(element: FormControl, character: string): () => void {
.then(() => __triggerKeyEvent__(element, 'keydown', characterKey, options))
.then(() => __triggerKeyEvent__(element, 'keypress', characterKey, options))
.then(() => {
const newValue = element.value + character;
guardForMaxlength(element, newValue, 'typeIn');

element.value = newValue;
if (isFormControl(element)) {
const newValue = element.value + character;
guardForMaxlength(element, newValue, 'typeIn');

element.value = newValue;
} else {
const newValue = element.innerHTML + character;
element.innerHTML = newValue;
}
fireEvent(element, 'input');
})
.then(() => __triggerKeyEvent__(element, 'keyup', characterKey, options));
Expand Down
17 changes: 15 additions & 2 deletions tests/unit/dom/type-in-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,24 @@ module('DOM Helper: typeIn', function (hooks) {
assert.equal(element.value, 'foo');
});

test('typing in not a form control', async function (assert) {
test('typing in a contenteditable element', async function (assert) {
element = buildInstrumentedElement('div');
element.setAttribute('contenteditable', 'true');
await typeIn(element, 'foo');

assert.verifySteps(expectedEvents);
assert.strictEqual(document.activeElement, element, 'activeElement updated');
assert.equal(element.innerHTML, 'foo');
});

test('typing in a non-typable element', async function (assert) {
element = buildInstrumentedElement('div');

await setupContext(context);
assert.rejects(typeIn(`#${element.id}`, 'foo'), /`typeIn` is only usable on form controls/);
assert.rejects(
typeIn(`#${element.id}`, 'foo'),
/`typeIn` is only usable on form controls or contenteditable elements/
);
});

test('typing in a disabled element', async function (assert) {
Expand Down