Bug 217156 – Text gets clobbered when assigning to input.defaultValue

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Joey Arhar]

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

[Joey Arhar]

I will upload a fix for this shortly

[Joey Arhar]

Created attachment 410187 [details]
Patch

[Alexey Proskuryakov]

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.

[Joey Arhar]

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.

[Alexey Proskuryakov]

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!

[Chris Dumez]

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

[Joey Arhar]

Created attachment 410408 [details]
Patch

[EWS Watchlist]

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

[Darin Adler]

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.

[Radar WebKit Bug Importer]

<rdar://problem/70072218>

[youenn fablet]

Also, you might need an entry in LayoutTests/imported/w3c/ChangeLog

[Joey Arhar]

Created attachment 413447 [details]
Patch

[Joey Arhar]

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.

[Darin Adler]

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?

[Joey Arhar]

Created attachment 413449 [details]
Patch

[Joey Arhar]

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

[EWS]

Committed r269528: <https://trac.webkit.org/changeset/269528>

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

[Darin Adler]

Test is failing on iOS?

[Joey Arhar]

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.

[Darin Adler]

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.

[Joey Arhar]

Reopening to attach new patch.

[Joey Arhar]

Created attachment 413492 [details]
Patch

[Joey Arhar]

I added a patch to add it to TestExpectations

[Darin Adler]

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

[Joey Arhar]

Created attachment 413493 [details]
Patch

[Joey Arhar]

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

[EWS]

Committed r269549: <https://trac.webkit.org/changeset/269549>

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