Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/45776.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
It seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408
It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
It seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Oops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David BaronIt seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Oops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
Hmm, I admit I'm not great at navigating Gecko code, but I can't seem to find how setting defaultValue avoids setting `mLastValueChangeWasInteractive`. I'm happy to follow another pattern, if I can find it. This way (just adding a local var to keep track) seemed the most straightforward. But suggestions appreciated!
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David BaronIt seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Mason FreedOops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
Hmm, I admit I'm not great at navigating Gecko code, but I can't seem to find how setting defaultValue avoids setting `mLastValueChangeWasInteractive`. I'm happy to follow another pattern, if I can find it. This way (just adding a local var to keep track) seemed the most straightforward. But suggestions appreciated!
At a minimum, I'd suggest making the change unconditionally in `ChildrenChanged()` rather than conditioning it on the boolean; I don't see the reason to make the `defaultValue` setter different from changing the child content. I think it's clear that it doesn't match WebKit but I think it's plausible (although I haven't really verified -- this is complicated) that it makes the behavior closer to Gecko's. (It's possible that the condition call to `SetLastChangeWasNotUserEdit` in `HTMLTextAreaElement::SetValueCommon` could be equivalent to what Gecko does -- but I didn't really check -- see below.)
Some previous Chrome changes here were https://chromium.googlesource.com/chromium/src/+/21d9f6214b83322e0a741b77977556c735d0b189 and https://chromium.googlesource.com/chromium/src/+/e91b6459cf18e90f2766a68a2487701610213be4 and https://chromium.googlesource.com/chromium/src/+/90b048661a3949cd38ef690052d6930c230b4100 , but I didn't dig in to the details enough to really understand what's going on and whether something there made the behavior diverge from WebKit.
The Gecko logic is that `HTMLTextAreaElement::ContentChanged` (similar to WebKit and Chromium `ChildrenChanged`) calls `HTMLTextAreaElement::Reset` which calls `HTMLTextAreaElement::SetValueInternal(..., ValueSetterOption::ByInternalAPI)` which calls `TextControlState::SetValue` which uses that option to set `changeKind` to `ValueChangeKind::Internal` which calls `HTMLTextAreaElement::OnValueChanged`, which, as a result of the `aChangeKind`, does not touch `mLastValueChangeWasInteractive`.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Also, whatever you do, you probably should have a feature flag for this.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Also, whatever you do, you probably should have a feature flag for this.
Done
David BaronIt seems a bit weird to me that the behavior for setDefaultValue should be different from the behavior for changing the child content. That doesn't appear to be the case in either Gecko or WebKit:
https://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#322
https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTextAreaElement.cpp#408It also seems like the WebKit source matches our current code rather than your modifications, so I'm curious what the source of the behavior difference is. It also seems like the Gecko code (which should go through `ContentChanged` and then `Reset`) matches our current code.
Mason FreedOops, I misread the Gecko code. It resets `mUserInteracted` but it *doesn't* reset `mLastValueChangeWasInteractive` which is the one that's relevant here. (But that holds for content changes too.)
But the webkit code looks like it resets both `m_lastChangeWasNotUserEdit` and `m_wasModifiedByUser`.
David BaronHmm, I admit I'm not great at navigating Gecko code, but I can't seem to find how setting defaultValue avoids setting `mLastValueChangeWasInteractive`. I'm happy to follow another pattern, if I can find it. This way (just adding a local var to keep track) seemed the most straightforward. But suggestions appreciated!
At a minimum, I'd suggest making the change unconditionally in `ChildrenChanged()` rather than conditioning it on the boolean; I don't see the reason to make the `defaultValue` setter different from changing the child content. I think it's clear that it doesn't match WebKit but I think it's plausible (although I haven't really verified -- this is complicated) that it makes the behavior closer to Gecko's. (It's possible that the condition call to `SetLastChangeWasNotUserEdit` in `HTMLTextAreaElement::SetValueCommon` could be equivalent to what Gecko does -- but I didn't really check -- see below.)
Some previous Chrome changes here were https://chromium.googlesource.com/chromium/src/+/21d9f6214b83322e0a741b77977556c735d0b189 and https://chromium.googlesource.com/chromium/src/+/e91b6459cf18e90f2766a68a2487701610213be4 and https://chromium.googlesource.com/chromium/src/+/90b048661a3949cd38ef690052d6930c230b4100 , but I didn't dig in to the details enough to really understand what's going on and whether something there made the behavior diverge from WebKit.
The Gecko logic is that `HTMLTextAreaElement::ContentChanged` (similar to WebKit and Chromium `ChildrenChanged`) calls `HTMLTextAreaElement::Reset` which calls `HTMLTextAreaElement::SetValueInternal(..., ValueSetterOption::ByInternalAPI)` which calls `TextControlState::SetValue` which uses that option to set `changeKind` to `ValueChangeKind::Internal` which calls `HTMLTextAreaElement::OnValueChanged`, which, as a result of the `aChangeKind`, does not touch `mLastValueChangeWasInteractive`.
Thanks for the suggestion. It turns out just unconditionally (other than the new feature flag) *not* setting `SetLastChangeWasNotUserEdit` in `ChildrenChanged()` does the trick and is quite a bit simpler.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
Commit-Queue | +2 |
LGTM. I'm still a little bit worried that the underlying mechanism here may differ from other browsers, but at this point I've gotten myself confused enough reading the code across three engines, and this does align us with other engines on the tests you've added (based on https://wpt.fyi/results/html/semantics/forms/constraints/form-validation-validity-textarea-defaultValue.html?label=pr_head&max-count=1&pr=45776 ).
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Make setting defaultValue from a trusted input event validate
This adds a large-ish test of the validation behavior of the
textarea element, which as far as I can tell is under-specified,
or at least I could not find this behavior in the spec. I looked
mainly here:
https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#validity-states
In particular, there are some interesting differences in the
validity of initially-empty vs initially-non-empty textareas.
This CL, though, also aligns the behavior of Chromium with both
WebKit and Gecko in the specific case where the `input` event
handler sets textarea.defaultValue. In that case, the textarea
will be validated correctly. Previous to this CL, Chromium would
not validate the textarea as a result of the change.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/45776
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |