Error when `fillIn`/ `typeIn` attempt to enter a value longer than `maxlength` #747

jaydgruber · 2019-12-03T02:01:15Z

What

adds failing test cases where fillIn and typeIn ignore the maxlength attribute of an input
adds a util function isMaxLengthConstrained
if isMaxLengthConstrained = true, will
fill-in helper
- replaces element.value w/ the new value, truncated to maxlength if needed
for type-in helper:
- on character entries that would exceed the maxlength, fires ['keydown', 'keypress', 'keyup'] events, but no input event, and element.value does not change
adds unit test cases for same

Why

helpers should respect constraint validation
requested failing test case in fillIn and typeIn ignore maxlength input attribute #684

rwjblue · 2019-12-04T15:54:26Z

Thanks for working on failing test cases here! If you have the time, you can probably implement a check that truncates (or whatever the "right" behavior is) here in these spots:

ember-test-helpers/addon-test-support/@ember/test-helpers/dom/type-in.ts

Line 89 in 7d8a9a5

element.value = element.value + character;

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

Line 48 in 7d8a9a5

element.value = text;

jaydgruber · 2019-12-04T19:22:28Z

I think fillIn is pretty straightforward - just truncate at maxlength, as it replaces any previous value in the control.

For typeIn I'm less sure. I think typeIn is additive, so if I did

await typeIn('input', 'a');
await typeIn('input', 'b');

I would end up w/ 'ab'. So I guess typeIn should stop modifying element.value when the maxlength of the combined (previous values + new values).length === maxlength.
But I think it should still fire all the key events? Like if a user tried to type past the maxlength of a field, they still hit the keys, just no more values get added.

rwjblue · 2019-12-04T20:06:31Z

But I think it should still fire all the key events? Like if a user tried to type past the maxlength of a field, they still hit the keys, just no more values get added.

Yep, that is what I think also but we should confirm that is the actual browser.

jaydgruber · 2019-12-05T21:59:55Z

I went down a bit of a rabbit hole reading docs around https://developer.mozilla.org/en-US/docs/Web/API/Constraint_validation
[max, maxlength, min, minlength, pattern, size, step] all have validation that prevents form submission, but looks like only maxlength prevents users from entering values.
I tried to figure out a way to check the Constraint Validation API's https://webplatform.github.io/docs/dom/ValidityState/tooLong/, but was getting false negatives. Not sure what sort of event (dirty flag maybe?) needs to be fired to get checkValidity to report correctly when the input was not user-entered.
Anyways, abandoned that approach in favor of simpler one-off

rwjblue

No changes were needed to typeIn?

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

jaydgruber · 2019-12-06T16:47:21Z

Still working on typeIn

addon-test-support/@ember/test-helpers/dom/-is-maxlength-constrained.ts

btecu · 2020-02-26T13:39:18Z

@jaydgruber what's the status on this?

jaydgruber · 2020-02-26T17:46:35Z

oh I guess I could have pinged someone for re-review. this one should be ready to go, pending the one above question about do I need to do more to make it 'TypeScript-y'

rwjblue · 2020-04-20T15:33:29Z

@ro0gr - Would you mind reviewing? This touches a bunch of the same general areas as #741 / #779 / #778 so it will need to land after those.

ro0gr · 2020-04-20T20:40:52Z

Seems like input event stops being triggered once we reach the maxlength constraint. I've checked it in https://codepen.io/ro0gr/pen/YzyGvrZ?editors=1010

As far as I can see, in this PR, we try to trim the input and keep emitting events. If my codepen test is correct, I think we should not emit any events. Maybe we should even throw, to make un-intended fillIn more clear to the test helper user. Some context with related discussion in the thread #778 (comment)

jaydgruber · 2020-04-20T20:43:42Z

Yeah, I think throw similarly makes sense here

addon-test-support/@ember/test-helpers/dom/-is-maxlength-constrained.ts

ro0gr · 2020-04-21T14:25:53Z

@jaydgruber I just wanna check if you are going to keep working on this? in case you are busy with something else, I can take it over. Otherwise it'd be glad to provide any assistance!

rwjblue · 2020-04-21T14:26:34Z

