If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

More than MAXLENGTH characters allowed in a text field after removing selection

RESOLVED FIXED

Status

()

Core
Selection
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: Christophe Bliard, Assigned: mats)

Tracking

({testcase})

Trunk
x86
All
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3.1) Gecko/20030527 Debian/1.3.1-2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3.1) Gecko/20030527 Debian/1.3.1-2

In any form containing a <INPUT TYPE="TEXT" MAXLENGTH="x">, you can type more
than x characters

Reproducible: Always

Steps to Reproduce:
- Filling the text field with x characters,
- Pressing Tab (go to the next control),
- then Shift-Tab (come back to the text control),
- press the right arrow (go the end of the text control),
- type a character (!)
Actual Results:  
there is x+1 characters in a text field limited to x characters

Expected Results:  
nothing ;)

Comment 1

15 years ago
The URL given WFM 20030527 PC/WinXP

Comment 2

15 years ago
Confirming on WinXP 2003052808. Perhaps related to bug 207094 or bug 204506?
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

15 years ago
Created attachment 125259 [details]
testcase

Updated

15 years ago
Keywords: testcase
OS: Linux → All
Summary: can type more than MAXLENGTH characters in a form with a text field → More than MAXLENGTH characters allowed in a text field when tabbing between fields
This is a core editor bug, as far as I can tell...  The code in
nsTextEditRules::TruncateInsertionIfNeeded gets the selection offsets; given the
steps to reproduce in this bug those come out to be 0 and 5 instead of 5 and 5
as expected.  As a result, the amount of truncation that needs to happen is
miscalculated.

The reason nsPlaintextEditor::GetTextSelectionOffsets returns the wrong value is
that the selection range seems to be on some weird node that's NOT the main
textnode for that editor.  As a result, the if statements in the loop of
GetAbsoluteOffsetsForPoints are never triggered, and so the 0,-1 start/end
values are what gets used.

Perhaps this code should default to returning 0,0 if the start/end nodes weren't
even found?  Or perhaps the issue with them not being found needs to be resolved?
Assignee: form → mozeditor
Blocks: 204506
Component: Layout: Form Controls → Editor: Core
QA Contact: desale → sairuh

Comment 5

13 years ago
Created attachment 155190 [details]
Simpler testcase

Steps to reproduce the bug:
1. Click on the input field
2. Enter the string 12 (the input field has maxlength=2)
3. Press Ctrl-A (Select All)
4. Press the Right arrow key (same with Left, Up and Down, but not Home or
PgUp)
5. Enter some character - it will be inserted despite the length limit

Comment 6

13 years ago
The bug is actually deeper - it is in nsSelection::MoveCaret(). Changing
component and description.
Assignee: mozeditor → selection
Component: Editor: Core → Selection
QA Contact: bugzilla
Summary: More than MAXLENGTH characters allowed in a text field when tabbing between fields → More than MAXLENGTH characters allowed in a text field after removing selection

Comment 7

13 years ago
Created attachment 155194 [details] [diff] [review]
Patch?

This patch solves the problem and seems to work correctly. However, I don't
understand the code in MoveCaret() and as well as the implications of this
patch. I could only find out that it is the call to TakeFocus() that ensures
the correct behavior.

Comment 8

13 years ago
Created attachment 155195 [details]
Simpler testcase

Forgot to remove some JavaScript code from the testcase, sorry about the spam.

Updated

13 years ago
Attachment #155190 - Attachment is obsolete: true
(Assignee)

Comment 9

