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

RESOLVED FIXED in Firefox 51



DOM: Core & HTML
8 years ago
2 years ago


(Reporter: mounir, Assigned: Thomas Wisniewski)


Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)



(1 attachment, 2 obsolete attachments)



8 years ago
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

3 years ago
I would like to

Comment 2

3 years ago
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

3 years ago
Note that this is confusingly NOT the "dirty value flag" ( ) from the HTML5 spec.

Comment 4

2 years ago
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:

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:

Flags: needinfo?(annevk)

Comment 5

2 years ago
minlength/maxlength require user interaction. Per (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

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

Comment 8

2 years ago
Thanks! I'll try to get this ticket done.
Assignee: nobody → wisniewskit
Flags: needinfo?(annevk)


2 years ago
Depends on: 1290283

Comment 9

2 years ago
Created attachment 8776099 [details] [diff] [review]

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:

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).
Attachment #8776099 - Flags: review?(mrbkap)
Comment on attachment 8776099 [details] [diff] [review]

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.
Attachment #8776099 - Flags: review?(mrbkap) → review+

Comment 11

2 years ago
Created attachment 8776237 [details] [diff] [review]

Alright, here's an updated patch that does that. Carrying over r+.
Attachment #8776099 - Attachment is obsolete: true

Comment 12

2 years ago
Created attachment 8778596 [details] [diff] [review]

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

Try seems fine:

Carrying over r+ and requesting check-in.
Attachment #8776237 - Attachment is obsolete: true


2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by
Track whether the last input/textarea change was done interactively, and enable the commented-out maxLength tracking code. r=mrbkap
Keywords: checkin-needed

Comment 14

2 years ago
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51


2 years ago
See Also: → bug 1203844
You need to log in before you can comment on or make changes to this bug.