232406 - A caret does not move in a Japanese input.

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Hiro]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.7a) Gecko/20040127 Firebird/0.8.0+

When a left/right cursor key is pushed in the Japanese undecided state, a caret
does not move according to it.
But internally, the caret is moving.

When correcting a Japanese character (a Hiragana, state which has not been
changed into a Kanji yet), it is accompanied very much by difficulty.
It is like editing a document using the word processor with which a caret is not
displayed.


Mac OSX 10.3.2
2004-01-27 build
(This bug is checked from the time of Mac OSX 10.1.x & Chimera.)


Reproducible: Always
Steps to Reproduce:
1.Turn on input-method.
2.Input temporary character.(eg. あいうえお)
3.The left / right cursor key is pushed.

Actual Results:  
A caret does not move.

Expected Results:  
A caret moves to the left/right.

Comment 1

[Greg K.]

[e-mail], what Camino build are you reporting this problem against?
(Please be sure you're testing using a current nightly build, and certainly
something newer than an old Chimera build.)

Comment 2

[Mike Pinkerton (not reading bugmail)]

this is still an issue in current nightlies.

Comment 3

[waverider]

Attachment: patch for caret and a little improvement of look and feel.

Comment 4

[Hiro]

(In reply to comment #3)
> Created an attachment (id=163309)
> patch for caret and a little improvement of look and feel.

With this patch, it has checked that the problem of bug236996(and bug264515?)
was also Fixed.

Comment 5

[Kathleen :Brade]

Comment on attachment 163309 [details] [diff] [review]
patch for caret and a little improvement of look and feel.

Could a new patch with more context be added to this bug?

I'd like to see some blank lines before new blocks of code.

I'd like to see the DEBUG_IME removed altogether or #ifdef'd (rather than
commented out).

Comment 6

[waverider]

Attachment: patch for caret

I removed context not related this bug and corrected typo.
Please check again.

Comment 7

[Kathleen :Brade]

Comment on attachment 163967 [details] [diff] [review]
patch for caret

I really need more context to easily review this diff.	Try:  cvs diff -u9
nsChildView.mm

I don't understand the "count++" line; I think the comment should be larger and
I think it should be part of the declaration line:
  PRUint32 count = countRanges(aString) + 1;

Comment 8

[waverider]

Attachment: patch for caret ver.3

I correct my mistakes and add comment.

When we preinput with IME, we can move caret (insertion point) freely in range
of markedText.
At present, "convertAttributeToGeckoRange" return IME textrange attributes,
but caret position is not returned.
I add one more aRange[i] and put NS_TEXTRANGE_CARETPOSITION with caret positon
into aRange[i].

Comment 9

[Kathleen :Brade]

Comment on attachment 164068 [details] [diff] [review]
patch for caret ver.3

This patch looks a bit better to me (though I'd still like some blank lines
added before the multi-line comments).	I'm sorry I can't do any testing.

Comment 10

[Josh Aas]

waverider - could you please give an short explanation of your patch in general?
Getting an idea of what you are trying to do will help me review it faster.

Comment 11

[waverider]

Explanation:

"convertAttributeToGeckoRange" should syncronize state of Gecko preinputting and
IME text preinputting.
At present, this function return state of preinput text, but don't retrun caret
position.
That is cause of this bug.

"convertAttributeToGeckoRange" put Gecko's IME Constants (defined in
mozilla/widget/public/nsGUIEvent.h) into each aRange[i] and send them to Gecko.
I add one more aRange[i] and put NS_TEXTRANGE_CARETPOSITION (=Gecko's IME
constants about caret position) with caret positon into aRange[i].

PS:
Bugzilla jp : http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=3187
in this topic, Joker reported that my patch works fine.

Please review my patch.

Comment 12

[Josh Aas]

If I understand the bug description correctly, this bug has existed since Mac OS
X 10.1.x and Chimera?

Comment 13

[waverider]

re:comment#12
Exactly.

Comment 14

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 164068 [details] [diff] [review]
patch for caret ver.3

This patch looks OK and does not appear to have any adverse impact on
non-japanese pages. I do not have the ability to test with Japanese pages,
however, so I cannot verify that this fixes the problem there. Assuming it
does, r=[e-mail]

Comment 15

[Josh Aas]

Comment on attachment 164068 [details] [diff] [review]
patch for caret ver.3

could use a review from someone with a little more expertise in this area

Comment 16

[Josh Aas]

Comment on attachment 164068 [details] [diff] [review]
patch for caret ver.3

Looks good to me. I am pretty sure I understand what is going on now.

waverider - thanks for doing this. Sorry the review process took so long - you
know how it goes...

Comment 17

[Mike Pinkerton (not reading bugmail)]

+  // Caret have not "range", So mStartOffset and mEndOffset are same value.

this needs to be rewritten in better english. It's also not clear if the current
caret can be a selection or if it's always an insertion point.

Comment 18

[waverider]

Attachment: patch ver.4 I correct comment for caret.

I am sorry about my poor English.
Please check again.

Comment 19

[Mike Pinkerton (not reading bugmail)]

+  // Get current caret position.
+  // Caret is indicator of insertion point, so mStartOffset and mEndOffset are
same value.
+  aRanges[i].mStartOffset = selRange.location + selRange.length;              
          
+  aRanges[i].mEndOffset = selRange.location + selRange.length;                
        

you didn't answer my question. is this code only for the case of an insertion
point (start == end) or for the current selection? If it's only for an insertion
point, why go through all the math and just set |mEndOffset = mStartOffset|? If
it's for a general selection, why the does the comment only mention an insertion
point?

Comment 20

[waverider]

>is this code only for the case of an insertion
>point (start == end) or for the current selection?
this code is only for insertion point.
# NS_TEXTRANGE_CARETPOSITION is prepared only for getting insertion point when
we preinput with IME.


 >If it's only for an insertion point, 
 >why go through all the math and just set |mEndOffset = mStartOffset|?
when we preinput with IME, we can move insertion point only range of preinputted
text.
for example, when we preinput "ilovecamino", we can move insertion point only
the inside of "ilovecamino".
If we move insertion point to between "e" and "c", we should make aRanges for
insertion point and send it to Gecko:
aRanges.mStartOffset = 5;
aRanges.mEndOffset = 5;
aRanges.mRangeType = NS_TEXTRANGE_CARETPOSITION;
insertion point never contain characters and mStartOffset == mEndOffset.

about NSInputManager, I could'nt find the document about insertion point when we
preinput with IME. 
I tested  and  discoverd that insertion point  is "selRange.location +
selRange.length".

Comment 21

[Josh Aas]

you didn't answer my question. is this code only for the case of an insertion
point (start == end) or for the current selection? If it's only for an insertion
point, why go through all the math and just set |mEndOffset = mStartOffset|? If
it's for a general selection, why the does the comment only mention an insertion
point?

^ please more directly respond to the question... I don't think pinkerton
understands yet. What exactly is the point of the math you are doing? Is there
ever a sitionat in which mEndOffset should not be equal to mStartOffset?

Comment 22

[waverider]

Attachment: correct my mistake.

Sorry... I misunderstand about mEndOffset.
about caret position, mEndOffset = 0.

Comment 23

[waverider]

Attachment: description about math of caret position.

This is simple description about math of caret position.

Comment 24

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 169096 [details] [diff] [review]
correct my mistake.

sr=pink. thanks for the better explanation.

josh, can you land it?

Comment 25

[Josh Aas]

landed on trunk