One thing which would aid JavaScripters everywhere would be to expose the current position of the cursor within a particular <input type="text" /> or <textarea></textarea> as relative to the value attribute string. For instance, if the string is empty or the cursor is at the beginning of a string, a theoretical .cursorPosition property would be 0. I would suggest the .cursorPosition property could reflect what the position of the next-typed character would be if the insert mode were on (instead of overstrike) : The quick brown fox jumped over the lazy brown dog. ^ cursor to the left of the "e" (== charAt(2)), .cursorPosition = 2 Being able to both get and set this property would be very nice. It would also assist in allowing JS to control positioned insertions and removal of data based on where the cursor position is. Or is this functionality already supported? ;)

.caretPosition seems better to me. Can you just grab the selection instead? Is this really necessary?

I think this would be very useful. Exposing a .caretPosition property for textareas and input fields would be quite clean and make it easy to insert text following the cursor. You can do something similar using IE 4/5's createTextRange, but that seems like a hack. See this site: http://www.faqts.com/knowledge_base/view.phtml/aid/1052/fid/130 It would also be nice to be able to get the current selection and modify it. Is there any current way to do this? Perhaps a .selectionStart and .selectionEnd should also be exposed, where they would also be offsets into the text.

I believe modifying selections is covered by the DOM-2 Range Recommendation. For the record, this RFE deals exclusively with a single cursor point, not with a selection. There is no selection to grab. Forgive me for being obtuse, but what's the difference between a cursor and a caret? As far as I know, a caret is ^. A cursor is a blinking |. :)

The caret *is* a selection. Look at Composer. The caret is merely the selection with the same start/ending locations. I think the DOM Range spec is sufficient. The different between a cursor and a caret (as far as this bug is concerned) is this: * a caret is the vertical line that blinks between characters and shows the location where typed characters will be inserted * a cursor is the arrow (typically) that moves around on the screen as you move your mouse. You might use the cursor (which should become an I-Beam shape) to place the caret in a new location. I'll let jst or someone else decide whether to resolve this bug as Invalid, WontFix, WorksForMe, or Dup.

We already have selectionStart and selectionEnd (see bug 70353, etc). Is that sufficient?

The bug you reference has been marked as a dup of bug # 58850 . In it, there is a comment: ------- Additional Comments From Simon Fraser 2001-07-05 17:01 ------- This is basically making .selectionStart, .selectionEnd, .textLength work for multiline textareas, as they do now for single line inputs. -- end comment -- I know this is unfair, to reply based on comments made after your last remark. Still, at the very least, it depends on 58850 getting fixed.

Clarification: the quote above is from 58850. Sorry for the spam.

Sounds like we already have what we need to support this, marking WONTFIX, reopen if you disagree.

I've verified that .selectionStart and .selectionEnd works for text input elements, so this is at least partially supported. For example, select some text in the Summary field above and then paste the following JavaScript into the address box: javascript:alert(document.forms['changeform'].short_desc.selectionStart) You'll see the position of the start of the selection. However, this does not work for textareas. Will fixing bug 58850 provide this functionality? Are XBL and DOM that closely related? If the fix for bug 58850 will provide the selectionStart and selectionEnd functionality to textareas, then I'd say this can stay wontfixed. You MIGHT be able to get this to work for textareas using the DOM 2 Range object, but that seems very convoluted. You'd have to create a range from window.getSelection() and determine afterward whether the range relates to a textarea. And I haven't been able to reliably get selections in text input fields and textareas. For anyone who's trying to struggle through getting this to work, I found the following article quite helpful: http://www.pbwizard.com/Articles/Moz_Range_Object_Article.htm

input.selectionStart n' friends are mozilla extensions to the DOM, we don't support that on textareas, bug 58850 is not direcly related to this, so if you want a RFE on file for supporting .selectionStart n' friends for textareas, reopen this bug.

Please. :)

Sure, but it won't happen for mozilla 1.0 unless someone contributes a fix or convinces me that it's important enough.

See also bug 85686, window.getSelection() fails when text selected in a form field.

*** Bug 128100 has been marked as a duplicate of this bug. ***

Ok, i just implemented this because i needed it. Stealing from jst patch forthcoming. --pete

Cathy, jst does 1.01 sound reasonable to land this? --pete

Will this patch fix bug 58850, bug 75629, and situation with multiline XUL textboxs? (Please say yes!)

