Bug 185433 – Resign Strong Password appearance when text field value changes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Similar to bug #184795, we should resign the Strong Password appearance when the value of the text field changes.

[Daniel Bates]

<rdar://problem/39958508>

[Daniel Bates]

Created attachment 339845 [details]
Patch with tests

[Daniel Bates]

Created attachment 339852 [details]
Patch and tests

Updated testDidResignInputElementStrongPasswordAppearanceAfterEvaluatingJavaScript() in Tools/TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm to reset the readytoResign flag each time it is invoked so that we one test run does not interfere with another.

[Ryosuke Niwa]

Comment on attachment 339852 [details]
Patch and tests

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

r=me if test cases for calling reset() and setting a smaller maxlength are added.

> LayoutTests/fast/forms/auto-fill-button/hide-auto-fill-strong-password-button-when-value-changes.html:6
> +window.markFieldsAsAutoFilled = true;

I'm not saying we have to change the test harness in this patch but it's not great that the helper script relies on a global variable like this.

> LayoutTests/fast/forms/auto-fill-button/hide-auto-fill-strong-password-button-when-value-changes.html:14
> +    for (let inputElement of inputElements)

Use const?

> LayoutTests/fast/forms/auto-fill-button/resources/process-auto-fill-button-type-and-invoke-runTest.js:8
> +    for (let inputElement of inputElements) {

Use const?

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 339852 [details]
> Patch and tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339852&action=review
> 
> r=me if test cases for calling reset() and setting a smaller maxlength are
> added.
> 

Will add tests for setting a smaller maxlength as well as a larger minlength. Note that in either case we do not modify the value of a text field.

Bug #185440 will add tests for Form.reset().

> > LayoutTests/fast/forms/auto-fill-button/hide-auto-fill-strong-password-button-when-value-changes.html:6
> > +window.markFieldsAsAutoFilled = true;
> 
> I'm not saying we have to change the test harness in this patch but it's not
> great that the helper script relies on a global variable like this.
> 

This line is unnecessary and we do not make use of window.markFieldsAsAutoFilled in this patch.

For completeness, an earlier iteration of this patch used this global to mark all text fields as autofilled.

> > LayoutTests/fast/forms/auto-fill-button/hide-auto-fill-strong-password-button-when-value-changes.html:14
> > +    for (let inputElement of inputElements)
> 
> Use const?
> 

I am not going to make this change because we are modifying inputElement's state below.  Making inputElement const and mutating its state would be equivalent in C++ to calling setter on a const object. And we typically do not have such const setters in C++. I do not see the benefit of doing something different for JavaScript.

> > LayoutTests/fast/forms/auto-fill-button/resources/process-auto-fill-button-type-and-invoke-runTest.js:8
> > +    for (let inputElement of inputElements) {
> 
> Use const?

Ditto.

[Daniel Bates]

Committed r231517: <https://trac.webkit.org/changeset/231517>