299417 - Left/right arrow keys in textarea/text input behave incorrectly when text is selected.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Wevah (Nate Weaver)]

Select some text in a textarea or text input, and then press the left or right
arrow. If the text was selected by double/triple clicking or dragging
left-to-right, then the left arrow key deselects and moves the caret one
character to the left of the right end of the selection; the right arrow
deselects moves one character to the right of the right end of the selection.
(The behavior is relative to the left end if the selection was made right-to-left.)

The arrow keys should deselect the text but move to the exact left or right end
of the selection.

User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2)
Gecko/20050628 Camino/0.9a1+

Comment 1

[Andrew Schultz]

this regressed between linux trunk 2005012506 and 2005012601, perhaps bug 278197?

Comment 2

[Simon Fraser [no longer active]]

Regression from bug 207623?

Comment 3

[Mats Palmgren (inactive)]

The change was intentional, see bug 207623 comment 9 and bug 207623 comment 10.
Other applications on Linux and Windows have this behaviour.

I'm not near a MacOSX box right now, could you please try a few other native
applications and see what they do?

Comment 4

[Uri Bernstein]

(In reply to comment #3)
> The change was intentional, see bug 207623 comment 9 and bug 207623 comment 10.
> Other applications on Linux and Windows have this behaviour.
> 
> I'm not near a MacOSX box right now, could you please try a few other native
> applications and see what they do?

Both TextEdit and XCode behave like Mozilla used to until the change, i.e., the
selection is cancelled, and the caret is positioned at its right edge (for
right-arrow) or left edge (for left-arrow).

Comment 5

[Mats Palmgren (inactive)]

Ok, thanks.
Does the original problem reported in bug 207623 occur on MacOSX, in a build
prior to my checkin?

If not, the fix might be to resurrect the "if (!isCollapsed && !aContinue)"
block and wrap it in a #ifdef XP_MACOSX.

Comment 6

[Uri Bernstein]

(In reply to comment #5)
> Ok, thanks.
> Does the original problem reported in bug 207623 occur on MacOSX, in a build
> prior to my checkin?

Yes, the original 207623 problem does occur on OS X (I tested on the Firefox 1.0
release).

Comment 7

[Andrew Schultz]

> Other applications on Linux and Windows have this behaviour.

I see the new behavior in the main gedit window, but not in the gtk filepicker
or text boxes in gedit, gimp or gtk-demo, although editting in the "hypertext"
demo had the new behavior.  It seems to be gtk standard behavior in
textarea-like things, but not text boxes.

Comment 8

[Simon Fraser [no longer active]]

This issue should be fixed for Mac. It's the kind of thing that users comment on
with respect to Firefox being "non Mac-like".

Comment 9

[Uri Bernstein]

Given the fact that IE/Windows behaves the "old" Mozilla way in textareas and
input text fields, I respectfully disagree with the decision to change our
behaviour even on Windows. Most people migrating to Firefox are coming from IE,
and I think we should meet their expectations.
See also bug 263375, comments 1 and 2.

Comment 10

[Simon Fraser [no longer active]]

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

Comment 11

[Mats Palmgren (inactive)]

(In reply to comment #7)
> I see the new behavior in the main gedit window, but not in the gtk filepicker
> or text boxes in gedit, gimp or gtk-demo, although editting in the "hypertext"
> demo had the new behavior.  It seems to be gtk standard behavior in
> textarea-like things, but not text boxes.

Let's name the two types of caret positioning after the selection collapse:
"caret-style": the caret moves as it do when there is no selection.
"selection-style": the caret is placed at the selection edge.

caret-style is the current behaviour on all platforms,
which is obviously wrong for MacOSX.

===

On Linux it depends which desktop environment you are using.

I'm normally using a SuSE 9.3/KDE 3.4 desktop where almost all
applications I tested are using caret-style.
(Gimp was one notable exception though.)

I also tried Ubuntu 5.04 Live CD, which uses a Gnome Desktop and it
uses "selection-style".

Could we detect which environment we're in and change our behaviour
accordingly? (do we have code to do that already?)

=== 

The Windows XP platform behaviour seems to be caret-style.
IE6 text form controls is an exception that uses selection-style.
If you look at the rest of IE6 - url bar, Preferences, Find,
Save/Open and other dialogs etc they all use caret-style.
For now, I think we should leave as is (caret-style), even if that
differs to IE6. I'd much rather be coherent with the desktop at large
than a particular application. 

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 13

[Mats Palmgren (inactive)]

This is the underlying problem for bug 207623:

When we do a "select all" the resulting selection range is set up
as DIV:0/DIV:1 (anchor:pos/focus:pos).
Then, on VK_RIGHT, it's set to DIV:1/DIV:1.

This makes nsTextEditRules/nsPlaintextEditor confused when
it tries to determine wether it should allow text insertion
considering MAXLENGTH. Ad-hoc trace:

nsTextEditRules::WillDoAction kOutputText
nsPlaintextEditor::InsertText "a"
nsTextEditRules::WillDoAction kInsertText
nsTextEditRules::TruncateInsertionIfNeeded
mEditor->GetTextLength 3
startNode 0x8873b78 div 1
endNode 0x8873b78 div 1
mEditor->GetTextSelectionOffsets 0 3
resultingDocLength 0, aMaxLength 3

as you can see it allows the insert operation, resulting in:

nsTextControlFrame@0x85acbfc next=0x85b2384 {0,285,2700,345} [state=80c440b0]
[content=0x85ae0a8] [sc=0x85acb10]<
  HTMLScroll(div)(-1)@0x85b21cc [view=0x85b32b8] {30,45,2640,255}
[state=002c6010] [content=0x85aebe8] [sc=0x85acda0]<
    Area(div)(-1)@0x85b230c [view=0x85b33d0] {0,0,2640,255} [state=00d06008]
sc=0x85b2280(i=1,b=0) pst=:-moz-scrolled-content<
      line 0x85b23fc: count=2 state=inline,clean,prevmarginclean,not
impacted,not wrapped,before:nobr,after:nobr[0x8100] {0,0,480,255} <
        Text(0)@0x85b2478[0,3,T]  next=0x859d574 {0,0,360,255} [state=40024030]
sc=0x85b23d0 pst=:-moz-non-element<
          "LTR"
        >
        Text(1)@0x859d574[0,1,T]  {360,0,120,255} [state=44424230] SELECTED
sc=0x85b23d0 pst=:-moz-non-element<
          "a"
        >
      >
    >
  >
>

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This restores the old behaviour on MacOSX without regressing bug 207623.
I made the text control recognise this case and move the selection
range from the anonymous DIV to its child text node instead.
(tested on Camino/MacOSX, Firefox/WinXP, SeaMonkey/Linux)

Comment 15

[Uri Bernstein]

(In reply to comment #14)
Can we perhaps do this as a hidden preference (similar to
layout.word_select.eat_space_to_next_word), which has a different default value
based on the OS, instead of as an #ifdef?
This has two advantages:

1. Windows/Linux users which are more used to "selection-style" would be able to
trigger that behavior.

2. Developers working on related code would be able to test the effect of their
work on other platforms, without having to actually compile and test on
different platforms (this is really the main reason I'm suggesting this).

Comment 16

[Uri Bernstein]

(In reply to comment #11)
> The Windows XP platform behaviour seems to be caret-style.
> IE6 text form controls is an exception that uses selection-style.
> If you look at the rest of IE6 - url bar, Preferences, Find,
> Save/Open and other dialogs etc they all use caret-style.
> For now, I think we should leave as is (caret-style), even if that
> differs to IE6. I'd much rather be coherent with the desktop at large
> than a particular application. 

One thing I noticed on windows is that in places where caret-style is used, the
caret is actually visible and blinking when the selection is in effect. In all
cases where there is only a selection, and no blinking caret, Windows behaves
"selection-style". This actually makes sense (somewhat), because the behavior of
the arrow keys should rely only on what's currently displayed on the screen, and
not on memory of where the non-visible caret "really" is.

Since we never show the caret when there is an active selection (and assuming we
don't want to change that), I suggest that in order to be consistent with
Windows UI, we should use "selection style" on Windows.

Also, it should be noted that "selection style" (with no visible caret) is the
only behaviour of all Office applications (including Outlook). this is true both
in the main editing area, and in all dialogs and forms (as far as I can see).

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to get to this for a bit (at least a week, possibly a
lot longer).  I'm completely swamped at the moment... :(

Comment 18

[:Gavin Sharp [email: [e-mail]]]

sdfsdfsdfsdfsdfsd

Comment 19

[Asa Dotzler [:asa]]

This is gonna need to make b4 if it's gonna make 1.1 so the 1.1 nomination is
redundant.

Comment 20

[Ashley Bischoff (blog at handcoding.com)]

http://greasemonkey.mozdev.org/

Comment 21

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #20)
> http://greasemonkey.mozdev.org/

Sorry for the irrelevant text there -- that was a bad paste on my part.

Comment 22

[Simon Fraser [no longer active]]

Any chance of getting reviews soon?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Not from me.  I'm completely swamped with other work, as I said in comment 17,
and I don't understand this code very well, so it would take me at least 4-5
hours to do the review, which makes it that much harder to find the time for it.
 If there's anyone at all who knows this code, I would much appreciate if they
could review this....

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 188779 [details] [diff] [review]
Patch rev. 1

I can review this, but I think a better approach would be to fix
TruncateInsertionIfNeeded (or its callees) to handle the select-all case.

Comment 25

[Asa Dotzler [:asa]]

simon, roc, what's going to happen here. We're coming up fast on beta.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would prefer a solution that fixes TruncateInsertionIfNeeded, but I can review
this patch is Mats doesn't want to do that for some reason.

+            NS_ASSERTION(domText,
+              "The children of the anonymous DIV in a text control should be
#text nodes");

Can't an anonymous DIV contain <BR>s too?

Comment 27

[Mats Palmgren (inactive)]

(In reply to comment #26)
> I would prefer a solution that fixes TruncateInsertionIfNeeded,

I did consider that but chose not to for some reason I can't remember
right now. Let me have a look at it again...

> Can't an anonymous DIV contain <BR>s too?

In a single line control it's only present when the value is "" and the
assertion is in a "else" fork of "if (startOffset == 0)" which implies
we should not see the <br> here.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #27)
> (In reply to comment #26)
> > Can't an anonymous DIV contain <BR>s too?
> 
> In a single line control it's only present when the value is "" and the
> assertion is in a "else" fork of "if (startOffset == 0)" which implies
> we should not see the <br> here.

I hope you're really sure about that ... note that you can paste multiline text
into a single line text control.

Comment 29

[Uri Bernstein]

Please take into consideration my comment #15 (requesting for this to be a
hidden preference) and comment #16 (requesting for this to be fixed for Windows,
in addition to Mac) when considering this patch. I got no response to these
comments - I'm not sure if this means that people disagree, or that they have
just been overlooked.

Comment 30

[Mats Palmgren (inactive)]

(In reply to comment #29)
> Please take into consideration my comment #15 (requesting for this to be a
> hidden preference)

Sure, I don't mind a pref. for this

> and comment #16 (requesting for this to be fixed for Windows,

I disagree, IE6/Windows actually uses "caret-style" for most of it's UI -
url-bar, Preferences, Find, Save/Open and other dialogs etc they all use
caret-style as do the rest of Windows XP proper. For migrating IE-users, my
guess is that the url-bar is more signicative of what kind of caret style
they expect than a form control.

I agree that the caret should be blinking when "caret-style" is used.
I'll file a separate bug on that.

Comment 31

[Mats Palmgren (inactive)]

(In reply to comment #28)
> note that you can paste multiline text into a single line text control.

Yes, as far as I know they are converted into \n

Comment 32

[Uri Bernstein]

(In reply to comment #30)
> I agree that the caret should be blinking when "caret-style" is used.
> I'll file a separate bug on that.

It's not just that the caret is not blinking, it's not visible at all. My point
was that Windows is consistent in that when the caret is visible, caret-style is
used, and when the caret is not visible, selection-style is used. This makes sense.

If we are to leave the situation as it is on this bug, but won't get to fixing
the "display caret when selection is active" bug for Ff 1.5, we would be
breaking that consistency. So I suggest that these two changes (compared to Ff
1.0) should be either done together, or not done at all.

Sorry for repeating myself. I just wanted to make sure that my point came
through clearly. The decision is yours, of course.

Comment 33

[Mats Palmgren (inactive)]

TruncateInsertionIfNeeded calls GetTextSelectionOffsets to find out
if there is a selection and how long it is.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/text/nsTextEditRules.cpp&rev=1.195&root=/cvsroot&mark=1332,1339#1301
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/text/nsPlaintextEditor.cpp&rev=1.88&root=/cvsroot&mark=566-571,594,601,652-654#557

In GetTextSelectionOffsets we have these nodes and offsets, which appears
correct in some sense:
start(<div>0x8800938, 1) end(<div>0x8800938, 1) parent(<div>0x8800938)

Then we call GetAbsoluteOffsetsForPoints() which is where the problem starts
because it seems to have two "modes". If called with #text nodes it correctly
returns the text length between start/end nodes. If called with elements it
returns the total length of the text nodes in that element.
I tried to work around that so that
if (endNode==startNode && endOffset==startOffset)
then I returned the same start/end text offset but then assertions went off
so other code is definitely dependent on the current behaviour.

It seems to work if I do it one level up in GetTextSelectionOffsets() but
I've only done limited testing so far.

(I remember now that when I looked at nsPlaintextEditor.cpp the first time,
I was thinking we'd better not upset it by passing selections that isn't a
range of #text nodes.)

Comment 34

[Uri Bernstein]

Is there anything I can do to help get this fixed for 1.8?

Could we consider backing out bug 207623 on the 1.8 branch, then come up with a
solution everybody is happy with on the trunk?

Comment 35

[Mats Palmgren (inactive)]

Attachment: Patch B

This is the patch described in comment 33.
Uri, can you please test this on MacOSX?

Comment 36

[Uri Bernstein]

Thanks, Mats.
I tested the patch on OS X. With the setting at "0" or "2", it indeed restores
the previous behavior accurately, as far as I could tell. I also checked the
testcase  for 207623 and verified that it has not regressed.

A couple of notes:
1. I think it would be more elegant and consistent if the pref had only two
values, and the default would be set according to the platform in all.js, i.e.
it would be set to a value representing "caret style" in the main init block,
and overwritten with a value representing "selection style" inside the #ifdef
XP_MACOSX block (line 1312). This is the way it is done for e.g.
layout.word_select.eat_space_to_next_word (the override there is for Windows),
and it doesn't require ugly (IMO) #ifdefs in the code itself. Also I don't think
that "platform default" is unambiguous on Windows and Linux, which use both
styles in different places.

2. On Mac native apps, arrow-up and arrow-down actually move the caret a line up
or down, starting from the beginning of the selection (regardless of the
"direction" of the selection). Personally I'm OK with the behavior
(re)introduced by the patch, but while we're at it we might want to go for
better compatibility.

3. This does not work (i.e., works reversely) for RTL text. However, this also
happens in Safari and native OS X apps, so we might want to keep it for now in
order to be bugwards-compatible.

I'll give this an r+ if issue 1 is fixed, or if you have a good reason not to
fix it. 2 and 3 could be filed as separate bugs, unless you want to fix 2 now.

(BTW, what does "f,a" mean?)

Comment 37

[Mats Palmgren (inactive)]

Attachment: Patch B, rev. 2

Make UP/DOWN fall through so that it moves the caret normally...

Comment 38

[Mats Palmgren (inactive)]

(In reply to comment #36)
> 1. I think it would be more elegant and consistent if the pref had only two
> values, and the default would be set according to the platform in all.js, 

Does that really work if you share the same profile between platforms?
Let's say I ovveride it on Linux by setting it to 2, then start MacOSX
build using the same profile - it finds 2, which is the default on this
platform and hence it's removed from prefs.js, the next time I start on
Linux the override I did is gone.

> 2. On Mac native apps, arrow-up and arrow-down actually move the caret
> a line up or down, starting from the beginning of the selection
> (regardless of the "direction" of the selection).

Ok, I didn't know that. I think this can be easily fixed by just
removing DOM_VK_UP/DOWN from the "caretStyle == 2" block...
Please try the latest patch.

> 3. This does not work (i.e., works reversely) for RTL text.

What did not work exactly?

> (BTW, what does "f,a" mean?)

It's not my comment but I think it means "focus,anchor".

Comment 39

[Uri Bernstein]

(In reply to comment #38)
> (In reply to comment #36)
> > 1. I think it would be more elegant and consistent if the pref had only two
> > values, and the default would be set according to the platform in all.js, 
> 
> Does that really work if you share the same profile between platforms?
> Let's say I ovveride it on Linux by setting it to 2, then start MacOSX
> build using the same profile - it finds 2, which is the default on this
> platform and hence it's removed from prefs.js, the next time I start on
> Linux the override I did is gone.

Well, that seems to be the way things are done. There are quite a few setting in
platform-specific #ifdefs in all.js, and they probably all suffer from the
problem you're describing (which, I think, is somewhat far-fetched). I don't see
why this one should be different.

> 
> > 2. On Mac native apps, arrow-up and arrow-down actually move the caret
> > a line up or down, starting from the beginning of the selection
> > (regardless of the "direction" of the selection).
> 
> Ok, I didn't know that. I think this can be easily fixed by just
> removing DOM_VK_UP/DOWN from the "caretStyle == 2" block...
> Please try the latest patch.
> 

I didn't actually try, but it's pretty clear that with the new patch, the
up/down movement will be from where the (invisible) caret is, which is at the
*end* of the selection if the selection was done "forward" (the more common
case). Mac behavior is to move from the beginning of the selected text,
regardless of the direction in which the selection was made.
The whole thing is minor, IMO, but I think that if we don't fully imitate
standard Mac behavior (which shouldn't be hard to do), I prefer the behavior
from the original patch (because it doesn't depend on the direction of the
selection).  

> > 3. This does not work (i.e., works reversely) for RTL text.
> 
> What did not work exactly?
> 

Pressing left-arrow places the caret at the beginning (right) of the selection,
and vice-versa. As I said, some people might want to keep this for compatability
with (buggy) system behavior.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> which, I think, is somewhat far-fetched

Not at all far-fetched in any setup that involves home directories on a file
server (NFS, AFS, etc).  MIT runs that way, for example....

Comment 41

[Uri Bernstein]

> Not at all far-fetched in any setup that involves home directories on a file
> server (NFS, AFS, etc).  MIT runs that way, for example....

I'm not sure I understand what the location of the home directory has to do with
it. Mats' scenario requires that:
1. A user uses the same profile using clients running on two different platforms
(regardless of where that profile is actually stored).
2. The user manually changes a config entry which has a different default value
on each of these platforms.

#1 I'm sure happens, but is not common. #2 is also, I would guess, uncommon.
So the math I was doing was: uncommon^2=far-fetched.
 
It seems to me that if we're really worried about this case, there's a lot of
other stuff that has to be changed. The preference discussed here is no
different than the dozens of others set in platform-specific #ifdefs in all.js.

Comment 42

[Uri Bernstein]

Actually, I now remember that when I migrated my profile from Windows (98) to
Mac it didn't go smoothly: I had to delete the chrome directory and re-install
extensions.
Is sharing profiles between platforms officially supported? Does it actually work?

...

Googling a bit got me this:
http://www.lifehacker.com/software/firefox/cross-platform-firefox-hack-125129.php

So it seems like you cannot simply share a profile between Mac and Windows.
Assuming the same is true for Linux, this makes the argument pretty theoretical.
If/when we want to officially allow cross-platform profile sharing, this will be
just one the issues we'll have to deal with.

Comment 43

[Mats Palmgren (inactive)]

(In reply to comment #39)
> Well, that seems to be the way things are done.

IMO they should be fixed so we support the scenario a described.

> Mac behavior is to move from the beginning of the selected text,
> regardless of the direction in which the selection was made.

Interesting, is that also the behaviour when extending the selection?

I did a quick fix that seems to work, but it involves changes in nsCaret
too so let's fix this as a separate bug.

> [for RTL] Pressing left-arrow places the caret at the beginning (right)
> of the selection, and vice-versa.

Hmm, that's not what I'm seeing (or were you describing what you think is the
desired behaviour?), anyway let's deal with this separately too.

So, I think we should take Patch B rev. 1 for now.

Comment 44

[Uri Bernstein]

Comment on attachment 197219 [details] [diff] [review]
Patch B

OK. I'm sorry I couldn't convince you about the #ifdef thing, but it's more
important to me that this gets fixed.

As for bidi - what I described was what I'm actually seeing (with the patch).
You have to use actual RTL text (not just an RTL paragraph or textbox). E.g.,
try it in the top textarea in attachment 128682 [details].

Comment 45

[Mats Palmgren (inactive)]

(In reply to comment #44)
> As for bidi - what I described was what I'm actually seeing (with the patch).
> You have to use actual RTL text (not just an RTL paragraph or textbox).

Ok, I see what you mean, thanks.

Comment 46

[Mats Palmgren (inactive)]

Comment on attachment 197219 [details] [diff] [review]
Patch B

See comment 33 regarding the editor change.
There are still a couple of problems to get correct Mac behaviour,
but those most likely also occurred before bug 207623 and I'm
trying to keep the patch somewhat safe for branch inclusion...

Comment 47

[Boris Zbarsky [:bzbarsky]]

I won't be able to sr this in time for 1.8b5.  And roc's traveling...

David, do you think you would be able to look?

Comment 48

[Asa Dotzler [:asa]]

Comment on attachment 197219 [details] [diff] [review]
Patch B

please don't request approval until you've got all the necessary reviews.
thanks.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here's what GetAbsoluteOffsetsForPoints is doing:

aInCommonParentNode is a node that contains text node descendants. aInStartNode
and aInEndNode are descendants of aInCommonParentNode and aInStartOffset and
aInEndOffset are offsets within those nodes. GetAbsoluteOffsetsForPoints forms a
hypothetical string that is the concatenation of all the text descendants of
aInCommonParentNode. If aInStartNode is a text node then the output start offset
is offset of the start point in the hypothetical string, otherwise the output
start offset is 0, the beginning of the string. If aInEndNode is a text node
then the output end offset is the offset of the end point in the hypothetical
string, otherwise the output end offset is the length of the string (the end of
the string).

This isn't really good but I guess it's OK if it was documented. It would make
more sense if aInStartNode and aInEndNode were restricted to be text nodes or
NULL.  In particular if aInStartNode/aInEndNode is not a text node then the
corresponding offset is ignored, so it's strange that GetTextLength goes to some
trouble to compute an end offset which is always ignored!

So here's what I recommend here for the GetTextSelectionOffsets fix:
-- Add a comment to GetAbsoluteOffsetsForPoints similar to what I mentioned above.
-- In GetTextLength, pass nsnull and zero as the start and end nodes/offsets.
-- In GetTextSelectionOffsets, if startNode is not a text node, assert that it's
the root node and startOffset is zero, and set startNode to nsnull. If endNode
is not a text node, assert that it's the root node and endOffset is the root's
child-count, and set endNode to nsnull.

Sound reasonable?

Comment 50

[Mats Palmgren (inactive)]

Attachment: Patch 3

As described in last comment, with one minor exception -
when typing in an empty text control we get calls with "<div> 0 / <div> 0"
but the number of children is 1 in this case because of the anonymous <br>
so there is an exception in the ASSERTION for this case.

This patch only changes nsPlaintextEditor.cpp compared with last version
(which has r=uriber).

IMHO we should just remove GetAbsoluteOffsetsForPoints() at some point.
GetTextSelectionOffsets() and GetTextLength() are the only callers and
they want vastly different behaviour anyway.
I bet the resulting code would be less complex and even smaller if we
do that.

Comment 51

[Uri Bernstein]

Patch 3, although fixing this bug, regresses bug 207623 when the pref is set to
2 (or 0, on Mac).

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 198547 [details] [diff] [review]
Patch 3

minusing based on Uri's comment

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #50)
> IMHO we should just remove GetAbsoluteOffsetsForPoints() at some point.
> GetTextSelectionOffsets() and GetTextLength() are the only callers and
> they want vastly different behaviour anyway.
> I bet the resulting code would be less complex and even smaller if we
> do that.

Feel free to give it a try.

Comment 54

[Wevah (Nate Weaver)]

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

Comment 55

[Mats Palmgren (inactive)]

Attachment: Patch 4

I have tested this more thoroughly now and have found additional
problems. First there is a problem when we have the DIV as either
start or end node and #text as the other. The current code does not
distinguish between the two cases, DIV:0 and DIV:child_count,
either can occur as start or end. When we have DIV:0 we should
break the loop early and only count up to the #text-node.
(I believe this is bug 204506)

Secondly, using the common ancestor from the range is bogus
when there are two or more #text-nodes (which is quite common
actually). Consider for example (text is "12345"):
startNode #text(0x87d9478) startOffset=3
endNode #text(0x87d9478) endOffset=2
rootNode div(0x8576c10)  childCount=2
   #text(0x8863130)
   #text(0x87d9478)

The common ancestor from the range is #text(0x87d9478) and we
would get the result {aOutStartOffset=2 aOutEndOffset=3} when
the correct result is {aOutStartOffset=4 aOutEndOffset=5} since
there is a #text-node preceeding it which is not part of the selection.
This doesn't matter in most cases, for example when checking MAXLENGTH
since it only cares about aOutEndOffset-aOutStartOffset) but it appears
to be used elsewhere, when editing password text for example:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/text/nsTextEditRules.cpp&rev=1.198&root=/cvsroot&mark=864#860


I think we should always use the root node for the iterator.

====

In some cases we have an empty #text-node as start/end node -
they are not editable because the frame width is zero.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/base/nsEditor.cpp&rev=1.449&root=/cvsroot&mark=3927#3897

(steps to reproduce that situation:
  1. load data:text/html,<input maxlength=5 value=12345>
  2. TAB to the text control (all text is selected)
  3. CTRL+C
  4. SHIFT+LEFT ("1234" is selected)
  5. CTRL+V (caret is after "4")
  6. SHIFT+RIGHT ("5" is selected)
  7. CTRL+V
  8. RIGHT
      now we have an empty #text as selection, 
      endNode==startNode and both offsets are 0.
      this situation only occurs for caret_style=1)

(I wonder if that empty #text-node should have been created in the
first place...)

This patch handles non-editable start/end-nodes too.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we should rev nsIPlaintextEditor just to make textLength unsigned.
Other than that, it looks fine although maybe GetTextSelectionOffsets could be
simplified. E.g., couldn't you remove the if blocks
+  if (startNode && !::IsTextNode(startNode)) {
and
+  if (endNode && !::IsTextNode(endNode)) {
before the loop, and then replace
+    NS_ASSERTION(!endNode, "failed to find the end #text-node");
+    NS_ASSERTION(endNode || endNodeOffset == nodeCount-1 || endNodeOffset == 0,
+                 "invalid non-#text end node offset");
+    endOffset = totalLength;
with
+    NS_ASSERTION(endNode == rootNode, "failed to find the end #text-node");
+    NS_ASSERTION(endNodeOffset == nodeCount-1 || endNodeOffset == 0,
+                 "invalid non-#text end node offset");
+    endOffset = endNodeOffset == 0 ? 0 : totalLength;

and similarly for startOffset?

Comment 57

[Mats Palmgren (inactive)]

Attachment: Patch 5

(In reply to comment #56)
> I don't think we should rev nsIPlaintextEditor just to make textLength
> unsigned.

Ok, I'll file that separately so it can be incorporated at a more
suitable time. Added a XXX comment for now.

> ... maybe GetTextSelectionOffsets could be simplified.

fixed

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 199732 [details] [diff] [review]
Patch 5

lovely!

Comment 59

[Mats Palmgren (inactive)]

Checked in to trunk 2005-10-16 17:54 PDT.

Filed bug 312675, bug 312676 and bug 312678 for other problems mentioned in
comments.

->FIXED

Comment 60

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is there any intention to get this on the branch, or has the fix become too
great a set of changes for this late?  This bug was nominated for earlier
milestones, but I don't see any flags now.  It's a regression from Camino 0.8.4
and I assume from Fx 1.0.x, too, and see smfr's comment 8.

Comment 61

[Asa Dotzler [:asa]]

too late for changes of this scale.

Comment 62

[jeremy]

This problem does not appear to be fixed at all in Camino v 1.0b1 for Mac OS 10.4.2

Comment 63

[Uri Bernstein]

(In reply to comment #62)
> This problem does not appear to be fixed at all in Camino v 1.0b1 for Mac OS
> 10.4.2

This is because the patch was only landed on the trunk, and not on the Gecko 1.8 branch, from which Camino 1.0 (and Firefox 1.5) are built. See comment #60 and comment #61. 

Comment 64

[Marcus Gilroy-Ware]

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

Comment 65

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 66

[Adam Guthrie]

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

Comment 67

[jordan314]

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

Comment 68

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 69

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Yeah, clearly, this is very important for Mac users. I strongly recommend landing
on the 1.8.1 branch.

Comment 70

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 199732 [details] [diff] [review]
Patch 5

approval for 1.8.1 landing FOR THE EDITOR part of the patch. This fix is important for Mac usability. There are no intefaces changes, only two comments are changed inside an idl.

Comment 71

[Samuel Sidler (old account; do not CC)]

roc, can you approve the layout part of this patch for the 1.8.1 branch?

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 199732 [details] [diff] [review]
Patch 5

layout part approved for 1.8.1

Comment 73

[Samuel Sidler (old account; do not CC)]

Mats: Care to check this in on the 1.8 branch?

Comment 74

[Samuel Sidler (old account; do not CC)]

Comment on attachment 199732 [details] [diff] [review]
Patch 5

timeless checked this into the MOZILLA_1_8_BRANCH at 2006-02-28 23:06.

Comment 75

[Samuel Sidler (old account; do not CC)]

Clearing blocking request since this patch has been checked in.

Comment 76

[jordan314]

I just want to note that this is STILL A BUG in Bon Echo (2.0 Alpha)

Comment 77

[jordan314]

Sorry for the newbie thread but how do I install this patch? It looks like source code that I'll have to recompile. Does firefox have any automated way of installing patches?
Sorry to be a pain but if someone replies and answers my question I'll make a web tutorial for others like me. My email is jordan314 at yahoo dot com.
I have firefox 1.5 and 2.0 alpha around but I still keep 1.0 around only because this bug drives me crazy. Thanks.

Comment 78

[Uri Bernstein]

Yes, I can confirm that this is not fixed on the 1.8 barnch (and therefore not in Bon Echo Alpha 2).
Apparently the main change to nsSelection.cpp somehow got left out when the rest of this patch got checked in (perhaps as a result of a conflict?). This is what got checked in instead: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&root=/cvsroot&file=nsSelection.cpp&rev1=3.199.2.8&rev2=3.199.2.9)

Jordan, there is nothing you can do about this as a user. Hopefully this will be fixed correctly sometime before the Firefox 2.0 release.

Comment 79

[Uri Bernstein]

Checked in the missing hunk from nsSelection.cpp to the 1.8 branch:
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.199.2.10; previous revision: 3.199.2.9
done

Comment 80

[jordan314]

yes! yes!! yes!!! This bug has been fixed in Firefox 2.0 Beta!!!!!