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?
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?
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
The bug is actually deeper - it is in nsSelection::MoveCaret(). Changing component and description.
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.
(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 firstname.lastname@example.org 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...
Created attachment 155251 [details] [diff] [review] Proposed patch
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.
*** 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.
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.
Comment on attachment 169488 [details] [diff] [review] WIP The edge case does not ScrollIntoView correctly...
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.
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?
Aaron, is there any chance you can review this for 1.8b? Should I ask someone else? who?
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.
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.)
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.
Comment on attachment 172134 [details] [diff] [review] Patch rev. 3 r=me
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
*** 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.
*** Bug 312312 has been marked as a duplicate of this bug. ***
*** Bug 312865 has been marked as a duplicate of this bug. ***