Bug 200037 – Allow clients to toggle a text input field between being viewable and having characters hidden while maintaining a yellow auto-filled appearance

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Priyanka]

Allow Clients to make an input field considered autofilled and secure while viewable

[Priyanka]

Created attachment 374760 [details]
Patch

[Priyanka]

Created attachment 374843 [details]
Patch

[Daniel Bates]

Test please. Look at strong password tests for examples.  Indentation of case statements look off, like tabs or something. Fix. Make sure no tab chars in files

[Daniel Bates]

Comment on attachment 374843 [details]
Patch

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

Good start, but this patch is missing invalidation logic to handle things like (off the top of my head):

1. Form reset - grep for ::reset
2. When HTML type attribute changes
3. When HTML value attribute changes


(2) and (3) call :: resignStrongPasswordAppearance. <— Grep that, it needs to be patched. Look at all callers, more patching may be needed.

> Source/WebCore/css/html.css:740
> +    background-color: #FAFFBD !important;
> +    background-image: none !important;
> +    color: #000000 !important;

In my opinion I would share with ^^^^ selectors instead of duplicating.

[Priyanka]

Created attachment 375585 [details]
Patch

[Priyanka]

Added tests, added invalidation logic, and shared with selectors.

[Priyanka]

Created attachment 375588 [details]
Patch

[Priyanka]

Created attachment 375590 [details]
Patch

[Priyanka]

Created attachment 375621 [details]
Patch

[Priyanka]

Created attachment 375623 [details]
Patch

[Priyanka]

Created attachment 375633 [details]
Patch

[Daniel Bates]

Comment on attachment 375633 [details]
Patch

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

This looks really good.

> Source/WebCore/ChangeLog:3
> +        Allow Clients to make an input field considered autofilled and secure while viewable

Bug title should be fixed 🙂:

Clients => clients

> Source/WebCore/ChangeLog:6
> +

I feel that the title is hard to read and confuses me. I don't understand how a field can be considered "secure" and viewable. <-- I know what you mean by reading the patch, but that's not how I would describe this change. Can we please improve the title? If not, please provide a more elaborated description of the purpose of this change under the "Reviewed by" line.

> Source/WebCore/css/html.css:740
> +    -webkit-text-security: none !important;
> +    cursor: default !important;
> +    font-family: monospace;

This is duplicating the properties of input:-webkit-autofill-strong-password (above). Please share.

> LayoutTests/fast/forms/auto-fill-button/resources/process-auto-fill-button-type-and-invoke-runTest-for-strong-password-viewable.js:13
> +window.onload = function ()
> +{
> +    if (!window.internals) {
> +        console.log("This test must be run in DumpRenderTree or WebKitTestRunner.");
> +        return;
> +    }
> +    let inputElements = document.getElementsByTagName("input");
> +    for (let inputElement of inputElements) {
> +        internals.setAutoFilledAndViewable(inputElement, inputElement.dataset.autoFilledAndViewable == "true");
> +    }
> +    if (window.runTest)
> +        window.runTest();
> +}

This duplicates the code in <https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/forms/auto-fill-button/resources/process-auto-fill-button-type-and-invoke-runTest.js>. Only difference is checking for inputElement.dataset.autoFilledAndViewable. Please modify process-auto-fill-button-type-and-invoke-runTest.js instead of duplicating it.

[Daniel Bates]

Comment on attachment 375633 [details]
Patch

I just realized that this patch does not handle the case where the field's value changes programmatically. We need to patch up setValueFromRenderer()

[Daniel Bates]

(In reply to Daniel Bates from comment #13)
> Comment on attachment 375633 [details]
> Patch
> 
> I just realized that this patch does not handle the case where the field's
> value changes programmatically. We need to patch up setValueFromRenderer()

And add a test case please 🙂

[Daniel Bates]

(In reply to Daniel Bates from comment #13)
> Comment on attachment 375633 [details]
> Patch
> 
> I just realized that this patch does not handle the case where the field's
> value changes programmatically. We need to patch up setValueFromRenderer()

Actually, I'm not sure what you intended the behavior to be for this feature when this happens. Either need to patch up setValueFromRenderer () or code is okay as-is and you just need to explain in the ChangeLog why we don't need to touch setValueFromRenderer().

[Priyanka]

(In reply to Daniel Bates from comment #15)
> (In reply to Daniel Bates from comment #13)
> > Comment on attachment 375633 [details]
> > Patch
> > 
> > I just realized that this patch does not handle the case where the field's
> > value changes programmatically. We need to patch up setValueFromRenderer()
> 
> Actually, I'm not sure what you intended the behavior to be for this feature
> when this happens. Either need to patch up setValueFromRenderer () or code
> is okay as-is and you just need to explain in the ChangeLog why we don't
> need to touch setValueFromRenderer().

As per off-line discussion, behavior is intentional and WebCore ChangeLog updated to describe the reason why we don't change setValueFromRenderer.

[Priyanka]

Created attachment 375651 [details]
Patch

[Priyanka]

Created attachment 375705 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375705 [details]
Patch

Clearing flags on attachment: 375705

Committed r248373: <https://trac.webkit.org/changeset/248373>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54039116>