613019 - Introduce a new "dirty flag" that reflect if the user has changed the value (for input and textarea)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Currently, the dirty flag is set to true if .value="" is called (but not if .setAttribute("value", "") is called. We need a flag that is only set to true if the user has changed the value and maybe revert to false if .value="" is called (for retro-compatibility?).

Comment 1

[Rohan Jaswal]

I would like to

Comment 2

[Rohan Jaswal]

I would like to work on this. Can you please give me the links to files associated with this bug?
I am a newbie.

Comment 3

[Chris Rebert]

Note that this is confusingly NOT the "dirty value flag" (https://html.spec.whatwg.org/multipage/forms.html#concept-input-value-dirty-flag ) from the HTML5 spec.

Comment 4

[Thomas Wisniewski]

Anne, there seems to be an contradiction in the final test for the minLength and maxLength web platform tests. Could you confirm that I'm correct that the maxLength test is currently wrong?

Links to the tests here:
- http://w3c-test.org/html/semantics/forms/constraints/form-validation-validity-tooLong.html
- http://w3c-test.org/html/semantics/forms/constraints/form-validation-validity-tooShort.html

Note that the source code for the final maxLength test has a comment stating that validity.tooLong should be "false due to lack of required interactive editing by the user", despite the dirty flag being true.

However, the equivalent final test for minLength says validity.tooShort should be true, with no mention of the interactivity requirement.

Based on the spec the maxLength test is wrong about interactivity being required, because min/maxLength are "controlled" by the element's dirty value flag, which is set to true not just with user-interaction, but also when the .value of the element is change programmatically:

- https://html.spec.whatwg.org/multipage/forms.html#the-maxlength-and-minlength-attributes
- https://html.spec.whatwg.org/multipage/forms.html#concept-input-value-dirty-flag

Comment 5

[Chris Rebert]

minlength/maxlength require user interaction. Per https://html.spec.whatwg.org/multipage/forms.html#attr-fe-maxlength (emphasis added):
> A form control maxlength attribute, [...]
> 
> Constraint validation: If an element has a maximum allowed value length,
> its dirty value flag is true,
> ***its value was last changed by a user edit*** (as opposed to a change made by a script),
> *and* the code-unit length of the element's value is greater than the element's maximum allowed value length,
> then the element is suffering from being too long.

Comment 6

[Thomas Wisniewski]

Then that would imply that the final "tooShort" test is wrong, and should also be expecting false, correct?

Comment 7

[Chris Rebert]

Correct. See https://github.com/w3c/web-platform-tests/issues/2214

Comment 8

[Thomas Wisniewski]

Thanks! I'll try to get this ticket done.

Comment 9

[Thomas Wisniewski]

Attachment: 613019-track-interactive-changes-to-input-and-textarea.diff

Here's a patch which adds a "the last value change was made interactively" flag, to best match the spec. I also activated the UpdateTooLongValidityState() methods to ensure that the change is working as it should. Try seems clear: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8f1ac16f9b9&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=24801207

I haven't yet figured out how to test this (ie, if there's a SpecialPowers API that would let me mimic a user interactively editing an input/textarea), but I figured I might as well post the patch for review while I work on that, since it does seem to be working fine based on manual testing (for instance it resolves bug 1203844).

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8776099 [details] [diff] [review]
613019-track-interactive-changes-to-input-and-textarea.diff

Review of attachment 8776099 [details] [diff] [review]:
-----------------------------------------------------------------

I have a couple of comments about the code style (nothing substantive). r=me with my comments addressed.

::: dom/html/HTMLInputElement.h
@@ +228,5 @@
>    NS_IMETHOD_(Element*) GetPlaceholderNode() override;
>    NS_IMETHOD_(void) UpdatePlaceholderVisibility(bool aNotify) override;
>    NS_IMETHOD_(bool) GetPlaceholderVisibility() override;
>    NS_IMETHOD_(void) InitializeKeyboardEventListeners() override;
> +  NS_IMETHOD_(void) OnValueChanged(bool aNotify, bool aWasInteractiveUserChange=false) override;

Ditto here.

::: dom/html/nsITextControlElement.h
@@ +168,5 @@
>  
>    /**
>     * Callback called whenever the value is changed.
>     */
> +  NS_IMETHOD_(void) OnValueChanged(bool aNotify, bool aWasInteractiveUserChange=false) = 0;

I don't think it makes sense to use a default value here, there aren't that many existing callsites and it's IMO better to be explicit.

::: dom/html/nsTextEditorState.cpp
@@ +975,5 @@
>      frame->SetValueChanged(true);
>    }
>  
>    if (!mSettingValue) {
> +    mTxtCtrlElement->OnValueChanged(true, true); // interactive change by user

When there are lists of bool (or int) arguments, it's nice to format the code as:

mTxtCtrlElement->OnValueChanged(/* aNotify = */ true, /* aInteractive = */ true);

so as not to confuse which is which.

Comment 11

[Thomas Wisniewski]

Attachment: 613019-track-interactive-changes-to-input-and-textarea.diff

Alright, here's an updated patch that does that. Carrying over r+.

Comment 12

[Thomas Wisniewski]

Attachment: 613019-track-interactive-changes-to-input-and-textarea.diff

Rebased and added a mochitest (based on the test-case in bug 1203844).

Try seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bea59b310bc

Carrying over r+ and requesting check-in.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bb8f329f10
Track whether the last input/textarea change was done interactively, and enable the commented-out maxLength tracking code. r=mrbkap

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2bb8f329f10