Closed Bug 207623 Opened 21 years ago Closed 19 years ago

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

Categories

(Core :: DOM: Selection, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: christophe.bliard, Assigned: MatsPalmgren_bugz)

References

()

Details

(Keywords: testcase)

Attachments

(3 files, 5 obsolete files)

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 ;)
The URL given WFM 20030527 PC/WinXP
Confirming on WinXP 2003052808. Perhaps related to bug 207094 or bug 204506?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file testcase
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
Attached file Simpler testcase (obsolete) —
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
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
Attached patch Patch? (obsolete) — Splinter Review
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.
Attached file Simpler testcase
Forgot to remove some JavaScript code from the testcase, sorry about the spam.
Attachment #155190 - Attachment is obsolete: true
(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.
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...
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #155194 - Attachment is obsolete: true
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.
Attachment #155251 - Flags: review?(mats.palmgren)
*** Bug 274757 has been marked as a duplicate of this bug. ***
Mats, is that last patch correct?
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-
Attached patch WIP (obsolete) — Splinter Review
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
Comment on attachment 169488 [details] [diff] [review]
WIP

The edge case does not ScrollIntoView correctly...
Attachment #169488 - Attachment is obsolete: true
Attached patch Patch rev. 2 (obsolete) — Splinter Review
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+
Aaron, is there any chance you can review this for 1.8b?
Should I ask someone else? who?
Flags: blocking1.8b?
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+
Attached patch Patch rev. 3Splinter Review
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.)
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 on attachment 172134 [details] [diff] [review]
Patch rev. 3

r=me
Attachment #172134 - Flags: review?(ginn.chen) → review+
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
Closed: 19 years ago
Flags: blocking1.8b?
Resolution: --- → FIXED
*** Bug 295725 has been marked as a duplicate of this bug. ***
*** Bug 251778 has been marked as a duplicate of this bug. ***
See bug 299417 for further analysis of the underlying cause for this bug.
Depends on: 299417
*** Bug 312312 has been marked as a duplicate of this bug. ***
*** 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.

Attachment

General

Created:
Updated:
Size: