Closed
Bug 207623
Opened 22 years ago
Closed 20 years ago
More than MAXLENGTH characters allowed in a text field after removing selection
Categories
(Core :: DOM: Selection, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: christophe.bliard, Assigned: MatsPalmgren_bugz)
References
()
Details
(Keywords: testcase)
Attachments
(3 files, 5 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
ginnchen+exoracle
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
The URL given WFM 20030527 PC/WinXP
Comment 2•22 years ago
|
||
Confirming on WinXP 2003052808. Perhaps related to bug 207094 or bug 204506?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•22 years ago
|
||
Updated•22 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
Comment 4•21 years ago
|
||
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•20 years ago
|
||
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•20 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•20 years ago
|
||
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•20 years ago
|
||
Forgot to remove some JavaScript code from the testcase, sorry about the spam.
Updated•20 years ago
|
Attachment #155190 -
Attachment is obsolete: true
Assignee | ||
Comment 9•20 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•20 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•20 years ago
|
||
Attachment #155194 -
Attachment is obsolete: true
Comment 12•20 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•20 years ago
|
Attachment #155251 -
Flags: review?(mats.palmgren)
Comment 13•20 years ago
|
||
*** Bug 274757 has been marked as a duplicate of this bug. ***
Comment 14•20 years ago
|
||
Mats, is that last patch correct?
Assignee | ||
Comment 15•20 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•20 years ago
|
||
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•20 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•20 years ago
|
||
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 19•20 years ago
|
||
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•20 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•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #170068 -
Attachment is obsolete: true
Attachment #172134 -
Flags: review?(bzbarsky)
Comment 23•20 years ago
|
||
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•20 years ago
|
||
Comment on attachment 172134 [details] [diff] [review]
Patch rev. 3
r=me
Attachment #172134 -
Flags: review?(ginn.chen) → review+
Assignee | ||
Comment 25•20 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
Closed: 20 years ago
Flags: blocking1.8b?
Resolution: --- → FIXED
Comment 26•20 years ago
|
||
*** Bug 295725 has been marked as a duplicate of this bug. ***
Comment 27•20 years ago
|
||
*** Bug 251778 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 28•20 years ago
|
||
See bug 299417 for further analysis of the underlying cause for this bug.
Comment 29•19 years ago
|
||
*** Bug 312312 has been marked as a duplicate of this bug. ***
Comment 30•19 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.
Description
•