Bug 217156 - Text gets clobbered when assigning to input.defaultValue
Summary: Text gets clobbered when assigning to input.defaultValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-30 17:18 PDT by Joey Arhar
Modified: 2020-11-06 16:21 PST (History)
14 users (show)

See Also:


Attachments
Patch (1.80 KB, patch)
2020-09-30 18:12 PDT, Joey Arhar
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2020-10-02 19:53 PDT, Joey Arhar
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2020-11-06 11:09 PST, Joey Arhar
no flags Details | Formatted Diff | Diff
Patch (3.14 KB, patch)
2020-11-06 11:19 PST, Joey Arhar
no flags Details | Formatted Diff | Diff
Patch (1.30 KB, patch)
2020-11-06 15:30 PST, Joey Arhar
no flags Details | Formatted Diff | Diff
Patch (1.28 KB, patch)
2020-11-06 15:38 PST, Joey Arhar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joey Arhar 2020-09-30 17:18:56 PDT
When script assigns to the defaultValue property of a number or email input, it may cause text which the user entered into the input to be removed.
In both input types, leading and trailing whitespace gets removed.
In number inputs, trailing '.' and 'e' characters also get removed.

This causes bugs in React where text is changed while the user is typing since React assigns to the defaultValue property:
https://github.com/facebook/react/issues/14551
https://github.com/facebook/react/issues/15418
https://github.com/facebook/react/issues/14168
https://github.com/facebook/react/issues/14356

The relationship between those react issues and this bug is discussed more here: 
https://github.com/whatwg/html/issues/5257#issuecomment-607548564

This has already been fixed in chrome and is now in the stable release: http://crrev.com/777263

This bug is tested by this WPT: https://wpt.fyi/results/html/semantics/forms/the-input-element/defaultValue-clobbering.html
Comment 1 Joey Arhar 2020-09-30 17:19:45 PDT
I will upload a fix for this shortly
Comment 2 Joey Arhar 2020-09-30 18:12:47 PDT
Created attachment 410187 [details]
Patch
Comment 3 Alexey Proskuryakov 2020-10-01 13:18:22 PDT
Comment on attachment 410187 [details]
Patch

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

Mechanical r- because we need to have a regression test that's checked in to the repository to go from failing to passing state.

> Source/WebCore/ChangeLog:13
> +        https://wpt.fyi/results/html/semantics/forms/the-input-element/defaultValue-clobbering.html

Looks like we have this test imported: https://trac.webkit.org/browser/webkit/trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/defaultValue-clobbering.html

But it is already passing: https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fsemantics%2Fforms%2Fthe-input-element%2FdefaultValue-clobbering.html

This is a reftest, so not sure what this means in the context of this fix.
Comment 4 Joey Arhar 2020-10-01 13:22:50 PDT
Thanks for the review Alexey!

I changed the WPT to no longer be a reftest in this change 23 days ago: https://github.com/web-platform-tests/wpt/commit/fe587e39a8e7ed68eed5f489b64c81533ff47a4f

Do you think that this change hasn't been imported to WebKit yet? On the trac.webkit.org link you sent me it looks like it's still the old one.
Comment 5 Alexey Proskuryakov 2020-10-01 15:26:51 PDT
I see. Re-importing the test as part of this patch would address the concern. 

