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

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: mounir, Assigned: Thomas Wisniewski)

Tracking

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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" (https://html.spec.whatwg.org/multipage/forms.html#concept-input-value-dirty-flag ) from the HTML5 spec.
(Assignee)

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:
- 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
Flags: needinfo?(annevk)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

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

Comment 8

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

Updated

2 years ago
Depends on: 1290283
(Assignee)

Comment 9

2 years ago
Created attachment 8776099 [details] [diff] [review]
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).
Attachment #8776099 - Flags: review?(mrbkap)
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.
Attachment #8776099 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8776237 [details] [diff] [review]
613019-track-interactive-changes-to-input-and-textarea.diff

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

Comment 12

2 years ago
Created attachment 8778596 [details] [diff] [review]
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.
Attachment #8776237 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2bb8f329f10
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

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