Bug 67173 - Add a test for lastChangeWasUserEdit in HTMLInputElement and HTMLTextAreaElement
Summary: Add a test for lastChangeWasUserEdit in HTMLInputElement and HTMLTextAreaElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 67152
  Show dependency treegraph
 
Reported: 2011-08-29 18:23 PDT by Ryosuke Niwa
Modified: 2011-08-29 19:02 PDT (History)
7 users (show)

See Also:


Attachments
adds a basic test (8.63 KB, patch)
2011-08-29 18:27 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-08-29 18:23:40 PDT
We should have a test for lastChangeWasUserEdit per the discussion on the bug 67152
Comment 1 Ryosuke Niwa 2011-08-29 18:27:28 PDT
Created attachment 105557 [details]
adds a basic test
Comment 2 Darin Adler 2011-08-29 18:35:32 PDT
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?
Comment 3 Kent Tamura 2011-08-29 18:41:23 PDT
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.
Comment 4 Darin Adler 2011-08-29 18:43:28 PDT
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.
Comment 5 Ryosuke Niwa 2011-08-29 18:47:44 PDT
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.
Comment 6 Ryosuke Niwa 2011-08-29 18:49:14 PDT
(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.
Comment 7 Ryosuke Niwa 2011-08-29 19:02:49 PDT
Committed r94038: <http://trac.webkit.org/changeset/94038>