Bug 233239 – When an "autofilled and viewable" field becomes empty, turn "autofilled and viewable" off

• Component: Forms
• Status: RESOLVED FIXED

[Ricky Mondello]

When a “autofilled and viewable” field becomes empty, turn “autofilled and viewable” off

[Ricky Mondello]

Created attachment 444479 [details]
Patch

[Ricky Mondello]

Created attachment 444481 [details]
Patch

[David Quesada]

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.

[Darin Adler]

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".

[Ricky Mondello]

Created attachment 444572 [details]
Patch

[Ricky Mondello]

Fixed up the bug title and the comment.

Looking to see how to test.

[Ricky Mondello]

Created attachment 444584 [details]
Patch

[Ricky Mondello]

Created attachment 444595 [details]
Patch

[Ricky Mondello]

Ready for review, with a passing test. :)

[Darin Adler]

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.

[Ricky Mondello]

Comment on attachment 444595 [details]
Patch

Wenson pointed out a better way to do the test.

[Ricky Mondello]

Created attachment 444610 [details]
Patch

[Ricky Mondello]

Comment on attachment 444610 [details]
Patch

Okay, one more time!

[Darin Adler]

Comment on attachment 444610 [details]
Patch

EWS hasn’t run enough for me to say commit-queue+

[Wenson Hsieh]

Comment on attachment 444610 [details]
Patch

The Windows test failure (storage/indexeddb/IDBKey-create-array-buffer-view-oom.html) is presumably unrelated.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/85535686>