Bug 67173 – Add a test for lastChangeWasUserEdit in HTMLInputElement and HTMLTextAreaElement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Ryosuke Niwa]

We should have a test for lastChangeWasUserEdit per the discussion on the bug 67152

[Ryosuke Niwa]

Created attachment 105557 [details]
adds a basic test

[Darin Adler]

Comment on attachment 105557 [details]
adds a basic test

View in context: https://bugs.webkit.org/attachment.cgi?id=105557&action=review

> Source/WebCore/testing/Internals.cpp:275
> +    if (HTMLInputElement* input = textField->toInputElement())

Wow, never saw this function before. Having toInputElement is crazy. Did we really add that?! Why have this for one specific element type?

> LayoutTests/fast/forms/textfield-lastchange-was-useredit.html:13
> +if (!window.layoutTestController || !window.internals)

This is the only line of code that needs to say "window." in its expression.

> LayoutTests/fast/forms/textfield-lastchange-was-useredit.html:22
> +    shouldBeFalse('window.internals.wasLastChangeUserEdit(textField)');

Just internals would be fine.

> LayoutTests/fast/forms/textfield-lastchange-was-useredit.html:26
> +    textField.value = 'hello';
> +    debug('\nAfter setting value');
> +    shouldBeFalse('window.internals.wasLastChangeUserEdit(textField)');

Another way to write this is:

    shouldBeFalse("textField.value = 'hello'; internals.wasLastChangeUserEdit(textField)");

Using lines like that makes the test output less wordy than all those debug statements.

> LayoutTests/fast/forms/textfield-lastchange-was-useredit.html:59
> +        textField.textContent = "hello\nworld";
> +        debug('\nAfter modifying textContent');
> +        shouldBeFalse('window.internals.wasLastChangeUserEdit(textField)');

Maybe should test modifying innerText too?

[Kent Tamura]

Comment on attachment 105557 [details]
adds a basic test

View in context: https://bugs.webkit.org/attachment.cgi?id=105557&action=review

>> Source/WebCore/testing/Internals.cpp:275
>> +    if (HTMLInputElement* input = textField->toInputElement())
> 
> Wow, never saw this function before. Having toInputElement is crazy. Did we really add that?! Why have this for one specific element type?

I think it was for WML-HTML abstraction.  toInputElement() returned dom/InputElement.

[Darin Adler]

Comment on attachment 105557 [details]
adds a basic test

View in context: https://bugs.webkit.org/attachment.cgi?id=105557&action=review

>>> Source/WebCore/testing/Internals.cpp:275
>>> +    if (HTMLInputElement* input = textField->toInputElement())
>> 
>> Wow, never saw this function before. Having toInputElement is crazy. Did we really add that?! Why have this for one specific element type?
> 
> I think it was for WML-HTML abstraction.  toInputElement() returned dom/InputElement.

I think we should stop using it, use hasTagName(inputTag) instead, and get rid of it.

[Ryosuke Niwa]

Thanks for the review! THAT was quick.

(In reply to comment #3)
> (From update of attachment 105557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105557&action=review
> 
> >> Source/WebCore/testing/Internals.cpp:275
> >> +    if (HTMLInputElement* input = textField->toInputElement())
> > 
> > Wow, never saw this function before. Having toInputElement is crazy. Did we really add that?! Why have this for one specific element type?
> 
> I think it was for WML-HTML abstraction.  toInputElement() returned dom/InputElement.

I'll avoid using this function then.

(In reply to comment #2)
> > LayoutTests/fast/forms/textfield-lastchange-was-useredit.html:13
> > +if (!window.layoutTestController || !window.internals)
> 
> This is the only line of code that needs to say "window." in its expression.

Oops, you're right. Will remove "window." from other lines.

> > LayoutTests/fast/forms/textfield-lastchange-was-useredit.html:26
> > +    textField.value = 'hello';
> > +    debug('\nAfter setting value');
> > +    shouldBeFalse('window.internals.wasLastChangeUserEdit(textField)');
> 
> Another way to write this is:
> 
>     shouldBeFalse("textField.value = 'hello'; internals.wasLastChangeUserEdit(textField)");
> 
> Using lines like that makes the test output less wordy than all those debug statements.

Okay.  I guess I'll change to that style for simple cases.

> > LayoutTests/fast/forms/textfield-lastchange-was-useredit.html:59
> > +        textField.textContent = "hello\nworld";
> > +        debug('\nAfter modifying textContent');
> > +        shouldBeFalse('window.internals.wasLastChangeUserEdit(textField)');
> 
> Maybe should test modifying innerText too?

That's a good point.  Will do.

[Ryosuke Niwa]

(In reply to comment #4)
> > I think it was for WML-HTML abstraction.  toInputElement() returned dom/InputElement.
> 
> I think we should stop using it, use hasTagName(inputTag) instead, and get rid of it.

Enthusiastically agreed.  Filed https://bugs.webkit.org/show_bug.cgi?id=67175.

[Ryosuke Niwa]

Committed r94038: <http://trac.webkit.org/changeset/94038>