88049 - Support .selectionStart & friends for textareas

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Alex Vincent [:WeirdAl]]

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?  ;)

Comment 1

[Kathleen :Brade]

.caretPosition seems better to me.

Can you just grab the selection instead?  Is this really necessary?

Comment 2

[Tim Powell]

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.

Comment 3

[Alex Vincent [:WeirdAl]]

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 |.  :)

Comment 4

[Kathleen :Brade]

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.

Comment 5

[Jesse Ruderman]

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

Comment 6

[Alex Vincent [:WeirdAl]]

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.

Comment 7

[Alex Vincent [:WeirdAl]]

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

Comment 8

[Johnny Stenback (:jst)]

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

Comment 9

[Tim Powell]

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

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[Alex Vincent [:WeirdAl]]

Please.  :)

Comment 12

[Johnny Stenback (:jst)]

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

Comment 13

[Jesse Ruderman]

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

Comment 14

[Fabian Guisset]

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

Comment 15

[Pete Collins (MDG)]

Ok, i just implemented this because i needed it.

Stealing from jst

patch forthcoming.

--pete

Comment 16

[Pete Collins (MDG)]

Attachment: patch to implement textLength, startSelection and endSelection for textarea widget

Comment 17

[Pete Collins (MDG)]

Cathy, jst does 1.01 sound reasonable to land this?

--pete

Comment 18

[Aaron Andersen]

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

Comment 19

[Fabian Guisset]

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()

Comment 20

[Fabian Guisset]

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

Comment 21

[Pete Collins (MDG)]

> 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

Comment 22

[Pete Collins (MDG)]

Attachment: revised patch

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

Comment 23

[Tim Powell]

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 24

[Fabian Guisset]

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;

Comment 25

[Pete Collins (MDG)]

cc'ing jst. 

--pete

Comment 26

[Johnny Stenback (:jst)]

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...

Comment 27

[Pete Collins (MDG)]

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

Comment 28

[Johnny Stenback (:jst)]

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

Comment 29

[Pete Collins (MDG)]

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 

Comment 30

[Chris Casciano]

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.

Comment 31

[Pete Collins (MDG)]

Attachment: revised patch

Comment 32

[Pete Collins (MDG)]

Attachment: ignore the previous patch, it was the wrong one

Comment 33

[Pete Collins (MDG)]

Attachment: ignore previous again! (i need to clean out my patches dir)

Comment 34

[Pete Collins (MDG)]

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


--pete

Comment 35

[Aaron Andersen]

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).

Comment 36

[Pete Collins (MDG)]

Yep works fine. No patch rot yet.  

I just don't have the time to chase down reviewers.

--pete

Comment 37

[Aaron Andersen]

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...

Comment 38

[Johnny Stenback (:jst)]

I can sr once someone has reviewed, bz?

Comment 39

[Boris Zbarsky [:bzbarsky]]

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....

Comment 40

[Pete Collins (MDG)]

Attachment: Agree, all i care about here is if it's a textarea

Added IsTextArea() method.

--pete

Comment 41

[Pete Collins (MDG)]

> 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 42

[Boris Zbarsky [:bzbarsky]]

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();

Comment 43

[Pete Collins (MDG)]

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

Comment 44

[Peter Van der Beken [:peterv]]

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.

Comment 45

[Pete Collins (MDG)]

Attachment: er, w/ the correct implementation for the setters . . .

Comment 46

[Pete Collins (MDG)]

Attachment: this would be the correct patch

I posted the wrong patch from the wrong machine.  

--pete

Comment 47

[Pete Collins (MDG)]

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

Comment 48

[Peter Van der Beken [:peterv]]

    * 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 49

[Pete Collins (MDG)]

Attachment: fixed comment past

Comment 50

[Boris Zbarsky [:bzbarsky]]

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...

Comment 51

[Aaron Andersen]

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

Comment 52

[Johnny Stenback (:jst)]

Seems unlikely at this point.

Comment 53

[Johnny Stenback (:jst)]

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.

Comment 54

[Alex Vincent [:WeirdAl]]

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

Comment 55

[Pete Collins (MDG)]

Attachment: for sr=

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

Comment 56

[Pete Collins (MDG)]

Attachment: page for testing

Comment 57

[Johnny Stenback (:jst)]

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

sr=jst

Comment 58

[Pete Collins (MDG)]

Checked into the trunk.

I heard rumors about a possible branch landing?


--pete

Comment 59

[kinmoz]

@@ -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.

Comment 60

[Pete Collins (MDG)]

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

--pete

Comment 61

[Pete Collins (MDG)]

Attachment: (patch in progress) Implementation for selectionStart/End

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

Comment 62

[Pete Collins (MDG)]

Attachment: test page

Comment 63

[Pete Collins (MDG)]

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

Comment 64

[Pete Collins (MDG)]

###!!! 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

Comment 65

[Michael Stearne]

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.

Comment 66

[Pete Collins (MDG)]

Right, i am almost finished implementing setSelection.

patch #98747 has the correct implementation for getSelection.


--pete

Comment 67

[kinmoz]

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.

Comment 68

[Pete Collins (MDG)]

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

Comment 69

[kinmoz]

> 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.

Comment 70

[Pete Collins (MDG)]

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

Comment 71

[Pete Collins (MDG)]

Attachment: implements getter and settter selectionStart/selectionEnd

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

Comment 72

[Pete Collins (MDG)]

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

--pete

Comment 73

[Pete Collins (MDG)]

Attachment: ok, this should do it.

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

Comment 74

[Pete Collins (MDG)]

Attachment: Patch for review and super review

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

Comment 75

[Tony Gorman]

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.

