14588 - Ctrl+right arrow skips over nbsp (nbsp should be word break)

Status: VERIFIED FIXED

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

Description

[sujay]

using 9/22 build of apprunner

1) launch apprunner
2) launch editor
3) place cursor at beginning of 2nd line("Here is an acute entity..")
4) start doing Ctrl+right arrow key to move cursor one word at a time

it does seem to work in the beginning, but once you get to this
word: "This sentence has two &nbsp" you'll notice the cursor
jumps to the end of that line, instead of going to each word
in that sentence..

all platforms.

Comment 1

[Kathleen :Brade]

this is expected behavior since there aren't any spaces (only nbsp's and
characters) in that sentence.  I tried to test 4.x but it eats/mangles the nbsp's
so it can't be used for comparison.

marking this invalid if/until we decide to change the behavior.
Simon? Charley?

Comment 2

[Simon Fraser [no longer active]]

The key and double-click handling should treat nbsps as white space. So sujay was
correct to file this bug.

Comment 3

[Kathleen :Brade]

ok, I agree.  We should start to think about whether we also want a hyphen to be
a breaking character or not.  (I wonder if there are any i18n issues here? ;-) )

note: this bug isn't just for right arrow but also when using left arrow.

Comment 4

[Charles Manske]

I remember hearing that I18N folks were responsible for deciding what is a
"whitespace" character, and thus the word separator for caret navigation,
and they want nbsp to NOT be whitespace. IMHO, this is wrong.

Comment 5

[Frank Tang]

1. It is true that i18n should own the line / word break and we have an api for
deciding line/word break.
2. It is not ture that we want to decie what is a whitespace.

I will just reassign this bug to myself and fix it right away. Let me make one
thing clear, you want to treat nbsp as word seperating boundary, right ?

Comment 6

[Frank Tang]

give me couple minutes, and I should fix it.

Comment 7

[Kathleen :Brade]

Frank, yes I think nbps's should be breaking points.  What about dashes?

Frank--when you finish you part of the bug, please reassign to mjudge for him to
fix the editor hooks to call your api.

Comment 8

[Frank Tang]

We already hookup the line/word break class for a long long long time. The
problme actually is not inside my word breaking class, but nsTextTransformer.cpp
itself. Why don't I just fix it for you. With mjudge's code review, of course.

Comment 9

[Kathleen :Brade]

adding Mike to cc list.  Mike, are you ok with Frank's plan?

Comment 10

[Frank Tang]

hum... thing are not that easy. What should we do with two nbsp together there?
How important is this bug ? It looks like it is not easy as I expected. The
white space compression code and the mixed with linebreaking code in
nsTextTransformer + the way nsTextTransformer implement (by using both 1byte
buffer and 2 byte buffer) make this difficult than I thought. Not a one liner
for me....

Comment 11

[Frank Tang]

Mark it M14

Comment 12

[Sean Richardson]

Not that anyone likely wants this bug complicated by any more issues,
but now is probably a better time than after a fix is made:

There are several characters in the "Special Characters" section of the
"Character Entities References" chapter of the HTML 4.0 spec
<URL:http://www.w3.org/TR/REC-html40/sgml/entities.html#h-24.4.1>
that almost certainly should be treated as whitespace for the purposes of
line-breaking, and there may be some others.

Prime candidates:
<!ENTITY ensp    CDATA "&#8194;" -- en space, U+2002 ISOpub -->
<!ENTITY emsp    CDATA "&#8195;" -- em space, U+2003 ISOpub -->
<!ENTITY thinsp  CDATA "&#8201;" -- thin space, U+2009 ISOpub -->
<!ENTITY zwnj    CDATA "&#8204;" -- zero width non-joiner, U+200C NEW RFC 2070
-->

Possibles, if hyphen is to be a breaking char.
<!ENTITY ndash   CDATA "&#8211;" -- en dash, U+2013 ISOpub -->
<!ENTITY mdash   CDATA "&#8212;" -- em dash, U+2014 ISOpub -->

Comment 13

[Frank Tang]

I don't think this should be beta. Move it to M17

Comment 14

[Kathleen :Brade]

updating summary to be more accurate; cc akkana since some of her work will rely 
on this functionality

Comment 15

[Frank Tang]

move to M18

Comment 16

[Frank Tang]

reassign to shanjian

Comment 17

[Shanjian Li]

Attachment: proposed fix part 1

Comment 18

[Shanjian Li]

Attachment: Proposed fix part2

Comment 19

[Shanjian Li]

Added ftang to cC list.

Frank, could you review the fix in your convenience?

Comment 20

[Frank Tang]

nsbeta3+ per i18n bug review meeting.
sorry, I have not review the code yet.

Comment 21

[Shanjian Li]

fix has been checked in.

Comment 22

[Marc Attinasi]

Reopened: we had to back out the changes to nsTextTransformer.cpp due to 
multiple regressions. I'm assuming that this bug will have to be re-addressed 
now, so reopening.

Comment 23

[Shanjian Li]

Attachment: a new fix which will not cause regression

Comment 24

[Shanjian Li]

the new fix has 2 improvement over previous one, which will not 
cause regressions found in previous fix.
1, nbsp is only treated as space character (isWhitespace = PR_TRUE;)
   when it is not for line break. 
2, If nbsp is the fist character, it will not be broken immediately,
   this make sure we advance at least one word ahead, thus fixed the 
   possible hanging.

Comment 25

[Frank Tang]

I think your latest fix is still too risky. Is that possible you can have two
version of
 nsTextTransformer::ScanNormalAsciiText_F(PRInt32* aWordLen,PRBool*
aWasTransformed)

Say ScanNormalAsciiText_F and ScanNormalAsciiText_F_ForWordBreak and have a if
statement in the
GetNextWord to switch between them. Keep the ScanNormalAsciiText_F the same as
today and put your fix into the new ScanNormalAsciiText_F_ForWordBreak routine?
In this way, we can 100% sure your change wont't change line break in layout.

Comment 26

[Shanjian Li]

the fix was modified according to ftang's suggestion. New fix only apply 
to wordjump.

fix checked in.

Comment 27

[sujay]

verified in 8/30 build.