OK #741, #779, and #778 have all landed. This is ready for a rebase, and the tweaks that @ro0gr mentioned above (thanks @ro0gr for reviewing!).

jaydgruber · 2020-04-21T16:21:45Z

@ro0gr I won't be able to get to it until probably Friday. I've got no objections if you want to pick it up. Thanks

ro0gr · 2020-04-21T18:44:48Z

@jaydgruber Great! sounds like you are in the game :) Take your time.

…-in and type-in to use helper

jaydgruber · 2020-04-25T02:23:39Z

@ro0gr this should be ready for another look

ro0gr

Looks good to me. Events test for IE seems to be broken though.

addon-test-support/@ember/test-helpers/dom/-is-maxlength-constrained.ts

ro0gr · 2020-04-25T18:59:22Z

tests/unit/dom/type-in-test.js

+    await assert.rejects(
+      typeIn(element, tooLongString).finally(() => {
+        // should throw before the second input event
+        assert.verifySteps(expectedEvents.slice(0, 8));


Looks like for isIE11 it would expect to throw before the third keypress, that doesn't seem correct.

@rwjblue do we have IE tests broken?

Ya, it is possible, we don't have IE11 under CI test at the moment. We should look into hooking that up... 🤔

I believe I corrected the assertion here, but I don't have an IE launcher for testem locally, so not sure the best way to confirm

addon-test-support/@ember/test-helpers/dom/type-in.ts

ro0gr

LGTM 👍

rwjblue · 2020-05-05T20:36:48Z

Awesome, thank you!

jaydgruber mentioned this pull request Dec 3, 2019

fillIn and typeIn ignore maxlength input attribute #684

Closed

rwjblue added the bug label Dec 4, 2019

jaydgruber force-pushed the issue-684-failing-tests branch from 97a3040 to ae4aab2 Compare December 5, 2019 21:44

rwjblue reviewed Dec 6, 2019

View reviewed changes

addon-test-support/@ember/test-helpers/dom/fill-in.ts Outdated Show resolved Hide resolved

addon-test-support/@ember/test-helpers/dom/fill-in.ts Outdated Show resolved Hide resolved

jaydgruber changed the title ~~Adds failing test cases for fillIn and typeIn do not respect maxlength attribute~~ WIP: Adds failing test cases for fillIn and typeIn do not respect maxlength attribute Dec 6, 2019

jaydgruber changed the title ~~WIP: Adds failing test cases for fillIn and typeIn do not respect maxlength attribute~~ [Bugfix] makes fillIn and typeIn respect maxlength attribute Dec 17, 2019

jaydgruber changed the title ~~[Bugfix] makes fillIn and typeIn respect maxlength attribute~~ WIP: [Bugfix] makes fillIn and typeIn respect maxlength attribute Dec 17, 2019

jaydgruber changed the title ~~WIP: [Bugfix] makes fillIn and typeIn respect maxlength attribute~~ [Bugfix] makes fillIn and typeIn respect maxlength attribute Dec 17, 2019

jaydgruber commented Dec 17, 2019

View reviewed changes

addon-test-support/@ember/test-helpers/dom/-is-maxlength-constrained.ts Outdated Show resolved Hide resolved

jaydgruber changed the title ~~[Bugfix] makes fillIn and typeIn respect maxlength attribute~~ WIP: [Bugfix] makes fillIn and typeIn respect maxlength attribute Apr 20, 2020

ro0gr reviewed Apr 20, 2020

View reviewed changes

addon-test-support/@ember/test-helpers/dom/-is-maxlength-constrained.ts Outdated Show resolved Hide resolved

ro0gr mentioned this pull request Apr 21, 2020

cleanup: restrict Target to string or Element only #823

Closed

2 tasks

added test cases. added is-maxlength-constrained helper. updated fill…

d1109f3

…-in and type-in to use helper

jaydgruber marked this pull request as draft April 25, 2020 00:26

jaydgruber force-pushed the issue-684-failing-tests branch from 4bc205e to d1109f3 Compare April 25, 2020 00:28

changed fillIn to throw on exceeds maxlength. changed test

daa6d28

lint fixes

a08253e

jaydgruber marked this pull request as ready for review April 25, 2020 02:22

jaydgruber changed the title ~~WIP: [Bugfix] makes fillIn and typeIn respect maxlength attribute~~ [Bugfix] makes fillIn and typeIn respect maxlength attribute Apr 25, 2020

ro0gr reviewed Apr 28, 2020

View reviewed changes

addon-test-support/@ember/test-helpers/dom/type-in.ts Outdated Show resolved Hide resolved

jaydgruber changed the title ~~[Bugfix] makes fillIn and typeIn respect maxlength attribute~~ WIP: [Bugfix] makes fillIn and typeIn respect maxlength attribute Apr 29, 2020

jaydgruber added 3 commits May 4, 2020 11:12

address PR comments

fe27abb

forgot to git add. linted

d9d0d65

should fix IE test

ba9b22e

jaydgruber changed the title ~~WIP: [Bugfix] makes fillIn and typeIn respect maxlength attribute~~ [Bugfix] makes fillIn and typeIn respect maxlength attribute May 4, 2020

ro0gr approved these changes May 4, 2020

View reviewed changes

rwjblue changed the title ~~[Bugfix] makes fillIn and typeIn respect maxlength attribute~~ Error when fillIn/ typeIn attempt to enter a value longer than maxlength May 5, 2020

rwjblue merged commit 1881f3e into emberjs:master May 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error when `fillIn`/ `typeIn` attempt to enter a value longer than `maxlength` #747

Error when `fillIn`/ `typeIn` attempt to enter a value longer than `maxlength` #747

jaydgruber commented Dec 3, 2019 •

edited

Loading

rwjblue commented Dec 4, 2019

jaydgruber commented Dec 4, 2019

rwjblue commented Dec 4, 2019

jaydgruber commented Dec 5, 2019

rwjblue left a comment

jaydgruber commented Dec 6, 2019

btecu commented Feb 26, 2020

jaydgruber commented Feb 26, 2020

rwjblue commented Apr 20, 2020

ro0gr commented Apr 20, 2020

jaydgruber commented Apr 20, 2020

ro0gr commented Apr 21, 2020 •

edited

Loading

rwjblue commented Apr 21, 2020

jaydgruber commented Apr 21, 2020

ro0gr commented Apr 21, 2020

jaydgruber commented Apr 25, 2020

ro0gr left a comment

ro0gr Apr 25, 2020

rwjblue Apr 29, 2020

jaydgruber May 4, 2020

ro0gr left a comment

rwjblue commented May 5, 2020

Error when fillIn/ typeIn attempt to enter a value longer than maxlength #747

Error when fillIn/ typeIn attempt to enter a value longer than maxlength #747

Conversation

jaydgruber commented Dec 3, 2019 • edited Loading

What

Why

rwjblue commented Dec 4, 2019

jaydgruber commented Dec 4, 2019

rwjblue commented Dec 4, 2019

jaydgruber commented Dec 5, 2019

rwjblue left a comment

Choose a reason for hiding this comment

jaydgruber commented Dec 6, 2019

btecu commented Feb 26, 2020

jaydgruber commented Feb 26, 2020

rwjblue commented Apr 20, 2020

ro0gr commented Apr 20, 2020

jaydgruber commented Apr 20, 2020

ro0gr commented Apr 21, 2020 • edited Loading

rwjblue commented Apr 21, 2020

jaydgruber commented Apr 21, 2020

ro0gr commented Apr 21, 2020

jaydgruber commented Apr 25, 2020

ro0gr left a comment

Choose a reason for hiding this comment

ro0gr Apr 25, 2020

Choose a reason for hiding this comment

rwjblue Apr 29, 2020

Choose a reason for hiding this comment

jaydgruber May 4, 2020

Choose a reason for hiding this comment

ro0gr left a comment

Choose a reason for hiding this comment

rwjblue commented May 5, 2020

Error when `fillIn`/ `typeIn` attempt to enter a value longer than `maxlength` #747

Error when `fillIn`/ `typeIn` attempt to enter a value longer than `maxlength` #747

jaydgruber commented Dec 3, 2019 •

edited

Loading

ro0gr commented Apr 21, 2020 •

edited

Loading