1320229 - Allow the paste into <input> and <textarea> without truncation if the paste is longer than "maxlength"

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

I have had multiple times when using my password manager where there is an unwritten, e.g. 20 character limit on the password field through a max length attribute. This has caused me to incorrectly set my password when I paste it in, as it is truncated in a way which I did not expect.

It would be nice if we could warn if something like that were to happen.

Comment 1

[Matthew N. [:MattN]]

This was discussed recently on chat and my suggestion was that we don't have to limit the user input to maxLength in all cases, the HTML spec seems to say:

User agents may prevent the user from causing the element's API value to be set to a value whose length is greater than the element's maximum allowed value length.

Perhaps we could rely on constraint validation mechanisms to warn the user about this in the common case?

Comment 2

[sanketh]

The spec talks generally about inputs, so would we make this change for all input types or just type=password?

If we make this change for all inputs, it seems to break some existing tests like test_bug603556.

Comment 3

[Matthew N. [:MattN]]

(In reply to sanketh from comment #2)

The spec talks generally about inputs, so would we make this change for all input types or just type=password?

I'm not sure which is most web compatible.

If we make this change for all inputs, it seems to break some existing tests like test_bug603556.

It's not surprising to have tests that ensure we don't unintentionally regress current behaviour… that doesn't mean we can't intentionally change it though. This is a mochitest, not a WPT.

Comment 4

[sanketh]

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #3)

It's not surprising to have tests that ensure we don't unintentionally regress current behaviour… that doesn't mean we can't intentionally change it though. This is a mochitest, not a WPT.

Good point. I did a quick skim through the web-platform-tests/wpt repo and this change does not seem to break anything.

So, is the plan now to wait for the owners of the component to pitch in and say whether they want this change?

Comment 5

[Matthew N. [:MattN]]

Are you interested in writing a patch? If so, I think you should propose a solution to an editor peer/owner and see if it's acceptable.

Comment 6

[sanketh]

I would be interested in writing a patch, but I have not contributed to Firefox before and might require a bunch of hand-holding.

What is the expected way to get in touch with the editor peer/owner? I cannot find a page for the editor component on MozillaWiki nor can I find an editor channel on Matrix. The link you shared on chat https://wiki.mozilla.org/Modules/All#Editor seems to list emails, so old-school email?

Comment 7

[Matthew N. [:MattN]]

I would recommend using the "Request information from" feature that appears at the bottom of the bug and then you put in one of their email addresses or nicknames and write a comment with your proposal.

Comment 8

[sanketh]

I didn't notice that, thanks for the pointer!

Comment 9

[sanketh]

Makoto: what do you think about Matt's suggestion?

The bug: if you paste text longer than maxlength of the input, the paste is silently truncated to the max value. This behaviour is succinctly illustrated by test_bug603556. This causes an issue when pasting a password from a password manager as the reporter pointed out.

Proposed fix: If the paste is longer maxlength, allow the paste without truncation and let the form validator (which implements the constraint validation api) handle it like it handles minlength.

We were wondering if this fix was compatible with the spec (see comment 2) and further if the editor team would accept a patch?

Thanks!!

Comment 10

[sanketh]

I tried to write a patch for the proposed fix to get a feel for how this works. My strawman solution, which seems to work, is to remove EditActionResult TextEditor::TruncateInsertionStringForMaxLength and all references to it. This removes maxlength truncation while inserting a character or while inserting a newline (i.e., all the time.) This seems a little stronger than the proposed fix which only talks about pastes. I wonder if this is acceptable? A cursory reading of the spec doesn't seem to obviously conflict with it.

Comment 11

[Makoto Kato [:m_kato]]

Nakano-san is better than me.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If validator always stops submitting the form, I think that it seems that the suggested approach is reasonable to me. On the other hand, some web apps may depend on maxlength behavior at setting value or something. So, the new behavior should be used only for user input.

Perhaps, when EditorBase::GetEditAction() returns EditAction::ePaste, EditAction::ePasteAsQuotation, EditAction::eDrop or EditAction::eReplaceText and EditorBase::GetEditActionPrincipal() returns nullptr or principal of addons.

But be aware about these points:

• this won't work with <input>.value setting case from addon because the EditAction is EditAction::eSetText, and TextControlState::SetValueWithTextEditor() does not set proper principal. If you need to support this case, you need to change all its and its ancestor callers take the principal.
• you might need to change TextControlState::SetValueWithoutTextEditor() for EditAction::eReplaceText case because we won't create TextEditor instance until it's actually required because of performance reason. But I don't find the truncating code of this case. Investigate this case.
• you shouldn't run validation while IME composition for avoiding flickering the validate result.
• for risk management, please make the new path can be enabled/disabled by pref.

Comment 13

[sanketh]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

Thanks so much for your comments!! I will take a closer look at the code, investigate the cases you suggested, and generally start writing a patch.

• this won't work with <input>.value setting case from addon because the EditAction is EditAction::eSetText, and TextControlState::SetValueWithTextEditor() does not set proper principal. If you need to support this case, you need to change all its and its ancestor callers take the principal.

I don't think we wish to support <input>.value, at least not at this point, because it does not impact this use-case (pasting a password.)

Comment 14

[sanketh]

Attachment: interplay between setUserInput and maxlength

Comment 15

[sanketh]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

• you might need to change TextControlState::SetValueWithoutTextEditor() for EditAction::eReplaceText case because we won't create TextEditor instance until it's actually required because of performance reason. But I don't find the truncating code of this case. Investigate this case.

I can confirm your observation using a modification of test_MozEditableElement_setUserInput.html attached above: it doesn't seem like maxlength affects the output of SetUserInput. So, for now, I am not going to touch TextControlState::SetValueWithoutTextEditor().

Comment 16

[sanketh]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

• you shouldn't run validation while IME composition for avoiding flickering the validate result.

You make a great point. If I am dealing with composed strings, then my patch seems to work as expected, here is a video of me playing around with Japanese IME. But, if I am working with not-fully-composed strings, then I am able to cause a tab crash---here is an example where it crashes with

Assertion failure: !mIsComposing, at /home/sanketh/Projects/mozilla-central/widget/TextEventDispatcher.cpp:141

I am not sure what the expected behavior should be in this case, or even if my IME setup is correct.

Would it make sense for me to push my current patch to phabricator so you can, if you wish, download and reproduce the behavior?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to sanketh from comment #15)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

• you might need to change TextControlState::SetValueWithoutTextEditor() for EditAction::eReplaceText case because we won't create TextEditor instance until it's actually required because of performance reason. But I don't find the truncating code of this case. Investigate this case.

I can confirm your observation using a modification of test_MozEditableElement_setUserInput.html attached above: it doesn't seem like maxlength affects the output of SetUserInput. So, for now, I am not going to touch TextControlState::SetValueWithoutTextEditor().

Oh, that must be a bug because I've never seen we intentionally ignore maxlength in that case in inline comment. (Anyway, out of scope of this bug.)

(In reply to sanketh from comment #16)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

• you shouldn't run validation while IME composition for avoiding flickering the validate result.

You make a great point. If I am dealing with composed strings, then my patch seems to work as expected, here is a video of me playing around with Japanese IME. But, if I am working with not-fully-composed strings, then I am able to cause a tab crash---here is an example where it crashes with

Assertion failure: !mIsComposing, at /home/sanketh/Projects/mozilla-central/widget/TextEventDispatcher.cpp:141

I am not sure what the expected behavior should be in this case, or even if my IME setup is correct.

Hmm... You must hit an existing bug... I guess that you commit composition in <textarea> with mouse click, but the state is not correctly cleaned up. Then, you type new composition string in <input> but due to the bug, TextEventDispatcher in the content process receives unexpected compositionstart even though it still have old composition.

Would it make sense for me to push my current patch to phabricator so you can, if you wish, download and reproduce the behavior?

Well, yes, I'd like to check your patch, but I feel not good today, so, perhaps, my reply will be delayed. Sorry for that.

Comment 18

[sanketh]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick, wait for a couple of days for ni?) from comment #17)

Would it make sense for me to push my current patch to phabricator so you can, if you wish, download and reproduce the behavior?

Well, yes, I'd like to check your patch, but I feel not good today, so, perhaps, my reply will be delayed. Sorry for that.

Sounds good. I'll try to get it out soon. No worries, take care!

Comment 19

[sanketh]

Attachment: Bug 1320229 - allow user pastes longer than input maxlength r?masayuki

Currently, all input (including user pastes) to an input field is truncated to
maxlength. This diff disables truncation for user pastes.

When (1) GetEditAction is ePaste, ePasteAsQuotation, eDrop, or
eReplaceText (ie we are dealing with a paste) and (2) GetEditActionPrincipal
is nullptr (ie we are dealing with a user edit and not a JS edit), allow a
paste without truncation. That means that, in this case, we will return
EditActionIgnored in place of TruncateInsertionStringForMaxLength.

This behavior is controlled by a new preference editor.truncate_user_pastes
which is false by default (set in all.js).

We also modify editor/libeditor/tests/test_bug603556.html which currently
expects the output of a paste longer than maxlength to be truncated.

Created editor/libeditor/tests/test_bug1320229-1.html which checks if a user
can paste a password longer than maxlength and if the field is then marked as
tooLong (this was the original concern of the reporter of Bug 1320229), and
editor/libeditor/tests/test_bug1320229-2.html which checks if eReplaceText
has consistent behavior regardless of whether the field has an associated editor
(this test works by calling setUserInput() before and after the element gets
focus.) ./mach test editor/libeditor tests pass.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31503d35be56
allow user pastes longer than input maxlength r=masayuki

Comment 21

[sanketh]

Thanks MattN and Masayuki for tolerating my stupid questions and helping me write an acceptable patch.

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/31503d35be56

Comment 23

[Matthew N. [:MattN]]

(In reply to sanketh from comment #21)

Thanks MattN and Masayuki for tolerating my stupid questions and helping me write an acceptable patch.

No worries at all. Glad you managed to write a patch :)

Timea, could this be verified with the Fx77 password manager work? Looks like we already have a regression: bug 1635224

Comment 24

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest Nightly 77.0a1 (2020-05-04) on Windows 10 and MacOS 10.13.
The field will get a red highlight after pasting in a string that is longer than the maxlength. The warning highlight is also dismissed if the user shortens the length to the maximum value.
Can also confirm the regression Bug 1635224.

Comment 25

[Timea Cernea [:tbabos][inactive]]

Attachment: Expected Result

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that this should be mentioned in release note and some documents for developers.

Comment 27

[Kohei Yoshino [:kohei]]

Thanks Nakano-san for the heads-up! I just posted a site compatibility note for this. Let me know if it needs any correction.

Comment 28

[Kohei Yoshino [:kohei]]

And here are people’s reactions: https://news.ycombinator.com/item?id=23205218

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Kohei Yoshino [:kohei] from comment #27)

Thanks Nakano-san for the heads-up! I just posted a site compatibility note for this. Let me know if it needs any correction.

Thank you for your quick work! It looks fine to me.

(In reply to Kohei Yoshino [:kohei] from comment #28)

And here are people’s reactions: https://news.ycombinator.com/item?id=23205218

Well, some comments are written with misunderstood of this change... It shouldn't cause any behavior change from point of web apps view.

Comment 30

[Pascal Chevrel:pascalc]

Added to 77 beta relnotes with this description:

Text exceeding maxlength will no longer be truncated when pasted into <input> or <textarea>
pointing to https://www.fxsitecompat.dev/en-CA/docs/2020/text-exceeding-maxlength-will-no-longer-be-truncated-when-pasted-into-input-or-textarea/

I'll check witht the marketing folks to see if we include it in the final release notes for 77.

Comment 31

[Thorsten Dencker]

Hi all,

could you please check the bug 1636855 https://bugzilla.mozilla.org/show_bug.cgi?id=1636855

This change is a showstopper for SAP customers as in certain scenarios the customer/user will loose the entered data. Could you please revert this incompatible change again and maybe you can find a non breaking solution. The maxlength attribute exists for ages unchanged and this is an incompatible change only done by Firefox.

Even the warning border doesn't help due to sever reasons:

• most probably it violates all accessibility standards like WCAG. Visual impaired users (e.g. color blind or complet blind users) will not see the red border.
• all users will not get any information why the field is getting red.
• fields with red border --> you will not see any differences
• red border occurs on focus out --> press enter to send the data

Please revert the change as the existing implementation breaks our scenarios without any possibility for Enterprise customers to revert the change.

Thanks Thorsten Dencker

Comment 32

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Reflecting the summary title.