Bug 200037 - Allow clients to toggle a text input field between being viewable and having characters hidden while maintaining a yellow auto-filled appearance
Summary: Allow clients to toggle a text input field between being viewable and having ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari 12
Hardware: All Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-23 11:41 PDT by Priyanka
Modified: 2019-08-07 10:25 PDT (History)
8 users (show)

See Also:


Attachments
Patch (18.43 KB, patch)
2019-07-23 21:33 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2019-07-24 18:43 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (36.47 KB, patch)
2019-08-05 17:25 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (36.51 KB, patch)
2019-08-05 17:35 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (78.42 KB, patch)
2019-08-05 17:46 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (84.07 KB, patch)
2019-08-06 06:43 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (172.76 KB, patch)
2019-08-06 08:32 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (86.08 KB, patch)
2019-08-06 11:39 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (86.33 KB, patch)
2019-08-06 14:55 PDT, Priyanka
no flags Details | Formatted Diff | Diff
Patch (86.51 KB, patch)
2019-08-07 09:51 PDT, Priyanka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Priyanka 2019-07-23 11:41:08 PDT
Allow Clients to make an input field considered autofilled and secure while viewable
Comment 1 Priyanka 2019-07-23 21:33:12 PDT
Created attachment 374760 [details]
Patch
Comment 2 Priyanka 2019-07-24 18:43:54 PDT
Created attachment 374843 [details]
Patch
Comment 3 Daniel Bates 2019-07-24 19:19:10 PDT
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
Comment 4 Daniel Bates 2019-07-24 19:45:01 PDT
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.
Comment 5 Priyanka 2019-08-05 17:25:28 PDT
Created attachment 375585 [details]
Patch
Comment 6 Priyanka 2019-08-05 17:26:08 PDT
Added tests, added invalidation logic, and shared with selectors.
Comment 7 Priyanka 2019-08-05 17:35:09 PDT
Created attachment 375588 [details]
Patch
Comment 8 Priyanka 2019-08-05 17:46:53 PDT
Created attachment 375590 [details]
Patch
Comment 9 Priyanka 2019-08-06 06:43:03 PDT
Created attachment 375621 [details]
Patch
Comment 10 Priyanka 2019-08-06 08:32:28 PDT
Created attachment 375623 [details]
Patch
Comment 11 Priyanka 2019-08-06 11:39:05 PDT
Created attachment 375633 [details]
Patch
Comment 12 Daniel Bates 2019-08-06 13:21:57 PDT
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.
Comment 13 Daniel Bates 2019-08-06 13:26:17 PDT
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()
Comment 14 Daniel Bates 2019-08-06 13:26:57 PDT
(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 🙂
Comment 15 Daniel Bates 2019-08-06 13:29:30 PDT
(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().
Comment 16 Priyanka 2019-08-06 13:39:59 PDT
(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.
Comment 17 Priyanka 2019-08-06 14:55:17 PDT
Created attachment 375651 [details]
Patch
Comment 18 Priyanka 2019-08-07 09:51:51 PDT
Created attachment 375705 [details]
Patch
Comment 19 WebKit Commit Bot 2019-08-07 10:24:26 PDT
Comment on attachment 375705 [details]
Patch

Clearing flags on attachment: 375705

Committed r248373: <https://trac.webkit.org/changeset/248373>
Comment 20 WebKit Commit Bot 2019-08-07 10:24:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-08-07 10:25:23 PDT
<rdar://problem/54039116>