1635224 - Issues with maxlength validation on textarea elements

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Matt Cosentino]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:77.0) Gecko/20100101 Firefox/77.0

Steps to reproduce:

Add to a form a textarea element with the maxlength attribute set.

Actual results:

Typing past the max length is prevented, good.

Pasting past the max length is allowed, which seems to be a regression from the current stable release. I don't necessarily see this as a problem though since it displays as invalid, the checkValidity function on the form returns false, and the form can't be submitted.

At this point, reload the page. The value persists in the field, good. The checkValidity function on the form returns true and the form can be submitted, not good.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I'm ~sure this is a regression from bug 1320229.

Comment 3

[sanketh]

I don't think this bug (checkValidity returning true when value of textarea is greater than maxlength) was introduced by D71689 but D71689 certainly exposed it. I am happy to work on this, but if some else wants to pick this up, that works as well.

Comment 4

[Matt Cosentino]

It also returns true if the value is set by javascript.

Comment 5

[sanketh]

(In reply to Matt Cosentino from comment #4)

It also returns true if the value is set by javascript.

I think that is okay.

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 length of the element's API value is greater than the element's maximum allowed value length, then the element is suffering from being too long. ---4.10.18.3 Limiting user input length: the maxlength attribute

Comment 6

[sanketh]

(In reply to Matt Cosentino from comment #0)

To summarize the situation.

Pasting past the max length is allowed, which seems to be a regression from the current stable release. I don't necessarily see this as a problem though since it displays as invalid, the checkValidity function on the form returns false, and the form can't be submitted.

This is not a bug, this is a feature, see bug 1320229 for why we did this.

At this point, reload the page. The value persists in the field, good. The checkValidity function on the form returns true and the form can be submitted, not good.

I think this is a bug and needs to be investigated further.

Comment 7

[Matt Cosentino]

Oh, I see. Well in my case, I noticed the problem initially with a TinyMCE editor. So, while technically a script edit, it was really a user edit. I suppose I can just validate that myself. The issue here seems to be that the dirty flag does not persist on a page reload.

Comment 8

[sanketh]

(In reply to Matt Cosentino from comment #7)

Oh, I see. Well in my case, I noticed the problem initially with a TinyMCE editor. So, while technically a script edit, it was really a user edit. I suppose I can just validate that myself.

If you'd like, you can disable this behaviour by setting editor.truncate_user_pastes to true.

The issue here seems to be that the dirty flag does not persist on a page reload.

I suspect that this is the case as well. But I need to look into it some more to be sure.

Comment 9

[sanketh]

I think I isolated the bug: we are not preserving mLastValueChangeWasInteractive between SaveState and RestoreState. It is reset to false on RestoreState and this is causing HTMLTextAreaElement::IsTooLong to always return false. We probably need to keep track of it like we keep track of mValueChanged. We do this in HTMLTextAreaElement::Clone.

Emilio, as someone who recently touched this file, do you have any thoughts on how to approach this? From a cursory glance, we don't seem to have access to a ValueChangeKind in RestoreState.

Also, SaveState/RestoreState in HTMLInputElement.cpp seems to have the same issue and indeed one can reproduce the bug with an input type="text".

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to sanketh from comment #9)

Emilio, as someone who recently touched this file, do you have any thoughts on how to approach this? From a cursory glance, we don't seem to have access to a ValueChangeKind in RestoreState.

Changing PresState's string variant to store also whether the value change was interactive, and restoring that, seems like the most obvious way to go around this, unless that wouldn't work by some reason.

Comment 11

[sanketh]

The minlength version of this bug affects firefox76... (I haven't verified if it affects esr68 but I think it does.)

Comment 12

[sanketh]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to sanketh from comment #9)

Emilio, as someone who recently touched this file, do you have any thoughts on how to approach this? From a cursory glance, we don't seem to have access to a ValueChangeKind in RestoreState.

Changing PresState's string variant to store also whether the value change was interactive, and restoring that, seems like the most obvious way to go around this, unless that wouldn't work by some reason.

Yup, that works, thanks for the suggestion, would you like to review my diff?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Sure, I'm happy to review :)

Comment 14

[sanketh]

Attachment: Bug 1635224 - Preserve mLastValueChangeWasInteractive between SaveState and RestoreState r?emilio

Modify PresState's string variant to also store whether the last change was
interactive, and preserve that property when saving and restoring state.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f7a4e412c29
Preserve mLastValueChangeWasInteractive between SaveState and RestoreState r=emilio,masayuki

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1f7a4e412c29

Comment 17

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Sanketh, Emilio, given that this is a 77 regression should we consider an uplift to beta or is it too risky or too much of an edge case to bother? Thanks

Comment 18

[sanketh]

IMO this is too much of an edge case to bother, but I don't think it should be that hard to uplift.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9145908 [details]
Bug 1635224 - Preserve mLastValueChangeWasInteractive between SaveState and RestoreState r?emilio

Beta/Release Uplift Approval Request

• User impact if declined: Validation after refresh may be broken.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple change, though we could avoid uplifting it if needed.
• String changes made/needed: none

Comment 20

[Emilio Cobos Álvarez (:emilio)]

I tend to think we should uplift it, it's relatively simple and it should apply cleanly.

Comment 21

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9145908 [details]
Bug 1635224 - Preserve mLastValueChangeWasInteractive between SaveState and RestoreState r?emilio

Given that both the developer and the reviewer are confident with the patch, that it adds tests and that it was a recent regression, let's take it in beta, thanks.

Comment 22

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33749d7c6b4a

Comment 23

[Petruta Horea [:phorea], PTO until 8/22/25]

Verified as fixed using Firefox 77 beta 5 and latest Nightly 78.0a1 2020-05-12 under Win 10 64-bit, Ubuntu 18.04 64-bit and Mac OS X 10.13.