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
I will upload a fix for this shortly
Created attachment 410187 [details] Patch
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.
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.
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!
(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.
Created attachment 410408 [details] Patch
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 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.
<rdar://problem/70072218>
Also, you might need an entry in LayoutTests/imported/w3c/ChangeLog
Created attachment 413447 [details] Patch
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 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?
Created attachment 413449 [details] Patch
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
Committed r269528: <https://trac.webkit.org/changeset/269528> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413449 [details].
Test is failing on iOS?
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.
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.
Reopening to attach new patch.
Created attachment 413492 [details] Patch
I added a patch to add it to TestExpectations
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
Created attachment 413493 [details] Patch
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
Committed r269549: <https://trac.webkit.org/changeset/269549> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413493 [details].