Summary: | Allow clients to toggle a text input field between being viewable and having characters hidden while maintaining a yellow auto-filled appearance | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Priyanka <pagarwal999> | ||||||||||||||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Enhancement | CC: | a.tion.surf, bweinstein, cdumez, commit-queue, dbates, jberlin, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | Safari 12 | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Priyanka
2019-07-23 11:41:08 PDT
Created attachment 374760 [details]
Patch
Created attachment 374843 [details]
Patch
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 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. Created attachment 375585 [details]
Patch
Added tests, added invalidation logic, and shared with selectors. Created attachment 375588 [details]
Patch
Created attachment 375590 [details]
Patch
Created attachment 375621 [details]
Patch
Created attachment 375623 [details]
Patch
Created attachment 375633 [details]
Patch
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 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()
(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 🙂 (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(). (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. Created attachment 375651 [details]
Patch
Created attachment 375705 [details]
Patch
Comment on attachment 375705 [details] Patch Clearing flags on attachment: 375705 Committed r248373: <https://trac.webkit.org/changeset/248373> All reviewed patches have been landed. Closing bug. |