13 years ago
(In reply to comment #7)
> Created an attachment (id=155194)
> Patch?
> 
> This patch solves the problem and seems to work correctly.

Almost, it fixes the bug, but you have also changed where the caret ends up
after a LEFT/RIGHT when you have an existing selection.

Another solution would be to remove the "if (!isCollapsed && !aContinue)"
block entirely. This would give you correct selection/caret movements under
Linux (which is to deselect and place the caret 1 pos left/right of where it
currently is), BUT the current code seems to emulate the Windows behaviour
(which is to deselect and place the caret where the selection started).
I'm guessing we originally had Linux behaviour and then someone added that
if-block to get Windows behaviour instead (on all platforms).

Could you track down the CVS checkin that added the if-block in question
and see what the checkin message says and if it mentions a bug number?

It seems incorrect to me to return that early in the function, skipping the
selection listener notifications and bidi stuff that comes later.

It seems quite easy to add a #ifdef here if we want platform-specific
behaviour - I don't know what the policy is on that though.

Comment 10

13 years ago
Yes, my "patch" is nonsense. I took a detailed look at what TakeFocus() is doing
and I see now that the whole if (!isCollapsed && !aContinue) block is doing
basically the same but with a simpler calculation for caret position (this seems
to fail here - as Boris pointed out it selects the wrong node). When collapsing
the selection it just places the caret to the right edge of the selection (for
the cursor right key). Only some programs on Windows do it this way, examples
are Internet Explorer and Word but not standard textfields or Notepad. Even
Internet Explorer has it for the text fields in web pages and the search dialog
but not for its address field and preferences dialog. As this behavior seems to
be inconsistent and non-default on Windows, I would prefer removing this block
entirely.

The checkin that added this was by mjudge@netscape.com on 19 Jun 1999 13:36. At
this time the code was in nsRangeList.cpp, function HandleKeyEvent() - the diff
is at
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base/src/Attic&command=DIFF_FRAMESET&file=nsRangeList.cpp&rev2=1.104&rev1=1.103
It's a mass change without a bug number, description "up/down selection BRFrames
dont allow selecting upon them for now. horizontal bars are now drawn selected.
ect." - this seems to be about the new SetDesiredX() function etc. but not about
this code. As mjudge isn't around anymore we probably won't find out the reason...

Comment 11

13 years ago
Created attachment 155251 [details] [diff] [review]
Proposed patch
Attachment #155194 - Attachment is obsolete: true

Comment 12

13 years ago
There is one regression that I noticed - when all text in a multiline text field
is selected, the arrow right key doesn't collapse the selection. The problem
doesn't occur with singleline fields (both in HTML and XUL) or HTML selections
so I suppose this is a bug in the multiline text field implementation.

Updated

13 years ago
Attachment #155251 - Flags: review?(mats.palmgren)
*** Bug 274757 has been marked as a duplicate of this bug. ***
Mats, is that last patch correct?
(Assignee)

Comment 15

13 years ago
Comment on attachment 155251 [details] [diff] [review]
Proposed patch

It fixes the original problem but it also introduces a regression
as noted in comment 12. To be more precise: when some text is
selected in a multiline textfield and the caret is at the end
position [of the last line] then the RIGHT key does not collapse
the selection. LEFT/UP/DOWN works though.
This is slightly annoying, and easier to trigger than the original
problem. So marking review- for now.
Attachment #155251 - Flags: review?(mats.palmgren) → review-
(Assignee)

Comment 16

13 years ago
Created attachment 169488 [details] [diff] [review]
WIP

Here is an updated patch that I think fixes the remaining problem.
It needs a lot more testing though and I would feel better if I knew
exactly why the added code is needed...

Will do some more investigation in a few days - taking bug for now
so I don't forget it. Help with testing this patch is appreciated.
Assignee: selection → mats.palmgren
Attachment #155251 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 17

13 years ago
Comment on attachment 169488 [details] [diff] [review]
WIP

The edge case does not ScrollIntoView correctly...
Attachment #169488 - Attachment is obsolete: true
(Assignee)

Comment 18

13 years ago
Created attachment 170068 [details] [diff] [review]
Patch rev. 2

The reason for the edge case for textareas is that we bump into the trailing
BRFrame (for which nsFrame::PeekOffset fails).
This patch has been tested on Mozilla/Linux and a static Firefox/WinXP build.
Attachment #170068 - Flags: superreview?(bzbarsky)
Attachment #170068 - Flags: review?(aaronleventhal)
Comment on attachment 170068 [details] [diff] [review]
Patch rev. 2

sr=bzbarsky, but could that extra bit be removed if bug 240933 is fixed?  If
so, please file a bug to do so and mark it dependent on bug 240933?
Attachment #170068 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 20

13 years ago
Aaron, is there any chance you can review this for 1.8b?
Should I ask someone else? who?
Flags: blocking1.8b?

Comment 21

13 years ago
Comment on attachment 170068 [details] [diff] [review]
Patch rev. 2

Sorry that I haven't been around lately to look at patches. Have Ginn Chen
review future patches involving selection/caret. He's been working a lot in
that area.

Can you add a comment to the MoveCaret declaration explaining what the
arguments are? I wasn't sure at first whether aContinue was for extending
selection or not.

Also, don't forget bz's request for the separate bug filing.
Attachment #170068 - Flags: review?(aaronleventhal) → review+
(Assignee)

Comment 22

13 years ago
Created attachment 172134 [details] [diff] [review]
Patch rev. 3

Unfortunately the fix now introduces a regression - when typing RIGHT in a
textarea on the last line and that line is empty, then the caret moves *back*
1 character. This makes the caret jump back/forward when holding RIGHT down
in a textarea when it should stay firm at the last position.
It does not occur when there is at least one char on the last line.

I am sure this did not occur when I attached the patch, so something must have
changed since then, nsSelection.cpp is still at rev. 3.191 so it must be some
other file. (I'm interested to know if you have any suggestions).

So, here's an updated patch that fixes that problem. There is only one
line added really:
+    tHint = mHint; // make the line below restore the original hint

The rest is just a rename of the parameter "aContinue" to "aContinueSelection"
to address Aaron's request to document it. (The name comes from TakeFocus()
which has the same parameter.)
(Assignee)

Updated

13 years ago
Attachment #170068 - Attachment is obsolete: true
Attachment #172134 - Flags: review?(bzbarsky)
Comment on attachment 172134 [details] [diff] [review]
Patch rev. 3

I'm really not qualified to r= this... Here's hoping Ginn knows this code.  The
change does look reasonable to me, though.
Attachment #172134 - Flags: superreview+
Attachment #172134 - Flags: review?(ginn.chen)
Attachment #172134 - Flags: review?(bzbarsky)

Comment 24

13 years ago
Comment on attachment 172134 [details] [diff] [review]
Patch rev. 3

r=me
Attachment #172134 - Flags: review?(ginn.chen) → review+
(Assignee)

Comment 25

13 years ago
Checked in 2005-01-25 14:38 PDT.
Filed bug 279824 on removing the workaround for <br> (see comment 18/19).

Many thanks to Wladimir Palant for doing the initial work on this bug.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Flags: blocking1.8b?
Resolution: --- → FIXED

Comment 26

13 years ago
*** Bug 295725 has been marked as a duplicate of this bug. ***

Comment 27

13 years ago
*** Bug 251778 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 28

12 years ago
See bug 299417 for further analysis of the underlying cause for this bug.
(Assignee)

Updated

12 years ago
Depends on: 299417
*** Bug 312312 has been marked as a duplicate of this bug. ***

Comment 30

12 years ago
*** Bug 312865 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.