Comment 76

[Tony Gorman]

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.

Comment 77

[Pete Collins (MDG)]

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

Comment 78

[Tony Gorman]

Ooops - missed that attachment.

Will look again once its s + sr

Comment 79

[Pete Collins (MDG)]

Attachment: very minor tweak. Patch for review and super review

Comment 80

[kinmoz]

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.

Comment 81

[rbs]

"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.

Comment 82

[rbs]

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.

Comment 83

[Pete Collins (MDG)]

Attachment: started refactoring setter so it will be general purpose

started refactoring setter so it will be general purpose and work on single or
multiline controls.

Not for review. 

--pete

Comment 84

[Martin Honnen]

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.

Comment 85

[Pete Collins (MDG)]

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

Comment 86

[William J. Edney]

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
- [e-mail]

Comment 87

[Pete Collins (MDG)]

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

Comment 88

[William J. Edney]

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

Comment 89

[Pete Collins (MDG)]

mmmmm, beer.


--pete

Comment 90

[Michael Stearne]

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

Comment 91

[Pete Collins (MDG)]

Attachment: where i last left off. Updated for renamed files

Comment 92

[Alex Vincent [:WeirdAl]]

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

Comment 93

[Pete Collins (MDG)]

Attachment: almost there

Comment 94

[Pete Collins (MDG)]

Attachment: Getter now solid

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

Comment 95

[Pete Collins (MDG)]

Attachment: That was the wrong patch off a different machine.

Comment 96

[Pete Collins (MDG)]

Attachment: Very solid patch.

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

Comment 97

[Pete Collins (MDG)]

Attachment: Complete finished patch

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

Comment 98

[Pete Collins (MDG)]

Patch 99276 is ready for review.

Ping Kin or anyone who can review this code.

Thanks

--pete

Comment 99

[kinmoz]

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

Comment 100

[Pete Collins (MDG)]

Cool thanks Kin.


--pete

Comment 101

[kinmoz]

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);

Comment 102

[Pete Collins (MDG)]

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

--pete

Comment 103

[William J. Edney]

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

Comment 104

[Pete Collins (MDG)]

Bill, yea sorry. I've been swamped w/ work. Life and death stuff. :(

I'll try to make a pass tonight.

--pete

Comment 105

[Pete Collins (MDG)]

==== 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

Comment 106

[Pete Collins (MDG)]

Attachment: updated patch

Comment 107

[Pete Collins (MDG)]

Attachment: killed tabs

Comment 108

[Brant Gurganus]

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

Comment 109

[Joe Francis]

>==== |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.

Comment 110

[Pete Collins (MDG)]

> 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

Comment 111

[Alex Vincent [:WeirdAl]]

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.

Comment 112

[William J. Edney]

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 113

[kinmoz]

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) {

Comment 114

[Pete Collins (MDG)]

Cool, some traction.

I'm pulling and building the trunk now. I'll go over this review tomorrow.

Thanks Kin

--pete

Comment 115

[kinmoz]

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()|.

Comment 116

[Pete Collins (MDG)]

Attachment: new test page w/ setSelectionRange()

Comment 117

[Pete Collins (MDG)]

Attachment: revised patch w/ SetSelectionRange implemented

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

Comment 118

[Pete Collins (MDG)]

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

Comment 119

[Alex Vincent [:WeirdAl]]

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.

Comment 120

[Pete Collins (MDG)]

Attachment: maintenence patch (needed a couple of merges)

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 121

[Pete Collins (MDG)]

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 122

[Johnny Stenback (:jst)]

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.

Comment 123

[Pete Collins (MDG)]

Attachment: Riding recent daily merges included jst's suggestion

Removed refcounting frames.

--pete

Comment 124

[Pete Collins (MDG)]

Attachment: placed setSelectionRange in the correct interface

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

--pete

Comment 125

[mark crane]

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.

Comment 126

[Pete Collins (MDG)]

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

--pete

Comment 127

[Susie Wyshak]

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

Comment 128

[Alex Vincent [:WeirdAl]]

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.

Comment 129

[Alex Vincent [:WeirdAl]]

Accidentally removed keywords, midair collision.  Sorry for bugspam.

Comment 130

[Alex Vincent [:WeirdAl]]

per describekeywords.cgi

Comment 131

[kinmoz]

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.

Comment 132

[Pete Collins (MDG)]

> 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

Comment 133

[kinmoz]

Attachment: Modified version of Pete's patch. (Rev 1.0)

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 134

[John Keiser (jkeiser)]

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));

Comment 135

[kinmoz]

Attachment: Modified version of Pete's patch (Rev 1.1)

> 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 136

[John Keiser (jkeiser)]

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 :)

Comment 137

[Tristan Nitot]

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!

Comment 138

[Pete Collins (MDG)]

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

Comment 139

[kinmoz]

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.

Comment 140

[Pete Collins (MDG)]

> 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

Comment 141

[Michael Buckland]

Discussed in bug triage meeting.  Plussing.

Comment 142

[kinmoz]

> 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.

Comment 143

[Aaron Andersen]

> 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...)

Comment 144

[Pete Collins (MDG)]

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 145

[Simon Fraser [no longer active]]

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

Patch is obsolete

Comment 146

[kinmoz]

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

Comment 147

[kinmoz]

FIXED

Comment 148

[Alex Vincent [:WeirdAl]]

Verified per testcase (attachment 106596 [details] )

Comment 149

[Chris Casciano]

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]

Comment 150

[kinmoz]

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

Comment 151

[Chris Casciano]

Done: Bug 190382

[sorry for the spam everyone]

Comment 152

[Simon Fraser [no longer active]]

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