Comment on attachment 80712 [details] [diff] [review] patch to implement textLength, startSelection and endSelection for textarea widget >Index: layout/html/forms/src/nsGfxTextControlFrame2.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/html/forms/src/nsGfxTextControlFrame2.cpp,v >retrieving revision 1.195.2.3 >diff -u -r1.195.2.3 nsGfxTextControlFrame2.cpp >--- layout/html/forms/src/nsGfxTextControlFrame2.cpp 13 Apr 2002 01:36:58 -0000 1.195.2.3 >+++ layout/html/forms/src/nsGfxTextControlFrame2.cpp 24 Apr 2002 01:02:35 -0000 >@@ -1527,7 +1527,8 @@ > { > PRInt32 type; > GetType(&type); >- if ((NS_FORM_INPUT_TEXT==type) || (NS_FORM_INPUT_PASSWORD==type)) { >+ if ((NS_FORM_INPUT_TEXT==type) || (NS_FORM_INPUT_PASSWORD==type) >+ || (NS_FORM_TEXTAREA==type)) { > return PR_TRUE; > } > return PR_FALSE; >Index: dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl >=================================================================== >RCS file: /cvsroot/mozilla/dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl,v >retrieving revision 1.5.36.1 >diff -u -r1.5.36.1 nsIDOMNSHTMLTextAreaElement.idl >--- dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl 10 Apr 2002 02:33:47 -0000 1.5.36.1 >+++ dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl 24 Apr 2002 01:02:35 -0000 >@@ -46,5 +46,9 @@ > [scriptable, uuid(ca066b44-9ddf-11d3-bccc-0060b0fc76bd)] > interface nsIDOMNSHTMLTextAreaElement : nsISupports > { >- readonly attribute nsIControllers controllers; >+ readonly attribute nsIControllers controllers; >+ >+ readonly attribute long textLength; >+ attribute long selectionStart; >+ attribute long selectionEnd; > }; Do we really want a textLength property? It can be very easily done in JS, textarea.value.length. Is there any compelling reason to implement it? Does IE have it? >+NS_IMETHODIMP >+nsHTMLTextAreaElement::GetSelectionStart(PRInt32 *aSelectionStart) >+{ >+ NS_ENSURE_ARG_POINTER(aSelectionStart); >+ nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_TRUE); >+ >+ nsresult rv; >+ if (formControlFrame) { >+ nsIGfxTextControlFrame2* textControlFrame = nsnull; >+ if (formControlFrame) >+ CallQueryInterface(formControlFrame, &textControlFrame); >+ Why check twice for if (formControlFrame)? I'd rewrite this as if (formControlFrame) { nsCOMPtr<nsIGfxTextControlFrame2> textControlFrame = do_QueryInterface(formControlFrame); Note that I don't know whether you can use nsCOMPtr with nsIGfxTextControlFrame2 but I see no reason why you couldn't. >+ if (textControlFrame) { >+ PRInt32 selectionEnd; >+ textControlFrame->GetSelectionRange(aSelectionStart, &selectionEnd); >+ } >+ } >+ return rv; >+} >+ This function could return an unitialized rv if GetFormControlFrame failed. The same is true for all the functions below. >+NS_IMETHODIMP >+nsHTMLTextAreaElement::SetSelectionStart(PRInt32 aSelectionStart) >+{ >+ nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_TRUE); >+ >+ nsresult rv; >+ if (formControlFrame) { >+ nsIGfxTextControlFrame2* textControlFrame = nsnull; >+ >+ if (textControlFrame) That will never be true, you set it to nsnull two lines above! Assuming you meant to do the same as in GetSelectionStart(), my comments there apply here as well. >+ rv = textControlFrame->SetSelectionStart(aSelectionStart); >+ } >+ return rv; >+} >+ >+NS_IMETHODIMP >+nsHTMLTextAreaElement::GetSelectionEnd(PRInt32 *aSelectionEnd) >+{ >+ NS_ENSURE_ARG_POINTER(aSelectionEnd); >+ nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_FALSE); >+ >+ nsresult rv; >+ if (formControlFrame) { >+ nsIGfxTextControlFrame2* textControlFrame = nsnull; >+ if (formControlFrame) >+ CallQueryInterface(formControlFrame, &textControlFrame); >+ Same as above... >+ if (textControlFrame) { >+ PRInt32 selectionStart(0); >+ rv = textControlFrame->GetSelectionRange(&selectionStart, aSelectionEnd); >+ } >+ } >+ return rv; >+} >+ >+NS_IMETHODIMP >+nsHTMLTextAreaElement::SetSelectionEnd(PRInt32 aSelectionEnd) >+{ >+ nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_TRUE); >+ >+ nsresult rv; >+ if (formControlFrame) { >+ nsIGfxTextControlFrame2* textControlFrame = nsnull; >+ And again... >+ if (textControlFrame) >+ rv = textControlFrame->SetSelectionEnd(aSelectionEnd); >+ } >+ return rv; >+} > > nsresult > nsHTMLTextAreaElement::Reset()

Doh I forgot to trim the attachement before submitting it! Sorry for that!

> Do we really want a textLength property? Yes and i can give you two reasons why. 1. <nsIDOMNSHTMLInputElement> implements it, so we should be consistent. 2. textLength is usually needed when using the other two attributes. If it is ommitted, then a consumer(c++) has to instantiate <nsIDOMHTMLTextAreaElement> in addition to this interface just for that specific attribute. So this is another good reason to have it around. --pete

Changes: - now using nsCOMPtr and ignoring "Whne in Rome" clause. - SetSelectionStart and SetSelectionEnd actually work now. - Fixed return values hey it was late when i did this last nite. ;-) --pete

I'd really like to see this for Mozilla1.0. IE currently has far better support for manipulating the text selection in textareas than Mozilla. I don't know of a way to get the selection in a textarea for Mozilla (see bug 85686 ). This patch would even out the difference and is important for developers that want to create textarea editing tools.

Comment on attachment 80765 [details] [diff] [review] revised patch Ok now I'm having a doubt... do those form frames have to be refcounted? No other code that I can see uses nsCOMPtr on them. jst? Also, I seem to remember that we shouldn't return the rv of a QI, but I'm not sure now. Last nit, the standard format for the return values (in the DOM code) is: rv = Function(); NS_ENSURE_SUCCESS(rv, rv); instead of if (NS_FAILED(rv = Function)) return rv;

cc'ing jst. --pete

Comment on attachment 80765 [details] [diff] [review] revised patch Using nsCOMPtr's on nsIFrame's is fine, you don't get the benefit of the refcounting since none of the frame implementations are refcounted, but you can still use nsCOMPtr's on them if you really want to. - In nsGfxTextControlFrame2.cpp: PRInt32 type; GetType(&type); - if ((NS_FORM_INPUT_TEXT==type) || (NS_FORM_INPUT_PASSWORD==type)) { + if ((NS_FORM_INPUT_TEXT==type) || (NS_FORM_INPUT_PASSWORD==type) + || (NS_FORM_TEXTAREA==type)) { This is in IsSingleLineTextControl(), NS_FORM_TEXTAREA's are not single line text controls, so this seems broken to me. - In nsIDOMNSHTMLTextAreaElement.idl, please follow the formatting used in the other nsIDOMHTML* idl files. - In nsHTMLTextAreaElement.cpp: +NS_IMETHODIMP +nsHTMLTextAreaElement::GetTextLength(PRInt32 *aTextLength) +{ Add NS_ENSURE_ARG_POINTER(aTextLength) here. - In nsHTMLTextAreaElement::GetSelectionStart(): + PRInt32 selectionEnd; + if (NS_FAILED(rv = textControlFrame->GetSelectionRange(aSelectionStart, &selectionEnd))) + return rv; What Fabian said, keep variable assignment out of if expressions, we're not trying to minimize linecount here, make it as readable as you can! Oh, and don't forget braces around the statement in if's, even if they're one-liners. There are several cases like this, same applies to all of them. Hmm, actually the whole block where that code is from could be written like this: + if (formControlFrame) { + nsCOMPtr<nsIGfxTextControlFrame2> textControlFrame = do_QueryInterface(formControlFrame); + + if (textControlFrame) { + PRInt32 selectionEnd; + return textControlFrame->GetSelectionRange(aSelectionStart, + &selectionEnd))) + } + } I.e. no need for the rv check after QI, and simply returning the return value fro GetSelectionRange() w/o using a local temporary is just fine... Again, similar code found in a few places, same applies to all... Needs work...

jst, thanks for the review. I'll get on this because this is something I am maintaining in a seperate distro. ;-( Less headaches when checked in. ;-) --pete

*** Bug 143754 has been marked as a duplicate of this bug. ***

I still haven't had the time to update this patch. So i'll take a minute to respond. - if ((NS_FORM_INPUT_TEXT==type) || (NS_FORM_INPUT_PASSWORD==type)) { + if ((NS_FORM_INPUT_TEXT==type) || (NS_FORM_INPUT_PASSWORD==type) + || (NS_FORM_TEXTAREA==type)) { > This is in IsSingleLineTextControl(), NS_FORM_TEXTAREA's are not single line > text controls, so this seems broken to me. This above code is crutial for the imlementation to work. I think the func name is misleading here, the semantics of it's usage imply a single nsIContent text node, which is exactly what we are. We are selecting within the range of a *single* moz-text node only. So that's why we set NS_FORM_TEXTAREA to be true. I'll try to clean up the rv test's this week and send in a new patch. Thanks --pete

This functionality, or lack there of, is a current topic of conversation at metafilter at the thread here: http://metatalk.metafilter.com/mefi/2214 The specific functionality that is missing is the ability to add basic html formatting to a comment is a textarea. This functionality is available in IE via the following script. http://www.metafilter.com/scripts/form_shortcuts_ie.js I see that some work is currently being done, but since metafilter is a very high profile site I'd ask that the priority be re-evaluated. This may not be a broken experience for Moz users, but based on the initial posters comments in the linked thread above its very obvious distiction in functionality for someone moving from IE to Moz/NN6/etc.

BTW: i've been baking this code using it extensively for a month now FWIW. --pete

Pete, does your latest patch for this still work? If it does we should get it reviewed and checked in (this bug is currently targeted at mozilla 1.0.1, and I would really like to see it fixed).

Yep works fine. No patch rot yet. I just don't have the time to chase down reviewers. --pete

Ok, let's get it in. If someone currently on this cc list can give us a review (jst?), I'll go hunt us down an sr on irc...

I can sr once someone has reviewed, bz?

I'm not convinced of the changes to IsSingleLineTextControl().... In particular: 1) nsGfxTextControlFrame2::CreateAnonymousContent will set the wrong overflow values with this patch 2) The same function will set editorFlags |= nsIPlaintextEditor::eEditorSingleLineMask; which doesn't seem right... (but an editor person should comment on that) 3) nsGfxTextControlFrame2::SelectAllContents() looks like it will break 4) nsGfxTextControlFrame2::IsScrollable() will return the wrong thing and so forth. Why do we need to change this function? Would it not make more sense to have a separate function called something like |IsSingleTextNodeControl()| or something that would basically return |IsSingleLineTextControl() || IsTextarea()|? Then change only the places we need to.... As a separate matter, it may make sense to create an nsIDOMNSHTMLTextControl or something that will have these props and be implemented by all the classes in question (just to prevent multiple interfaces that look identical). But that's jst's call....

Added IsTextArea() method. --pete

> it may make sense to create an nsIDOMNSHTMLTextControl Disagree, nsIDOMNSHTMLTextAreaElement is synonymous w/ nsIDOMNSHTMLInputElement. So implementing the nsIDOMNSHTMLTextAreaElement interface make the most sense to me. --pete

Comment on attachment 87720 [details] [diff] [review] Agree, all i care about here is if it's a textarea >+ nsresult rv = GetValue(val); >+ *aTextLength = val.Length(); >+ >+ return rv; Shouldn't you test NS_SUCCEEDED(rv) before changing *aTextLength? >+nsHTMLTextAreaElement::SetSelectionStart(PRInt32 aSelectionStart) >+{ >+ nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_TRUE); >+ >+ if (formControlFrame) { >+ nsIGfxTextControlFrame2* textControlFrame(nsnull); >+ >+ if (textControlFrame) { How does this work exactly? Same for SetSelectionEnd();

Dude, it's an auto string. You tell me what happens if you call Length() on an unitialized autostring. YOU GET ZERO!! > How does this work exactly? Same for SetSelectionEnd(); I'll tell you how it works. Exactly the same as the implementation for nsHTMLInputElement. Please stop querying me on a patch that was written over TWO MONTHS ago. I can't remember what i had for breakfast this morning. Mozilla is choking from the bureaucracy of "patch police". Holding a tribunal for every single patch is not very efficient. Instead, someone who knows this code well should review it. Kindly stop wasting my time and r= this sucker already. --pete

Geee Pete, stop acting silly! >+ nsIGfxTextControlFrame2* textControlFrame(nsnull); >+ >+ if (textControlFrame) { Why exactly do you need the if here? You know it's null, you just set it to null yourself.

I posted the wrong patch from the wrong machine. --pete

For the record, patch #80765 has the correct implementations for the setters. But since i've been chasing my tail around w/this patch, the implementation got misconstrued over time and revisions to the point where a working implementation just got plain old broken. Which supports my justified bitching comments above. When you have all these chefs poking their heads in w/ their 2 cents, forking over their opinions, the dinner gets spoiled. Valuable time is wasted and contributors who take the time to post a patch get very frustrated. --pete

* Find out whether this control edits plain text. (Currently always true.) * @return whether this is a plain text control */ + virtual PRBool IsTextArea() const; + /** + * Find out whether this control is a textarea. + * @return whether this is a textarea text control + */ virtual PRBool IsPlainTextControl() const; The comments for these functions are inversed.

Comment on attachment 89087 [details] [diff] [review] fixed comment past r=bzbarsky, but for the record there is no guarantee that a failed call will not initialize out params to some crap.... It just happens that GetValue() does not to so...

I don't suppose there's any chance of this getting into the 1.0 branch (after we get an sr=) is there?

Seems unlikely at this point.

Comment on attachment 89087 [details] [diff] [review] fixed comment past - In nsGfxTextControlFrame2::IsTextArea(): + if (nsHTMLAtoms::textarea == tag.get()) + return PR_TRUE; Loose the .get() on the nsCOMPtr, it's not needed here. - In nsHTMLTextAreaElement::GetSelectionStart(): + nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_TRUE); + + if (formControlFrame) { + nsIGfxTextControlFrame2* textControlFrame(nsnull); + CallQueryInterface(formControlFrame, &textControlFrame); Why not use a nsCOMPtr here like you do in all other places? If you'd do that you could loose the null check for formControlFrame. - In nsHTMLTextAreaElement::SetSelectionStart(): + nsIFormControlFrame* formControlFrame = GetFormControlFrame(PR_TRUE); + + if (formControlFrame) { + nsCOMPtr<nsIGfxTextControlFrame2> + textControlFrame(do_QueryInterface(formControlFrame)); + No need for the if (formControlFrame) check here, do_QueryInterface() is null safe. + if (textControlFrame) ... - In nsHTMLTextAreaElement::GetSelectionEnd(), same thing, and also the same thing in nsHTMLTextAreaElement::SetSelectionEnd(). sr=jst if you remove those unnecessary null checks and consistently use nsCOMPtr's.

Note that this sr= is intended for the 1.1 branch; I specifically asked jst for that.

per jst comments and added necessary additional checks for IsTextArea() in nsGfxTextControlFrame2.cpp

Comment on attachment 89401 [details] [diff] [review] for sr= sr=jst

Checked into the trunk. I heard rumors about a possible branch landing? --pete

@@ -2798,7 +2812,7 @@ if (!firstRange) return NS_ERROR_FAILURE; - if (IsSingleLineTextControl()) + if (IsSingleLineTextControl() || IsTextArea()) { firstRange->GetStartOffset(aSelectionStart); firstRange->GetEndOffset(aSelectionEnd); I don't think the patch that just landed on the trunk will return the expected offsets when you have lines with returns etc ... the reason is that the content inside a textarea isn't completely inside a single text node like it is in a textfield. Textarea content contains textnodes *and* <br> nodes, so just relying on the offsets in the first range of the selection will give you wrong answers if the caret is ever between a text node and a <br>, or the range end points are in 2 different containers ... you can get into those situations by arrowing around or clicking the mouse at the end of a line, or dragging across lines.

Ok yep i see, I'm on it now. --pete

This implemenation works fine however, there is a nasty bug where if you select (collapsed or expanded) just past the end of a text node the values for <nsIDOMRange> GetStartOffset and GetEndOffset are the container div and *not* the starting and ending text nodes. I really couldn't write a solid workaround for it. It needs to be fixed in nsIDOMRange i assume. I will implement the setters for selectionStart/End next. --pete

Er, the methods i meant were GetStartContainer and GetEndContainer. I seem what triggers the problem is clicking on the hidden <br> at the end of any text node line. --pete

###!!! ASSERTION: offset we got from binary search is messed up: 'i<= mContentLength', file nsTextFrame.cpp, line 3515 Break: at file nsTextFrame.cpp, line 3515 It is asserting because of the offset mismatch here. --pete

While using attachment http://bugzilla.mozilla.org/attachment.cgi?id=89748&action=view with build 2002070108, the example does not work as expected when there are new lines in the text field. Reproduction: 1. Text area contains following: This is test line 1 This is test line 2 This is test line 3 This is test line 4 This is test line 5 This is test line 6 2. Select the word "test" of line 3 and press "get selection". "start"=8 and "end"=12 and "length"=120. start should = 48 and end should = 52. 3. Click "set selection", the correct area for start=8 and end=12 is selected, the word "test" of line 1, but this should be the select from offset 48 to 52.

Right, i am almost finished implementing setSelection. patch #98747 has the correct implementation for getSelection. --pete

Pete, it *isn't* a bug that the range start/end containers can be the actual <div> ... there are no guarantees with selection that the selection will always be inside textnodes. It is only a bug if the container is ever anything other than the div or one of it's children. Any code that interprets the selection ranges will have to handle this case or you will not get predictable results. We should also make sure that the selection offset getting/setting methods handle empty textnodes too. There are rare occassions when the editor can leave one lying around (right jfrancis?) ... also once we make it so that you can manipulate the nodes under a textarea via the dom/js people will be able to insert empty textnodes too.

Kin, right and i have a very painful workaround that deals w/ this to the point where it is 95% functional. The problem is that when you select the div there is no accurate offset to go by. The resulting offset value from the div selection doesn't make sense. How do i know what node the selection is intended for w/out some kind of offset? I will have another look at it. I think i am handling blank text nodes because i am QI'ing the children of the div and if there is an <nsIDOMText> text node, it is counted. I will double check this as well to insure they are counted. --pete

> The problem is that when you select the div there is no accurate offset > to go by. The resulting offset value from the div selection doesn't make sense. The offset you get if one of the end points of the range is in a text node isn't exactly accurate either. That's why you have to run through all the children of the div adding up all the text nodes and their lengths and the total number of <br> nodes you run across till you hit the text node containing the end point you are interested in, to which you then add the end points offset. With an end point in the div you do the same thing, except you don't have to worry about partially selected text nodes like the previous case. > How do i know what node the selection is intended for w/out some kind of > offset? I'm not sure what you mean by this question. The range guarantees that the endpoints will either be the same (collapsed), or the start is before the end point (uncollapsed). If the selection is uncollapsed all the nodes entirely between the end points are selected, if the start point is in a text node, only those chars *after* the start offset are in the selection, if the end point is in a text node only those chars in the text node *before* the offset are included in the selection.

Yea, the problem i'm trying to describe here is this. We know we are at the end of a node when this happens. So the offset is enough if i add the br's to get me aproximately to the target text node. This works in most cases. The real problem is when the offset number isn't high enough to reach the node that was selected therefore giving me the previous node which is wrong. It is tough to explain so i'm going to post the full patch when i'm finished w/ the setter implementation and then write up a test for this edge case i'm trying to explain here. --pete

This patch implements the getters and setters, lastly i need to merge in the workaround I have for edge case div selects and figure out one very specific difficult case and test for empty textnodes. --pete

Ok, duh, i see now the offset is giving me the childNode number. This should be easy to do. --pete

This should do the trick. I'll see if i can tighten things up tomorrow. Also let me know if i should lose the debug methods or if anyone can use them. -pete

Ok, i'll take some eyeballs on this if anyone has time. Also if someone can post a test case using appends, deletes and inserts on textarea textNodes and seeing if our getters and setters here are still accurate that would be awsome. Thanks --pete

http://www.concept6.co.uk/mozilla/mozillaeditor.htm is a little test page I whipped up to test this textarea features. I have noticed that .getSelectionEnd returns inconsistent information if the caret is positioned at the end of the textarea contents. 1). Place the caret at the end of the text in the textarea 2). Use the Caret end button 3). Repeat 1) + 2) until you get a value such as 1 or 0 (or 2 on a few occasions). Apart fom that, this functionality is great and goes some way to allowing simple HTML editors to work well in Mozilla. As soon as this is fixed I am gonna crack on and finish something with a bit more power.

Refinement to test If you position the caret anywhere in the text other than right at the end and then move to the end using either the END key or the arrows, then all is well. However, if you position the caret at the end of the text using a mouseclick, it goes wrong. If you position the caret at the end and then move to the left and then back again, it works as expected.

Tony, the problem you are seeing is *not* using attachment 89928 [details] [diff] [review] right? Your test case works properly for me using a trunk build w/ the patch provided below. Thanks --pete

Ooops - missed that attachment. Will look again once its s + sr

Comment on attachment 90222 [details] [diff] [review] very minor tweak. Patch for review and super review I've only had time to look at your SetSelectionEndPoints() changes so far. Here are some of my notes: --- There seems to be an assumption that the first child in a text widget is a textnode ... this isn't true for text widgets that have no content, or have an initial blank line. --- If SetSelectionRange() is ever called, SetSelectionEndPoints() won't set the correct selection if the textarea's div contains more than one child. --- Before the IsTextArea() check, there seems to be some code that sets the initial values of selStart and selEnd ... that's not needed in the textarea case since it looks like the code assumes that the start and end offsets of the range are in the same container node. Can't we just rewrite this entire method so that no assumptions are made as to what type of widget we are, and how many children there are underneath the widget? --- What is the purpose of getting firstTextNode's parent and then asking it if it has children? If it's an error check, should we throw an error if it has no children? --- Should you just continue if you have a br node since it can never be a DOMText node anyways? --- What's this special case about? + if (i <= 2) { + // if our start/end is less than length + // of first node make the assignment and bail --- targetNode gets set only when targetSelect falls between count and the contents of a text node. This won't work if one of the offsets we are trying to find is on a blank line between <br> nodes.

"Can't we just rewrite this entire method so that no assumptions are made as to what type of widget we are, and how many children there are underneath the widget?" Ideally, it seems to me that's what to do -- and that's the idea behind FIXptr. http://lists.w3.org/Archives/Public/www-xml-linking-comments/2001AprJun/att-0074/01-NOTE-FIXptr-20010425.htm Taking a look at FIXptr might provide some insight for this bug. petejc, I suggest taking a look at what I did in bug 122524 for "View Selection Source". There is a function |getPath| in attachment 82672 [details] [diff] [review] which might perhaps provide you with some useful clues, and which can be made to compute a FIXptr "tumbler" relative to the <textarea> node, and by descending the path (again, see a |for| loop to that effect in the attachment), you could add up the number of characters along the way to compute (in a certain way) the selection offsets in terms of number of characters.

For summary: a FIXPtr is a compact string to locate any point in the DOM. It is somewhat the "label" that one might get when walking the DOM, e.g., the pointer "/1/5/12(6)" identifies the 6th character of the 12th child element inside the 5th child element inside the root element of the document. "(6)" is optional because if the 12th child isn't a text node, "(6)" does have a meaning, and the correct syntax in such a case doesn't have it, e.g., "/1/5/12". Internally the notation can be different (e.g., an array "{1,5,12}" can be used as |getPath| did). I draw attention to FIXPtr because it provides a conceptual framework to investigate this bug (and thinking generically as kin indicated might help to get a nearly bug free patch). For the purpose of this patch, you can choose to interpret: selectionStart = 0, to mean textarea/0, i.e., the first text-node inside textarea (whether a direct decesdant or not). If there is no text-node (yet), it can mean the logical first child (which also means that there is no actual text to select anyway). selectionStart > 0, to mean there must be a text-node (or perhaps that a text-node should be created), and one could recover the node of interest by walking the <textarea> hierarchy (a la FIXptr) and adding up the number of characters on the text-nodes along the way.

started refactoring setter so it will be general purpose and work on single or multiline controls. Not for review. --pete

I have just tried the test page http://bugzilla.mozilla.org/attachment.cgi?id=89748&action=view with Mozilla 1.1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1) Gecko/20020826) and I am a bit puzzled by the results when I enter a second line into the textarea and then select some text in that line with the mouse and try the get selection button. The values then shown for start and end are relative to the line and not relative to the complete text in the textarea. Is there some property that tells me also which line the selection is in? I had a look at http://lxr.mozilla.org/seamonkey/source/dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl#52 and it doesn't show any property for the line so I think it is a bug that the start and end values are relative to the line.

Yea, the code that has been checked in isn't finished. There are patches below that yeild a solid working implementation but they haven't been checked in. I also started refactoring the code to be more general purpose but haven't had the time to finish it. ;-( --pete

Pete - Finish this for 1.2 final and you'll receive a case of beer (foreign or domestic) of your choice from me ;-). - Bill Edney - bedney@technicalpursuit.com

Bill, are you serious? I will certainly take you up on the offer if it is valid. Infact i'll try to crank it out this weekend. Beer is a first tier motivator for me. ;-) --pete

I'm completely serious. I'll send you contact info via e-mail.

mmmmm, beer. --pete

Whoa. I'll get your beer for next weekend if you look at bug # 97284.

Ah, but what will you do for the reviewers? :) And whoever checks this in and watches the t-box?

Getter is now solid w/ less code bloat. Still need to revise setter and cut down code bloat there as well now that i understand better how the offfsets work. --pete

Still need to handle reverse values. In other words if a user puts a higher value for a start selection than then the end selection value. Need to flip them. Other than that this patch is simplified and works exactly as it should. --pete

There are some tabs that were expanded in this patch so i'll post the same patch w/out the shite space noise for review after this complete patch. --pete

Patch 99276 is ready for review. Ping Kin or anyone who can review this code. Thanks --pete

I'll try to look at the patch over the next couple of days.

Cool thanks Kin. --pete

Comment on attachment 99276 [details] [diff] [review] Complete finished patch Sorry for the delay, I haven't actually finished reviewing this stuff due to other distractions, but rather than wait till I'm done, I thought I'd post some of the notes I had so far before I lose them ... ==== I actually like the name |IsTextArea()| since that's exactly what it looks for. To confuse things even more, single line text controls can also have multiple lines, depending on what platform we are running on. -PRBool nsTextControlFrame::IsTextArea() const +PRBool nsTextControlFrame::IsMultiLineTextControl() const ==== This should be |NS_FORM_TEXTAREA == type|. - if (nsHTMLAtoms::textarea == tag) + if (NS_FORM_TEXTAREA) return PR_TRUE; ==== The comment in |GetFirstNode()| isn't exactly correct. It currently says: // for a text widget, the text of the document is in a single // text node under the body. Let's make sure that's true. Text widgets use a "div" as a root node, and under that node can be one or more text and br nodes. Note that an empty text widget will contain a single br and *no* text node. ==== Also in |GetFirstNode()|, the length of |childNodeList| is retrieved but never used. Also, if all you are interested in is finding out if |rootNode| has children, you can just call |root->HasChildNodes()|. nsCOMPtr<nsIDOMNodeList> childNodesList; rootNode->GetChildNodes(getter_AddRefs(childNodesList)); if (!childNodesList) { NS_WARNING("rootNode has no text node list"); return NS_ERROR_FAILURE; } PRUint32 numChildNodes = 0; childNodesList->GetLength(&numChildNodes); ==== I'd prefer if we split this out into 2 statements: + NS_ADDREF(*aFirstNode = firstChild); ==== I really don't think there's a need for a |GetFirstTextNode()|. The only real difference between it and |GetFirstNode()| is a |QueryInterface()|, and besides the only method that calls |GetFirstTextNode()| is |SetSelectionEndPoints()| which also calls |GetFirstNode()| also? ==== It's entirely possible that |SetSelectionEndPoint()| will get called before any call is ever made to |SetSelectionStartPoint()|, in which case, |mStart| would be invalid to compare against. In general I think it's a bad idea to try and track an |mStart| between |SetSelectionStart()| and |SetSelectionEnd()| calls, especially given the fact that it can be thrown off by the user simply using the mouse to click or select elsewhere in the text widget. Also why the need to reset |targetSelect = aSelEnd|? It should have already been set to |aSelEnd| above this code. + // if end is less than start flip em + if (aSelStart == eIgnoreSelect && mStart > aSelEnd) { + targetSelect = aSelEnd; + flip = PR_TRUE; } ==== There also needs to be some discussion of what exactly is expected to happen in certain situations, for example when the caller sets an end point that is before the selection start, or a start point which is after the current selection end. Right now both cases are handled inconsistently, the first case basically sets a selection that leaves the end point at a different place, than the user just set, and the latter case, just throws an error. Personally, I think that in both situations we should just collapse the selection to the point being set. ==== |SetSelectionEndPoints()| should never place an endpoint to the right of a br node that is the last child of the div. ==== In |GetTargetNode()| we get the parent of |aFirstNode|, why do we need to ask if |parent| has children? + aFirstNode->GetParentNode(getter_AddRefs(parent)); + if (NS_FAILED(rv) || !parent) return rv; + + PRBool hasChildNodes(PR_FALSE); + rv = parent->HasChildNodes(&hasChildNodes); ==== |GetSelectionRange()| probably shouldn't differenciate between single line text controls and textareas for the reason I said above regarding some platforms with multi line text in single line text controls. Also, I see this |if| construct, when would the |else| ever be used? + if (IsSingleLineTextControl()) { ... + } else if (IsMultiLineTextControl()) { ... + } else { // multiline ==== setSelectionRange() needs to be added to dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl. ==== SetSelectionRange() needs the following line removed otherwise, it won't work with textareas: if (!IsSingleLineTextControl()) return NS_ERROR_NOT_IMPLEMENTED; ==== Because we are currently concerned about code bloat, I would remove all of your debug methods which are not used: + nsresult PrintTagName(nsIContent* aContent); + nsresult DumpContent(nsIContent* aContent, const char* aName); + nsresult DumpContent(nsIDOMNode* aNode, const char* aName); + nsresult DumpNodeData(nsIDOMNode* aNode, const char* aName); + void PrintPointer(nsISupports* aPointer, const char* aName);

Ok, i should have some time tonight. I'll read through Kins comments and adjust/dispute where necessary. --pete

Pete - The tree is closing for 1.2 beta, with only driver approval checkins after that. It is still possible to get this into 1.2. Please? :-) - Bill Edney

Bill, yea sorry. I've been swamped w/ work. Life and death stuff. :( I'll try to make a pass tonight. --pete

==== I actually like the name |IsTextArea()| Fine we will keep it then. I see it is already being used now in other code. ==== The comment in |GetFirstNode()| isn't exactly correct. It currently says: Removed comment. I can't even remember if i wrote it. *sigh*. But yep i fully understand the text and br node dichotomy. ==== Also in |GetFirstNode()|, the length of |childNodeList| is retrieved but never used. Right, that is something i forgot to remove. Gone. ==== I really don't think there's a need for a |GetFirstTextNode()|. The only real difference between it and |GetFirstNode()| is a |QueryInterface()|, and besides the only method that calls |GetFirstTextNode()| is |SetSelectionEndPoints()| which also calls |GetFirstNode()| also? I don't think i wrote GetFirstTextNode. Did I? Anyway It's gone now. Bye. ==== There also needs to be some discussion of what exactly is expected to happen in certain situations, for example when the caller sets an end point that is before the selection start, or a start point which is after the current selection end. Right now both cases are handled inconsistently, the first case basically sets a selection that leaves the end point at a different place, than the user just set, and the latter case, just throws an error. Personally, I think that in both situations we should just collapse the selection to the point being set. This is dealing w/ an edge case. So i suggest we ignore it in this bug and file a meta bug after this code is checked in. Right now the code currently in the trunk is horribly broken. After we act expeditiously and get this patch in, an edge case may behave inconsistently. No big deal, it doesn't warrant this code from not going in now. ==== |SetSelectionEndPoints()| should never place an endpoint to the right of a br node that is the last child of the div. ????? I lost you on this one. ==== In |GetTargetNode()| we get the parent of |aFirstNode|, why do we need to ask if |parent| has children? Gone. Some more extraneous cruft from moving code around. ==== |GetSelectionRange()| probably shouldn't differenciate between single line text controls and textareas for the reason I said above regarding some platforms with multi line text in single line text controls. Also, I see this |if| construct, when would the |else| ever be used? Gone, we are operating only on textareas. I am not touching the original code past else and risk a regression somewhere. Perhaps that can be removed at a later date. ==== setSelectionRange() needs to be added to dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl. Nope not in the scope of this RFE. I'm not interested in adding it right now. I really don't have the time. Perhaps post 1.2. ==== SetSelectionRange() needs the following line removed otherwise, it won't work with textareas: if (!IsSingleLineTextControl()) return NS_ERROR_NOT_IMPLEMENTED; Leaving for now. Tha can be another bug. RFE implement setSelectionRange for nsIDOMNSHTMLTextAreaElement interface. ==== Because we are currently concerned about code bloat, I would remove all of your debug methods which are not used: Gone. If i can get an r and sr on this patch i am certain i can get an a=. I tested this impl very thoroughly and it is solid w/ exception to a single edge case mentioned above. People need this implementation now rather than later. Please review ASAP. Updated patch forthcoming. Thanks --pete

[RFE] is deprecated in favor of severity: enhancement. They have the same meaning.

>==== |SetSelectionEndPoints()| should never place an endpoint to the right of a >br node that is the last child of the div. > >????? I lost you on this one. The deal there is that a selection that is after a br will cause any typed text to appear on a new line. But in the case of a br that is the last child of a div, there *is no new line*. So the caret will appear at the end of the last line, and then if the user types, the text will mysteriously appear on a newly created line below. If you catch this case and instead set the selection to before the br, the caret will look identical (at the end of the last line), but now typed text will be before the br, and hence on that same line, where the user expects. >==== There also needs to be some discussion of what exactly is expected to >happen in certain situations, for example when the caller sets an end point >that is before the selection start, or a start point which is after the current >selection end. Right now both cases are handled inconsistently, the first case >basically sets a selection that leaves the end point at a different place, than >the user just set, and the latter case, just throws an error. Personally, I >think that in both situations we should just collapse the selection to the >point being set. > >This is dealing w/ an edge case. So i suggest we ignore it in this bug and file >a meta bug after this code is checked in. Right now the code currently in the >trunk is horribly broken. After we act expeditiously and get this patch in, an >edge case may behave inconsistently. No big deal, it doesn't warrant this code >from not going in now. The question is, why not get this correct now? Is it hard to do? If it's not hard then it doesn't really matter whether the current patch is way better than nothing: we should fix any outstanding issues now while we have focus on them.

> now while we have focus on them Joe, I'm trying to be realistic here. I submitted the first patch for this back in April. It is now November. Some contributors just don't have an abundant amount of time to devote. I'm sure every one cc'd on this bug would rather have running code than broken code. I'm also for a "divide and conquer" approach here. Meaning, check in solid runing code sooner rather than later. Then meta issues become more manageable and smaller to deal w/ another time another pass. I would be much more inclined to fix an edge case next time around. Right now, since it has beed almost a month since I've had an official response to the last patch i posted, i'm sure it will fail now on a merge. Where as if the code was checked in already, and i had extra time, I can fix the specific edge case in question which is fundamentally trivial. Gice me the essentials here for me to check this in sooner rather than later. --pete

Reason for blockage change: I have confirmed by looking at the source code that this bug blocks bug 58850, not the other way around. Incidentally, bug 58850 is rated blocker. So although this is rated enhancement, I recommend bumping up the severity level a great deal. We need eyeballs on this bug to get Mr. Collins some reviews, patch updates against bit rot and to get it fixed yesterday.

I agree. Pete has had this work done for a while now, we need to get him some feedback so that he can fix whatever the outstanding issues are and get it approved and checked in. I wish I could help more, but I'm not a C/C++ hacker (it's been a loooong time). I do supply beer though (ask Pete, he knows I'm good for it). Ok, so I guess I'll offer another case of 'beer of your choice' for anyone that can assist Pete in getting this all the way through the approval process and checked in (although I'm not hopeful for 1.2 anymore :-(). Cheers, - Bill

Comment on attachment 102464 [details] [diff] [review] killed tabs I got up to |GetPositionFromText()| in the diff, these are my notes so far: ==== In |GetFirstNode()| you call |GetChildNodes()| to retrieve a list of the children, just to answer the question, "Does it have children?" You can avoid a malloc and some extra work that happens behind the scenes by simply calling |HasChildNodes()|. ==== If the first line of the textarea is blank, won't this code cause |SetSelectionEndPoints()| to return pre-maturely since the QI of a br node to chardata will probably fail with an NS_ERROR_NO_INTERFACE? + nsCOMPtr<nsIDOMNode> firstNode; + nsresult rv = GetFirstNode(getter_AddRefs(firstNode)); + if (NS_FAILED(rv)) return rv; - nsCOMPtr<nsIDOMNode> firstNode = do_QueryInterface(firstTextNode, &rv); - if (!firstNode) return rv; + nsCOMPtr<nsIDOMCharacterData> firstTextNode(do_QueryInterface(firstNode, &rv)); + if (NS_FAILED(rv)) return rv; ==== The |if (aSelStart != eIgnoreSelect && aSelEnd != eIgnoreSelect)| block in |SetSelectionEndPoints()| will need to be reworked to support textareas since it assumes that the entire contents of the widget is in a single text node. ==== If we ever hit the |else| clause, |firstNode| should be a br node which is *not* a container, so the |SetStart()| and |SetEnd()| calls should throw errors since the br is not a container. That would probably mean we are adding a bogus range to the selection at that point. + if (firstTextNode) { selectionRange->SetStart(firstTextNode, aSelStart); selectionRange->SetEnd(firstTextNode, aSelEnd); + } else { + selectionRange->SetStart(firstNode, aSelStart); + selectionRange->SetEnd(firstNode, aSelEnd); + } ==== |mStart| is never set if both endpoints of the selection are specified, for example via a call to |SetSelectionRange()|, this can yield unexpected results if the caller ever tries to extend the selection with a call to |SetSelectionEnd()| without a prior call to |SetSelectionStart()|. As mentioned in my previous review, I think it's a bad idea to try and track an |mStart| between |SetStartSelection()| and |SetEndSelection()| calls, especially given the fact that the user can easily throw the saved state off by simply clicking in the text widget. ==== I mentioned this in the previous review, |targetSelect = aSelEnd;| shouldn't be necessary since the code immediately preceding this already does |targetSelect = aSelEnd| when |aSelStart == eIgnoreSelect|. + // if end is less than start flip em + if (aSelStart == eIgnoreSelect && mStart > aSelEnd) { + targetSelect = aSelEnd; + flip = PR_TRUE; } ==== There's potential for using a br node as the container arg to these |SetStart()| and |SetEnd()| calls in the following code. Also there is no checking being done to see if |firstRange| is already positioned. If it is, you'll need to make sure that you aren't calling the set methods with a start that is after the current end, and an end that is before the current start, otherwise the range will throw an error. + // for single line text controls + if (targetSelect <= PRInt32(firstNodeLen) && mStart <= PRInt32(firstNodeLen)) { + if (aSelStart != eIgnoreSelect) + firstRange->SetStart(firstNode, targetSelect); + else if (!flip) + firstRange->SetEnd(firstNode, targetSelect); + else { + // we are flipping values so set start here + firstRange->SetStart(firstNode, targetSelect); + firstRange->SetEnd(firstNode, mStart); + } ==== I'm guessing that we need to check to make sure that the set calls below don't trigger an error by setting a start after the current range end, and an end before the current range start: + } else { // for multiline text controls + PRInt32 pos(0); + nsCOMPtr<nsIDOMNode> targetNode(nsnull); + GetTargetNode(firstNode, targetSelect, getter_AddRefs(targetNode), &pos); + if (!targetNode) + return NS_OK; + if (aSelStart != eIgnoreSelect) + firstRange->SetStart(targetNode, pos); + else if (!flip) + firstRange->SetEnd(targetNode, pos); + else { + // we are flipping values so set start here + firstRange->SetStart(targetNode, pos); + // using mStart here because it is greater than aSelEnd + GetTargetNode(firstNode, mStart, getter_AddRefs(targetNode), &pos); + firstRange->SetEnd(targetNode, pos); + } + } ==== Why would we zero out |mStart| if we've flipped values? Shouldn't that be set to whatever |targetSelect| was? + if (flip) + mStart = 0; + ==== I try to discourage using this pattern |if (NS_FAILED(rv) || !nodeList) return rv;| pattern, if |rv| really was NS_OK, and we had |!nodeList|, then we would return NS_OK to the caller without ever setting |*aResult|. It might be a good idea to initialize |*aResult| ahead of time. + rv = aDiv->GetChildNodes(getter_AddRefs(nodeList)); + if (NS_FAILED(rv) || !nodeList) return rv; + + if (aOffset == 0) { + *aResult = 0; + return NS_OK; + } ==== Change the following to |nsCOMPtr<nsIDOMText> domText(do_QueryInterface(item));| + nsCOMPtr<nsIDOMText> domText; + domText = do_QueryInterface(item); ==== Both |GetPositionFromDiv()| and || use this same |if| pattern mentioned above + if (NS_FAILED(rv) || !nodeList) return rv; + if (NS_FAILED(rv) || !item) return rv; ==== |GetPositionFromDiv()| can return without ever setting |*aResult| if the div has no children. ==== We should make this an |if (br) ... else| or call |continue| if we have a br to avoid an unnecessary QI and check. Also change |domText| QI to |nsCOMPtr<nsIDOMText> domText(do_QueryInterface(item))|: + nsCOMPtr<nsIDOMHTMLBRElement> br(do_QueryInterface(item)); + if (br) + ++textOffset; + nsCOMPtr<nsIDOMText> domText; + domText = do_QueryInterface(item); + if (domText) {

Cool, some traction. I'm pulling and building the trunk now. I'll go over this review tomorrow. Thanks Kin --pete

Comment on attachment 102464 [details] [diff] [review] killed tabs Here's the rest of my review ... ==== Remove the blank line at the start of |GetTargetNode()|. ==== |GetTargetNode()| can return an |aResult| that is set to a br, which means the callers of this method will try to set one of the range endpoints to (br, pos) which is wrong. I think if you have a br node you should be returning the br's parent and the offset (position) of the br under that parent. Also as I mentioned in a previous review, you need to make sure you don't return an (aResult, pos) that is to the right of a br which is the last child of the text widget. ==== |GetTargetNode() also uses that |if| pattern in a couple of places, that can cause us to return NS_OK without ever initializing the return values: + if (NS_FAILED(rv) || !parent) return rv; + if (NS_FAILED(rv) || !item) return rv; ==== In |GetTargetNode()| if |targetNode| is ever null that means an offset was specified that was greater than the length currently in the text widget ... do you want to return an error? Or at least return the root and an offset that is equivalent to the end of the content (keeping in mind what I said above about brs that are the lastchild)? Right now we will return NS_OK without ever initializing |aResult| or |aPosition|. ==== Change the following in |GetTargetNode()| to |nsCOMPtr<nsIDOMText> domText(do_QueryInterface(item))|: + nsCOMPtr<nsIDOMText> domText; + domText = do_QueryInterface(item); ==== I mentioned this in a previous review ... |GetSelectionRange()| probably shouldn't differenciate between single line text controls and textareas because some platforms can have multi line text in single line text controls. You should be able to get by with just the code you added which is currently in the |IsMultiLineTextControl()| case. + if (IsSingleLineTextControl()) { ... + } else if (IsMultiLineTextControl()) { ... + } else { // multiline ==== I don't like relying on a QI to tell us if a node is the root, since that could change someday. We should probably fetch the root node like you did in |GetFirstNode()| and compare the start and end nodes against that. + nsCOMPtr<nsIDOMHTMLDivElement> div(do_QueryInterface(startNode)); ==== It would be nice if |GetPositionFromDiv()| and |GetPositionFromText()| could be merged into one method named something like |DOMPointToOffset()| since they are so similar. Also |GetTargetNode()| could be the inverse |OffsetToDOMPoint()|.

Ok, Kin. This should be solid. I implemented SetSelectionRange() and posted a good test page to try and break this code. Can you please give this a last pass? Thanks --pete

Kin, some notes on the latest patch. 1. Tracking saved states w/ mStart and mEnd seems to work nicely. The code deals w/ a click that can occur between setSelectionStart and setSelectionEnd despite which one is called first. 2. The code properly handles any combination of values that can be throw at it. 3. As far as the br nodes go, I think I do the right thing where the callers of OffsetToDOMPoint check for a br element and use SetStartBefore. It's hard for me to visualize since i can't seem to produce a case on the test page that will break it. 4. I renamed the methods as you suggested but I'll pass on combining the two for now. I hope we can shoot this pig into the air soon. ;-) --pete

Updating milestone (1.0.1 is already gone). Probably too late to make Mozilla 1.2, adding 1.3 keyword instead. I'd like to nominate for 1.0.2 as well.

Can we get a little love on this code? It has been three weeks since I posted the patch here looking for a final review. Thanks --pete

Comment on attachment 108870 [details] [diff] [review] maintenence patch (needed a couple of merges) jst, Kin, going through the request tracker. I need review and super review. Thanks --pete

Comment on attachment 108870 [details] [diff] [review] maintenence patch (needed a couple of merges) - In nsHTMLTextAreaElement::SetSelectionRange(): + nsCOMPtr<nsIFormControlFrame> formControlFrame = getter_AddRefs(GetFormControlFrame(PR_TRUE)); No need to use an nsCOMPtr here, nsIFrame n' friends are not ref-counted. + nsCOMPtr<nsITextControlFrame> + textControlFrame(do_QueryInterface(formControlFrame)); Same here, but it's kinda nice to use nsCOMPtr here to hide the call to QI. Other than that, the changes to the DOM code is fine, so r=jst on that, but I'd like rods to have a look at the frame code, so forwarding the review request to him.

Removed refcounting frames. --pete

setSelectionRange is in nsIDOMNSHTMLTextAreaElement.idl now where i ment to put it. --pete

Evan Williams of Blogger fame wrote the following: "Hi, Mark. We're running into a bug that could be pretty major for us (and make me take back that "preferred browser" statement). Do you know what the status is on this? http://bugzilla.mozilla.org/show_bug.cgi?id=88049 Thanks, Ev. " He wants to optimize the new version of blogger to work with Mozilla but apparently this is holding him back.

Thanks guys, let's not let this patch die on the vine here. --pete

added nsbeta1 and topembed as it's a blocker for Blogger.

Nominating for blockage of 1.3 beta (as in this bug should be one of those blocking a "Make Mozilla 1.3 beta not suck" bug). Also removing 1.0.2 keyword, as we can pretty much kiss that one goodbye.

Accidentally removed keywords, midair collision. Sorry for bugspam.

per describekeywords.cgi

Comment on attachment 109078 [details] [diff] [review] placed setSelectionRange in the correct interface This patch still has problems related to the fact that it stores state (mStart and mEnd). In the interest of expediancy I spent about 45 minutes rewriting some portions of the patch to both simplify and fix the problems I noted. I'll post it tonight, if I can, or tomorrow morning. jkeiser has agreed to do the review as soon as I post it. Assuming we get reviews in order, we should have something for checkin by some time tomorrow.

> This patch still has problems related to the fact that it stores state (mStart and mEnd) Real actual problems that you can reproduce an error? Or theoritical problems? I tested this patch out to a great extent and like I mentioned above, but I couldn't "break" the implementation. --pete

Differences between this patch and the previous patch: * Modified |SelectAllContents()| to use nsIEditor::SelectAll() in both the single line text control and textarea case. * Removed GetFirstTextNode(). * Simplified |SetSelectionEndPoints()|. You are now required to specify both selection end points. We no longer maintain |mStart| and |mEnd| state. * Modified |SetSelectionStart()| and |SelectionEnd()| to call |GetSelectionRange()| and collapse the selection should the new index result in the range |IsIncreasing| rule to be broken. * Modified |SetSelectionRange()| so that it mimics the behavior specified in item #4 above. * Merged |DomPointToOffsetDiv()| and |DomPointToOffsetText()| into one method |DOMPointToOffset()|. * Modified |OffsetToDOMPoint()| to return DOMPoints in all cases, that is it never returns a br as a result node. * Removed the enums |eIgnoreSelect| and |eSelectToEnd| because they are no longer needed. * I didn't make any modifications to the content code that jst already reviewed, except for an indentation fix in one of the idl files. I did notice that while testing the previous patch, errors were not being propogated out of the content methods so you couldn't really tell at the JS level if things worked or not. I personally wouldn't have used COMPtrs for things known to be un-refcounted but it looks like pre-existing code does it anyways. To answer pete's questions above: > Real actual problems that you can reproduce an error? Yes, if there is an existing uncollapsed selection, Calling |SetSelectionStart()| with an index that places it after the current end yields an error that doesn't get propogated out to JS. Nothing gets done, but the new index is still recorded in mStart. You don't get the desired result until a |SetSelectionEnd()| is called. The reverse case using |SetSelectionEnd()| is also true. In short the fact that you keep state requires that you use |SetSelectionStart()| and |SetSelectionEnd()| in pairs to get the correct results ... if that's the case then why ever use them if we have |SetSelectionRange()|? The fact that the user can throw off the saved state |mStart| and |mEnd| by simply making a selection elsewhere in the textarea, either by clicking or through typing, is also problematic. If you can imagine someone using |SetSelectionStart()| to select backwards from the current caret position to a word boundary to perform an edit, and then later in the editing session, somewhere else in the doc, using |SetSelectionEnd()| to select forward to the next boundary, what they would get selected is not what the user would expect. > Or theoretical problems? I suppose everything is theoretical at this point since no one is really using these methods except for auto complete. The bottom line, is we shouldn't be saving any state between SetSelection* calls, they will just result in bugs being filed, probably against me, in the future, so I'd rather just make things work consistently now, and work with whatever the current selection is.

Comment on attachment 109768 [details] [diff] [review] Modified version of Pete's patch. (Rev 1.0) Looks mostly good. A number of nits and one thing that looks very much like a logic error to me (#6). 1. instead of adding getter_AddRefs to GetFormControlFrame, could you not assign them to nsCOMPtr at all? I wouldn't mind if you left them the same, but since you're changing them ... SetSelectionRange has several new frame nsCOMPtrs too. 2. Both of these are not necessary (either an assertion or an ensure_true will do). I personally prefer just the assertion; we shouldn't waste time in opt builds verifying contract when all the callers are internal. SetSelectionEndPoints does this too. NS_ASSERTION(mEditor, "Should have an editor here"); NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_INITIALIZED); 3. There are several "nsresult result"s in there. nsresult rv for consistency with the other nsresults, por favor. 4. nsCOMPtr<nsIDOMHTMLBRElement> ... You might want to consider just checking if (domText) { } else { } instead of also checking BR. You will have slightly fewer missed QI's that way and a lot fewer QI calls; and you could get rid of #include "nsIDOMHTMLBRElement.h". Your call though; I don't have strong feelings on it since QI hits are not too expensive. Your way allows more error checking at least. (If you go with it, put the error check in both OffsetToDOMPoint and DOMPointToOffset, if you could--we'll catch more stuff that way.) 5. This if (NS_SUCCEEDED()) should really be an NS_ENSURE_SUCCESS. We should not fall through to a happy case after an exception is thrown. PRUint32 textLength(0); if (NS_SUCCEEDED(domText->GetLength(&textLength))) { 6. Looks like an off-by-one error to me (should be aOffset < count+(PRInt32)textLength): // check if aOffset falls within this range if (aOffset >= count && aOffset <= count+(PRInt32)textLength) { With this code the DOM node will be the wrong node, and the position in the node will be invalid (position == textLength). Also in these places: NS_ASSERTION((aNodeOffset >= 0 && aNodeOffset <= textLength), 7. NS_ENSURE_ARG_POINTER for an internal interface is not necessary--assertion instead, please, opt builds shouldn't pay. This happened in multiple places. NS_ENSURE_ARG_POINTER((aSelectionStart && aSelectionEnd));

> 1. instead of adding getter_AddRefs to GetFormControlFrame, could you not > assign them to nsCOMPtr at all? I wouldn't mind if you left them the same, > but since you're changing them ... SetSelectionRange has several new frame > nsCOMPtrs too. I removed the nsCOMPtr references in the selection methods in both nsHTMLTextAreaElement.cpp and nsHTMLInputElement.cpp. I also made those methods propogate any errors out to the caller. > 2. Both of these are not necessary (either an assertion or an ensure_true will > do). I personally prefer just the assertion; we shouldn't waste time in opt > builds verifying contract when all the callers are internal. > SetSelectionEndPoints does this too. > > NS_ASSERTION(mEditor, "Should have an editor here"); > NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_INITIALIZED); It turns out that |SelectAllContents()| is only called from one place, so there is no need to have a one line utility function, so I removed it and made the |mEditor->SelectAll()| call directly from the caller. Note that the check for a null mEditor is still necessary since the function can be triggered before an editor is ever created. > 3. There are several "nsresult result"s in there. nsresult rv for consistency > with the other nsresults, por favor. All changed to |rv|. > 4. nsCOMPtr<nsIDOMHTMLBRElement> ... > You might want to consider just checking if (domText) { } else { } instead of > also checking BR. You will have slightly fewer missed QI's that way and a lot > fewer QI calls; and you could get rid of #include "nsIDOMHTMLBRElement.h". > Your call though; I don't have strong feelings on it since QI hits are not too > expensive. Your way allows more error checking at least. (If you go with it, > put the error check in both OffsetToDOMPoint and DOMPointToOffset, if you > could--we'll catch more stuff that way.) Heh, I was on the fence about re-ordering the checks when originally reworking the patch for exacty the reason you mentioned, but I also liked that fact that we were checking the node types. The current reallity is that the editor controls what content gets placed under the div, and it can only be a text or a br node, so I've re-ordered things as you mentioned above to reduce things to one QI. > 5. This if (NS_SUCCEEDED()) should really be an NS_ENSURE_SUCCESS. We should > not fall through to a happy case after an exception is thrown. > PRUint32 textLength(0); > if (NS_SUCCEEDED(domText->GetLength(&textLength))) { Agreed, done. > 6. Looks like an off-by-one error to me (should be aOffset < > count+(PRInt32)textLength): > // check if aOffset falls within this range > if (aOffset >= count && aOffset <= count+(PRInt32)textLength) { > > With this code the DOM node will be the wrong node, and the position in the > node will be invalid (position == textLength). Also in these places: > > NS_ASSERTION((aNodeOffset >= 0 && aNodeOffset <= textLength), Actually there is no error in either of those cases. Having a DOMPoint of (textNode, textNodeLength) is legitimate and simply refers to the point under the text node that is after the last character. Plaintext edits done with a selection set with a DOMPoint like this: <div><textnode>text|</textnode><br></div> will yield the same results as a point like this would: <div><textnode>text</textnode>|<br></div> Also, note that in a flattened string context which we are trying to emulate, both of the points illustrated above would map to the same text offset of 4. > 7. NS_ENSURE_ARG_POINTER for an internal interface is not necessary--assertion > instead, please, opt builds shouldn't pay. This happened in multiple places. > > NS_ENSURE_ARG_POINTER((aSelectionStart && aSelectionEnd)); > Actually the method in question here is |GetSelectionRange()| which is a public API method which is called from outside the layout dll. We should be checking all incoming pointers in public methods no?

Comment on attachment 109856 [details] [diff] [review] Modified version of Pete's patch (Rev 1.1) GetSelectionRange is in a "public" API, but it is not really a public API. It is used only in Mozilla, and I still don't feel we should protect ourselves from ourselves when it comes to passing null pointers in to be filled in. We can run in debug builds and deal with crashes ourselves. In opt builds we should *not* be verifying that contract is fulfilled unless it's actually used by a web developer or maybe is in an embedding API. nsTextControlFrame and its attendant interfaces is neither of those. GetSelectionStart() and GetSelectionEnd(), which *are* public APIs, check their arguments, which is fine. Everything else looks good; no need to hold up the process for this, you can change before checkin or convince me on AIM how silly I'm being :)

The evangelism team is in touch with blogger.com, (see commment #125) and all of us would love that bug to be fixed. All we need is a super review!

I just had some time play w/ this. If you enter a start value greater than the end value, it doesn't flip them. eg: (length is 43) start 40 end 34 Doesn't create the selection 34 -> 40. This is something I had working and as i understand it, was a requirement. Other than that, it seems to work good. --pete

Ah, back from vacation ... I've removed: > NS_ENSURE_ARG_POINTER((aSelectionStart && aSelectionEnd)); in my local tree at jkeiser's request. As for not being able to enter a start value greater than the end value, that was intentional ... I was trying to make it so that you would get predictable selection behavior, no matter what the order used to set the end points was. Using your example above, if we allowed automatic swapping of values: start 40 end 34 would yield 34->40, but what if the user then tried to set the selection to: start 41 end 43 The setting of start would yield 40->41 with the setting of end yielding a 40->43 instead of the intended 41->43. The way it is implemented in the Rev1.1 patch, if you specify a start > end or an end < start, the selection is collapsed to the point you specified.

> I was trying to make it so that you would get predictable > selection behavior, no matter what the order used to set the end points was. That makes it inconsistent w/ the behavior w/ nsIDOMNSHTMLInputElement.idl selectionStart/End will/did reverse the values. > but what if the user then tried to set the selection to: > start 41 end 43 That is a new data pair and is irrelevent to the previous flipped pair. --pete

Discussed in bug triage meeting. Plussing.

> That makes it inconsistent w/ the behavior w/ nsIDOMNSHTMLInputElement.idl > selectionStart/End will/did reverse the values. I'm not sure what you are referring to in that interface file? There aren't any comments in there that mention this auto-swapping. Are you referring to the convenience method setSelectionRange()? I think it would be a bit disorienting to some people if they set selectionStart to a value, and then retrieved it's value and found that it was different. The way it's implemented in the patch, you set a value, that's what you get (minus the cases where negative values and values > than the content length are specified). > That is a new data pair and is irrelevent to the previous flipped pair. All I was trying to say was this, both startSelection and endSelection can be set independently of each other, and when they are set, they will have to interact with the selection's current start and end points. If we allowed auto-swapping when setting selectionStart, we would have to check the current selection end point, and swap if necessary ... but that means that if a selectionEnd were set immediately afterwards, we wouldn't necessarily get what we would expect because selectionStart might not be the value we just set. Now if you are trying to argue that the convenience function setSelectionRange() should do swapping, I can understand that since it is effectively replacing the entire selection, but that would mean you could get different results between using setSelectionRange() and setting both selectionStart and selectionEnd. As it is currently in the patch, setSelectionRange() acts as if the user set selectionStart and then setSelectionEnd, which was what I thought was the whole point of that convenience function.

> Discussed in bug triage meeting. Plussing. Never has a single ascii character brought more joy to my life than that '+'. The only thing that could make it better now would be if I could see the characters 'S' and 'R' together on this bug. (Yeah, this is bugspam, I know...)

Kin, my point is a simple one, in the existing implementation of nsIDOMNSHTMLInputElement selectionStart/End, paired values are indeed flipped. Not flipping them in nsIDOMNSHTMLTextAreaElement is an inconsistency despite the fact that there is nothing documented in the interface files. I personally don't care. I only implemented "flipping" in the later patches to appease sentiment in this bug "why not get this correct now?". So now i'm pointing this same fact out to you. In any case I think a lot of people will be happy to just see this code checked in already. --pete

Comment on attachment 108870 [details] [diff] [review] maintenence patch (needed a couple of merges) Patch is obsolete

Patch Rev 1.1 (minus the NS_ENSURE_ARG_POINTER() macro jkeiser requested) landed on TRUNK: mozilla/content/html/content/src/nsHTMLInputElement.cpp revision 1.280 mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp revision 1.120 mozilla/dom/public/idl/html/nsIDOMNSHTMLTextAreaElement.idl revision 1.7 mozilla/layout/html/forms/src/nsTextControlFrame.cpp revision 3.111 mozilla/layout/html/forms/src/nsTextControlFrame.h revision 3.46

FIXED

Verified per testcase (attachment 106596 [details] )

Just trying to hack together some quick editor buttons (similar to that of blogger and movable type) at hit some strange behavior (that is available in the current testcase). It seems that when a user highlights all the text with the mouse selectionEnd == length, however when selecting all text with CMD/CTRL-A selectionEnd == length + 1. While inserting text at this higher value seems to work properly, it screws with resetting focus in the following example: http://placenamehere.com/Mozilla/js_textareas.html You'll notice that if you select all the text in the textarea via CMD-A and then hit one of the editor buttons it throws off my calculation for the new selectionEnd and grabs the first character in the newly inserted tag. My Build: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.3b) Gecko/20030122 While I can easily test that the selection isn't longer the then length in my script to get it to work, should I have to? Is there a reason for this behavior? [And should this be a new bug altogether? This one is rather busy]

Chris, file a new bug and assign it to me.

Done: Bug 190382 [sorry for the spam everyone]

*** Bug 75629 has been marked as a duplicate of this bug. ***