Bug 233239 - When an "autofilled and viewable" field becomes empty, turn "autofilled and viewable" off
Summary: When an "autofilled and viewable" field becomes empty, turn "autofilled and v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ricky Mondello
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-16 22:22 PST by Ricky Mondello
Modified: 2021-11-17 22:27 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.54 KB, patch)
2021-11-16 22:26 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Patch (1.51 KB, patch)
2021-11-16 22:30 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Patch (1.45 KB, patch)
2021-11-17 14:13 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Patch (6.82 KB, patch)
2021-11-17 14:53 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Patch (8.51 KB, patch)
2021-11-17 16:20 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff
Patch (5.43 KB, patch)
2021-11-17 16:58 PST, Ricky Mondello
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ricky Mondello 2021-11-16 22:22:20 PST
When a “autofilled and viewable” field becomes empty, turn “autofilled and viewable” off
Comment 1 Ricky Mondello 2021-11-16 22:26:56 PST
Created attachment 444479 [details]
Patch
Comment 2 Ricky Mondello 2021-11-16 22:30:48 PST
Created attachment 444481 [details]
Patch
Comment 3 David Quesada 2021-11-17 11:49:46 PST
Comment on attachment 444481 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        When an âautofilled and viewableâ field becomes empty, turn âautofilled and viewableâ off

I think these smart quotes are *too smart* for the ChangeLog.
Comment 4 Darin Adler 2021-11-17 12:34:26 PST
Comment on attachment 444481 [details]
Patch

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

We should add a regression test, which is required for bug fixes in WebKit; I believe we have this wired up so we can make such tests

> Source/WebCore/html/HTMLInputElement.cpp:1165
> +    // Clear AutoFilled and viewable on field becomming empty.

This comment doesn’t fit WebKit’s coding style. Comments should answer the question "why" rather than just saying the same thing the code does below. The comment above has a similar problem. Please consider rewording so this does something other than just saying in English what the code just below it does.

Also, misspelling of the word "becoming".
Comment 5 Ricky Mondello 2021-11-17 14:13:39 PST
Created attachment 444572 [details]
Patch
Comment 6 Ricky Mondello 2021-11-17 14:15:12 PST
Fixed up the bug title and the comment.

Looking to see how to test.
Comment 7 Ricky Mondello 2021-11-17 14:53:51 PST
Created attachment 444584 [details]
Patch
Comment 8 Ricky Mondello 2021-11-17 16:20:14 PST
Created attachment 444595 [details]
Patch
Comment 9 Ricky Mondello 2021-11-17 16:21:12 PST
Ready for review, with a passing test. :)
Comment 10 Darin Adler 2021-11-17 16:35:07 PST
Comment on attachment 444595 [details]
Patch

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

I did not set commit-queue+ yet since you may want to consider a different test, which could change the patch further. But you are also welcome to land this and then refine from here.

> Source/WebCore/testing/Internals.idl:413
> +    undefined setValueFromRenderer(HTMLInputElement inputElement, DOMString text);

I am really happy we added a test, but this should new function should not have been required.

We should be able to construct a test that changes the value by selecting the text and then doing a backspace with eventSender.keyDown('\u0008'), or something roughly like that, which would do more end-to-end testing and not require any change to internals.
Comment 11 Ricky Mondello 2021-11-17 16:37:33 PST
Comment on attachment 444595 [details]
Patch

Wenson pointed out a better way to do the test.
Comment 12 Ricky Mondello 2021-11-17 16:58:38 PST
Created attachment 444610 [details]
Patch
Comment 13 Ricky Mondello 2021-11-17 16:59:01 PST
Comment on attachment 444610 [details]
Patch

Okay, one more time!
Comment 14 Darin Adler 2021-11-17 17:16:19 PST
Comment on attachment 444610 [details]
Patch

EWS hasn’t run enough for me to say commit-queue+
Comment 15 Wenson Hsieh 2021-11-17 22:05:39 PST
Comment on attachment 444610 [details]
Patch

The Windows test failure (storage/indexeddb/IDBKey-create-array-buffer-view-oom.html) is presumably unrelated.
Comment 16 EWS 2021-11-17 22:26:57 PST
Committed r285982 (244379@main): <https://commits.webkit.org/244379@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444610 [details].
Comment 17 Radar WebKit Bug Importer 2021-11-17 22:27:27 PST
<rdar://problem/85535686>