392809 - text box editor regression with control+arrow keys

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

This problem is seen in SM 2.0a1pre 20070810 (a nightly trunk build).
It is NOT seen in the last of the SM 1.5a nightly trunk builds.

The behavior of the text editor, the one used to edit text in boxes like
the description box into which this text is being typed, has changed in
a way that is really irritating.  

The control+left arrow and contrl+right arrow key combinations now work 
differently than they have worked for the past ~10 years.  Prior to 
SM 2.0a1pre, they moved the cursor backward or forward to the beginning
of the previous or next word, and they did not treat punctuation 
characters differently from letters (with one exception noted below).  

That is, punctuation characters such as period and comma, when appended 
to a word, were treated just like any other letter in the word for purposes 
of control+arrow cursor movements.  (One exception: when an apostrophe is 
followed immediately by another letter, the letter following the apostrophe 
is treated as the first letter of a new word for control+arrow movements.)

Consider the line of text below.  With SM up through 1.5a, the control+
arrow combination would move the cursor to be positioned immediately 
to the left of each of the capital letters shown, and would not stop 
anywhere else.

>  For My Purposes, Yes It Would Be.  I Can'T Speak For Others.

But in SM 2.0a1pre, the punctuation characters now cause a new behavior
for control+arrow.  In addition to stopping at the beginning of words,
It now also stops just past any punctuation character (in the direction
it is moving).  So, in the sample line of text above, SM 2.0a1pre not
only stops just to the left of the capital letters, but also stops just
to the right or left of the periods and commas, depending on the direction
of the arrow.  When using control+right arrow, it stops to the right of 
each punctuation character.  When using control+left arrow, it stops to
the left of each punctuation character.  

Consequently, it stops TWICE between words that are separated by commas or
periods.  In the sample text above, when going from left to right, it stops
to the right of the comma, and then again to the left of the Y, and it 
stops to the right of the period and then again to the left of the I.
When going from right to left, it stops to the left of the I, then to 
the left of the period then to the left of the B.  It also stops to the 
left of the Y, the left of the comma, and the left of the P.  

Once consequence of this change is that it takes many more key strokes
to move the cursor across a typical line of text.  To a touch typist 
who has spent ~10 years using the editor as it was previously, this 
has instroduced MANY typing mistakes.  This regression needs to be fixed.

Perhaps this should be a "core" bug, but I don't know what component.
Perhaps "Editor" ?

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The dots below show that the cursor stops to the LEFT of the corresponding 
character for the given operation in the given version of SM

>  For My Purposes, Yes It Would Be.  I Can'T Speak For Others.
   .   .  .         .   .  .     .    . .   . .     .   .       SM1.5a ->
   .   .  .         .   .  .     .    . .   . .     .   .       SM1.5a <-
   .   .  .        ..   .  .     .  . . .   . .     .   .       SM2.0a1pre ->
   .   .  .       . .   .  .     . .  . .   . .     .   .       SM2.0a1pre <-

Comment 3

[Jesse Ruderman]

Reopening per bug 385565 comment 9.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Below is a snippet of text with which I have the problem.  Copy and paste
it into the bugzilla comment editor box, and try it for yourself.

The problem is seen with any word that is preceeded by a punctuation 
character, such as a period or an open (left) parenthesis.  
(Note: the text in the snippet below is not factually accurate. :)

- ------------------ begin snippet ----------------
Mailnews silently ignores the failure, but does go on to update the .msf 
file (or perhaps the .msf file was updated first, before the failure to 
open the .rc file).  The .msf file contains a (normally redundant) copy
of the information found in the .rc file.  When mailnews restarts, it reads
- ------------------ end snippet ----------------

ctrl-left arrow should stop at the beginning of the current word or the
preceeding word.
ctrl-right arrow should stop at the end of the current word, or the next one.

But when a word is preceeded by a punctuation character (such as in 
"(normally" or ".msf" or ".rc"), ctrl-right arrow stops after the character, 
not before it, and ctrl-left arrow skips over the word, over the punctuation
and over the space that preceeds the word, and stops at the tail end (right
side) of the preceeding word.  

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

component: Selection? 
Moving the cursor right and left a word at a time is selection?

Comment 6

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

I assume you have layout.word_select.eat_space_to_next_word set to 'true' and layout.word_select.stop_at_punctuation set to 'true', as would be the default on Windows.

With those settings, I get this:
   .   .  .         .   .  .     .    . .   . .     .   .       SM1.5a ->
which I guess is OK with you?

With this line:
> open the .rc file).  The .msf file contains a (normally redundant) copy
I get
  .    .    .  .       .    .   .    .        .  .        .          .
So obviously we should be stopping before the leading '.' and not after it, and I'll fix that (and similar for the reverse direction).

As far as I can tell bug 385565 did fix this bug as filed, and you have morphed this into another bug which is not fixed yet. That's OK.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The two layout.word_select prefs are set to the default of true, as you 
surmised.  

With this line, I expect the cursor to stop to the left of the dots shown below:
> open the .rc file).  The .msf file contains a (normally redundant) copy
  .    .   .   .       .   .    .    .        . .         .          .    
(same in both directions)

Comment 8

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

I'd like to land the word-selection patch in bug 391584 before working on this.

Comment 9

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

I guess http://lxr.mozilla.org/mozilla1.8/source/layout/generic/nsTextTransformer.cpp#456 is what makes this work in Firefox2. Need to figure out how best to replicate that in the new world.

Comment 10

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

Attachment: fix

Two new behaviours are required:
-- Always permit stopping between whitespace and following punctuation
-- Don't stop between punctuation and following non-punctuation if the preceding punctuation run follows white-space.

Comment 11

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

checked in.

Comment 12

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

There were tests in the patch.

Comment 13

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

This caused regressions, see bugs 385565 and 407155. I backed this out.

Comment 14

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

Attachment: fix v2

The regressions were because the previous version of this patch was confused about whether the aAfterPunct parameter to BreakWordBetweenPunctuation was taking aForward into account in the definition of "after". This version of the patch should fix that. I've also added tests for bug 385565/407155.

Comment 15

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

Maybe in the very long term the right way to do word breaking is actually to build one big string and then call native platform APIs...

Comment 16

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

We wouldn't block on this although I still want to take the patch for 1.9.

Comment 17

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

Comment on attachment 292345 [details] [diff] [review]
fix v2

I would like to get beta exposure for this change to word-caret-movement. This code is reasonably well regression-tested (and getting better tested with the tests in this patch).

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 292345 [details] [diff] [review]
fix v2

a1.9b4=beltzner

Comment 19

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

checked in