Ideally, please confirm that the reimported test fails in WebKit test harness (run-webkit-tests) without the fix. Thank you!
Comment 6 Chris Dumez 2020-10-01 15:30:23 PDT
(In reply to Joey Arhar from comment #4)
> Thanks for the review Alexey!
> 
> I changed the WPT to no longer be a reftest in this change 23 days ago:
> https://github.com/web-platform-tests/wpt/commit/
> fe587e39a8e7ed68eed5f489b64c81533ff47a4f
> 
> Do you think that this change hasn't been imported to WebKit yet? On the
> trac.webkit.org link you sent me it looks like it's still the old one.

WPT resync in WebKit are not automatic and not frequent so it is extremely likely that change was not sync'd up yet. As Alexey said, I suggest importing the latest version of the WPT test as part of your patch.
Comment 7 Joey Arhar 2020-10-02 19:53:42 PDT
Created attachment 410408 [details]
Patch
Comment 8 EWS Watchlist 2020-10-02 19:54:36 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 9 Darin Adler 2020-10-03 14:12:52 PDT
Comment on attachment 410408 [details]
Patch

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

The code change is great, but this does not correctly import the updated web-platform-tests test. We will need an expected.txt file if the type of this test changes.

> Source/WebCore/ChangeLog:10
> +        of a number or email input causes the text the user entered into the
> +        input in some situations.

Missing word here. This says "causes the text ... in some situations"; I think you mean causes the text to be clobbered or overwritten.
Comment 10 Radar WebKit Bug Importer 2020-10-07 17:19:15 PDT
<rdar://problem/70072218>
Comment 11 youenn fablet 2020-10-08 04:54:21 PDT
Also, you might need an entry in LayoutTests/imported/w3c/ChangeLog
Comment 12 Joey Arhar 2020-11-06 11:09:39 PST
Created attachment 413447 [details]
Patch
Comment 13 Joey Arhar 2020-11-06 11:11:20 PST
Sorry for the delay.
I found that the WPT test_driver.send_keys() wasn't able to put characters into the input element, so I made a layout test with eventSender instead.
Comment 14 Darin Adler 2020-11-06 11:16:23 PST
Comment on attachment 413447 [details]
Patch

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

> LayoutTests/fast/forms/defaultValue-clobbering.html:12
> +shouldBeFalse('numberinput.validity.valid');

Do we also want to check the value, not just the validity, of the input element here?
Comment 15 Joey Arhar 2020-11-06 11:19:25 PST
Created attachment 413449 [details]
Patch
Comment 16 Joey Arhar 2020-11-06 11:20:08 PST
Comment on attachment 413447 [details]
Patch

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

>> LayoutTests/fast/forms/defaultValue-clobbering.html:12
>> +shouldBeFalse('numberinput.validity.valid');
> 
> Do we also want to check the value, not just the validity, of the input element here?

I added checks for numberinput.value
Comment 17 EWS 2020-11-06 12:17:25 PST
Committed r269528: <https://trac.webkit.org/changeset/269528>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413449 [details].
Comment 18 Darin Adler 2020-11-06 15:05:06 PST
Test is failing on iOS?
Comment 19 Joey Arhar 2020-11-06 15:21:45 PST
https://ews-build.s3-us-west-2.amazonaws.com/iOS-14-Simulator-WK2-Tests-EWS/r413449-1362/fast/forms/defaultValue-clobbering-pretty-diff.html

Hm yeah it looks like eventSender wasn't able to put text into the input element on ios...
I'll look into it but it will take a bit since I was developing on linux, and I don't really know why this would happen.
Comment 20 Darin Adler 2020-11-06 15:25:30 PST
Sure. Multi-platform development is difficult.

In the short term we need to either revert or add something to TestExpectations to skip that test, because a failing test will slow down our continuous integration machinery as it keeps retrying to see if the test will pass.
Comment 21 Joey Arhar 2020-11-06 15:30:12 PST
Reopening to attach new patch.
Comment 22 Joey Arhar 2020-11-06 15:30:13 PST
Created attachment 413492 [details]
Patch
Comment 23 Joey Arhar 2020-11-06 15:30:56 PST
I added a patch to add it to TestExpectations
Comment 24 Darin Adler 2020-11-06 15:35:07 PST
Comment on attachment 413492 [details]
Patch

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

> LayoutTests/TestExpectations:4647
> +# Fails on iOS: https://bugs.webkit.org/show_bug.cgi?id=217156
> +fast/forms/defaultValue-clobbering.html [ Failure ]

We should instead put this into LayoutTests/platform/ios/TestExpectations
Comment 25 Joey Arhar 2020-11-06 15:38:00 PST
Created attachment 413493 [details]
Patch
Comment 26 Joey Arhar 2020-11-06 15:38:46 PST
Comment on attachment 413492 [details]
Patch

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

>> LayoutTests/TestExpectations:4647
>> +fast/forms/defaultValue-clobbering.html [ Failure ]
> 
> We should instead put this into LayoutTests/platform/ios/TestExpectations

Thanks, I was wondering if there was a way to only mark it as failing on iOS
Comment 27 EWS 2020-11-06 16:21:21 PST
Committed r269549: <https://trac.webkit.org/changeset/269549>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413493 [details].