Bug 217156

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