159869 - Cannot input Japanese (2 byte) character in the browser.

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Mao Sasaki]

Cannot input Japanese (2 byte) character in the browser input field..
It is possible to input Japanese (2 byte) character in URL field, though.

Problem not occurs with OmniWeb and IE for OS X.

Comment 1

[Kyle Yuan]

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

Comment 2

[Winnie Lam]

Mao, do you mean text input fields like TEXTAREA?

Comment 3

[Mao Sasaki]

Yes. Every TEXTAREA such as Google's search text area, BBS's text area, etc. Problem not occurs with CFM Mozilla and CFM Netscape as well.

Comment 4

[Winnie Lam]

adding sfraser and ftang to cc: list

Comment 5

[Greg K.]

Duplicate of bug 151110?

Comment 6

[Mao Sasaki]

Problem not occurs with Mozilla　Mach-o, either.

Comment 7

[Kathleen :Brade]

-->brade
I am planning to work on this when my patch to bug 151110 is accepted

Comment 8

[Kathleen :Brade]

Attachment: start of IME work

Frank--please look this over and let me know where I need to clean up gecko IME
events (nsTextEvent, nsReconversionEvent, etc.)

Simon--please look this over and let me know what you think about overall
approach, etc.

Current problems: space doesn't scroll, probably lots of subtle ime things

Things that do seem to work: typing (including arrowing and deletion), typing
special characters like é, typing in japanese (とよた),
tabbing between form elements

Comment 9

[Frank Tang]

found some info which is missing from the man page
http://www.geminisolutions.com/WebObjects_4.5/Documentation/Developer/YellowBox/ReleaseNotes/AppKit.html

\Input Manager

This release introduces some additional input method API. At the core of
interaction, insertText: and setMarkedText:selectedRange: can receive an
instance of NSAttributedString as their arguments where they were restricted to
NSString in the previous releases. Input servers are expected to query valid
attributes with the validAttributesForMarkedText method. Currently the following
attributes are supported by NSTextView for marked text: Foreground color
(NSForegroundColorAttributeName), background color
(NSBackgroundColorAttributeName), and underline (NSUnderlineAttributeName).

This release provides more sophisticated programmatic interaction between the
user and the input methods, including allowing an input method to track mouse
events on text.

When Developer Release 2 ships, Apple's Developer Support web site will make
available source code for a sample input method that uses the new APIs to
implement a hex input method (allowing you to type a hex number to specify any
Unicode character).

 
-end of copy and paste=

the man page does not talk about 
Foreground color (NSForegroundColorAttributeName), 
background color (NSBackgroundColorAttributeName), 
and underline (NSUnderlineAttributeName).

Comment 10

[Frank Tang]

hum, there are no NSUnderlineAttributeName

I think it is NSUnderlineStyleAttributeName instead

Comment 11

[Frank Tang]

see
http://developer.apple.com/techpubs/macosx/Cocoa/TasksAndConcepts/ProgrammingTopics/AttributedStrings/Tasks/AccessingAttrs.html
about how to look into the NSAttributedString that inserText and setMarktedText
pass in

Comment 12

[Frank Tang]

Attachment: ftang's patch

not much. add some NSLog
return some meaningful array
add mInCompositoin ot keep state and mSelectedRang

Comment 13

[Kathleen :Brade]

Attachment: combined patches of above plus dispatching of keypress event

I would like to check in this patch before finishing IME

I believe that this patch does not regress any current behavior (pageup/down,
arrow scrolling, space to page down, etc.).  IME behavior is ok but not
complete.

reviews?

Comment 14

[Frank Tang]

Attachment: patch need to be merged with last patch. just archive here

basic ime is working in this patch but the following is not there yet
1. candidate window show in the wrong place we currently ask it to show in 0,0
2. mouse hit in other place does not reset input state when we aer in ime mode.
We need to call makredTextAbandoned to current input manager and send
composition end event to layout in ResetInputState and mouese down.
3. reconversion don't work as expect
don't take this patch, wait till I merge with the last patch brade put down

Comment 15

[Greg K.]

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

Comment 16

[Frank Tang]

Attachment: merged above two patch

this patch obsolete two patches above.

Comment 17

[Frank Tang]

what is working ?
1. different keyboard mapping (say russian keyboard)
2. dead key (option-e and e produce é)
3. basica japanese / chinese ime and koeran ime under "sentance" and "word" mode
known issue with last patch:
1. performance (lot of NSLog, turn DEBUG_IME off may solve it)
2. candidate window show in left top  corner of screen
3. korean ime does not work correctly in "character" mode
4. mouse click other place before the ime text may cause problem 
5. mouse operation within IME does not work (neither carbone or window )
6. ime reconvserion (japanese ime control+1) are not implemented yet.

Comment 18

[Simon Fraser [no longer active]]

I like the ALWAYS_SEND_THROUGH_INSERTTEXT code path in the patch, but the patch
needs cleaning up, and we need to ensure that we're feeding gecko all the key
events it wants in the correct sequence here.

Comment 19

[Mike Pinkerton (not reading bugmail)]

+  NSLog(@"in underlineAttributeToTextRangeType = %d", aUnderlineStyle);

make sure to remove the log before landing.

+static PRUint32 underlineAttributeToTextRangeType(PRUint32 aUnderlineStyle)
+static void fill_textrange_in_text_event(nsTextEvent *aTextEvent,
NSAttributedString* aString, NSRange selRange)

can we keep the naming scheme consistent?

+  NSRange limitRange;
...
+  limitRange = NSMakeRange(0, [aString length]);

combine these lines. this is c++. done in several places.

+  id attributeValue;
...
+    attributeValue = [aString attribute:NSUnderlineStyleAttributeName ...

same thing here. keep variables scoped appropriately if they're not used outsdie
of the scope. in fact |attributeValue| isn't used at all. Why bother having it?

in countRanges()...
+  PRUint32 i = 0;

rename this variable |count| or something descriptive. it's more than just the
induction variable of a loop. It's the return value. It has meaning.

+  unsigned int len = [tmpStr length];
+  PRUnichar* buffer = new PRUnichar[len + 1];
+  if (buffer)
+  {
+    [tmpStr getCharacters: buffer];
+    buffer[len] = (PRUnichar)'\0';
+  }
+

don't we have a routine that does this already? I thought we did. If not, maybe
we should.

+- (void) doCommandBySelector:(SEL)aSelector;
+{
+  [super doCommandBySelector:aSelector];
+}

i think you can remove this

+  if ( [aString isKindOfClass:[NSAttributedString class]])
+    [aString retain];
+  else
+    aString = [[NSAttributedString alloc] initWithString:aString];

instead of this pattern, can't you just do:

if ( ![aString isKindOfClass:[NSAttributedString class] )
  aString = [[[NSAttributedString alloc] initWithString:aString] autorelease];

then you don't have to worry about releasing it yourself and you don't have to
retain the one you don't create.

+- (NSAttributedString *) attributedSubstringFromRange:(NSRange)theRange;

you should probably stick to the cocoa naming scheme here, and call it like
"createAttributedSubstringFromRange" so that people know they own it. Either
that, or autorelease it. I'd tend towards autoreleasing it, myself.

+    [self convert: theEvent message: NS_KEY_PRESS
+            isChar: &isChar
+            toGeckoEvent: &geckoEvent];
+    geckoEvent.isChar = isChar;

why not pass &geckoEvent.isChar to the |isChar| param?

+        for (PRUint32 i = 0; i < length; i++) {
+        unichar c = [text characterAtIndex: i];
+        geckoEvent.charCode = c;
+        isKeyEventHandled = mGeckoChild->DispatchWindowEvent(geckoEvent);
+        }

better indenting here and combine the first 2 lines into 1
  geckoEvent.charCode = [text characterAtIndex: i];

Comment 20

[Simon Fraser [no longer active]]

> +    attributeValue = [aString attribute:NSUnderlineStyleAttributeName ...

> same thing here. keep variables scoped appropriately if they're not used outsdie
> of the scope. in fact |attributeValue| isn't used at all. Why bother having it?

that confused me too. But the call to [aString attribute:...] change the range.
It seems an odd way to count ranges, tho.

Comment 21

[Kathleen :Brade]

Attachment: cleaned up patch

This patch is now cleaned up as listed below:
 * All NSLog calls should be in #ifdefs (which will be removed when we're all
happy with ime functionality).
 * ALWAYS_SEND_THROUGH_INSERTTEXT #ifdef is gone
 * static functions named consistently
 * some commented out code is now #ifdef'd so it can be more easily
enabled/disabled
 * some variable cleanup
 * doCommandBySelector is left in but with a comment; I see two warnings if
this is not present
 * strings are autorelease'd
 * I did NOT cleanup the calls to [self convert: theEvent ...] since that is an
existing api; I'll clean it up on another pass since I'm not fond of it either.


These things are planned for subsequent patches:
 * make é entry more closely match Cocoa
 * make IME more closely match Cocoa behaviors
 * remove the #ifdefs
 * the things that ftang mentions in comment 17
 * overall cleanup and simplification of keyDown

Comment 22

[Simon Fraser [no longer active]]

+// sends gecko an ime composition event
+- (nsRect) sendCompositionEvent:(PRInt32)aEventType;
+
+// sends gecko an ime text event
+- (void) sendTextEvent:(PRUnichar*) aBuffer
attributedString:(NSAttributedString*) aString  selectedRange:(NSRange)selRange;

If these are just internal, move the declarations to the .mm file in a

@interface ChildView(Private)

protocol declaration.

+    case 0: return NS_TEXTRANGE_RAWINPUT;
+    case 1: return NS_TEXTRANGE_CONVERTEDTEXT;
+    case 2: return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT;

do we have an enum that can be used?


+  while (limitRange.length > 0) {
+    [aString attribute:NSUnderlineStyleAttributeName 
+             atIndex:limitRange.location 
+             longestEffectiveRange:&effectiveRange
+             inRange:limitRange];

I'd like to see a comment that says what this call does, since it's non-obvious.

+    // XXX ftang: hack to work around a problem

What problem?


+  PRUnichar* buffer = new PRUnichar[len + 1];
+  if (buffer)
+  {
+    [tmpStr getCharacters: buffer];
+    buffer[len] = (PRUnichar)'\0';
+  }

Be nice to avoid the allocation here. Maybe use a stack buffer if possible.

+  // note: having this here prevents a couple warnings

That's odd. What warnings? (This european would prefer "a copule of warnings").

+  PRUnichar* buffer = new PRUnichar[len + 1];
+  if (buffer)

Another allocation to avoid.

Comment 23

[Mike Pinkerton (not reading bugmail)]

> Be nice to avoid the allocation here. Maybe use a stack buffer if possible.

given that the size of the string is unknown, how can you use a stack buffer?

Comment 24

[Kathleen :Brade]

Attachment: better patch with places where ftang needs to comment/explain

Comment 25

[Simon Fraser [no longer active]]

pink: the ususal

PRUnichar  buffer[MAX_BUFFER_SIZE];
PRUnichar* bufPtr = buffer;

if (len > MAX_BUFFER_SIZE)
 bufPtr = new PRUnichar[len];

...
if (bufPtr != buffer)
  delete [] bufPtr;

I wrote a C++ class that does this somewhere (lxr for 'spillablestackbuffer'). I
with it was in XPCOM.

Comment 26

[Frank Tang]

+static PRUint32 underlineAttributeToTextRangeType(PRUint32 aUnderlineStyle)
+{
+#ifdef DEBUG_IME
+  NSLog(@"in underlineAttributeToTextRangeType = %d", aUnderlineStyle);
+#endif
+
+  // XXX ftang: please comment here if you have any idea what constant
+  // could be used for case 2; I couldn't find anything other than masks
+  switch (aUnderlineStyle)
+  {
+    case NSNoUnderlineStyle:     return NS_TEXTRANGE_RAWINPUT;
+    case NSSingleUnderlineStyle: return NS_TEXTRANGE_CONVERTEDTEXT;
+    case 2:                      return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT;
+    default:
+      NS_ASSERTION(0, "cannot convert");
+  }
+  return NS_TEXTRANGE_SELECTEDRAWTEXT;
+}

// please see http://developer.apple.com/techpubs/macosx/Cocoa/
// TasksAndConcepts/ProgrammingTopics/AttributedStrings/Tasks/AccessingAttrs.html
// about underline attribute. We are not clear where does the value 2 define right
// now. We saw this will be feed in if we return [NSArray array]; 
// in  validAttributesForMarkedText
// in japanese ime, type 'aaaaaaaaa' and hit space will make the ime send you
// some part of text in 1 (NSSingleUnderlineStyle)  and some part in 2. 
// ftang will ask apple about more detail


+static PRUint32 countRanges(NSAttributedString *aString)
+{
+  // XXX ftang: please comment here
+  PRUint32 count = 0;
+  NSRange effectiveRange;
+  NSRange limitRange = NSMakeRange(0, [aString length]);
+  while (limitRange.length > 0) {
+    [aString attribute:NSUnderlineStyleAttributeName 
+             atIndex:limitRange.location 
+             longestEffectiveRange:&effectiveRange
+             inRange:limitRange];
+    limitRange = NSMakeRange(NSMaxRange(effectiveRange), 
+                             NSMaxRange(limitRange) - NSMaxRange(effectiveRange));
+    count++;
+  }
+  return count;
+}
// iterate thorugh all the NSUnderlineStyleAttributeName and count how many 
// different segment is here. 
// it is copy from sample code in http://developer.apple.com/techpubs/macosx/Cocoa
// /TasksAndConcepts/ProgrammingTopics/AttributedStrings/Tasks/AccessingAttrs.html


+static void convertAttributeToGeckoRange(NSAttributedString *aString, PRUint32
inCount, nsTextRange* aRanges)
+{
+  // XXX ftang: please expand/clarify
+  // get the maximum effective range for underline style for the given string
+  // limitRange is the resulting maximum effective range based on all ranges

// convert the cocoa range into the nsTextRange array used in Gecko 
// iterate thorugh the attributed string and map the underline attribute to 
// gecko IME TEXTRANGE attributes
// We may need to change the code here if we change the implementation of 
// validAttributesForMarkedText

+static void fillTextRangeInTextEvent(nsTextEvent *aTextEvent,
NSAttributedString* aString, NSRange selRange)
+{ 
+  // XXX ftang: please comment here

// count the number of segment in the attribute string, 
// allocate the right size of nsTextRange
// convert the attribute string into array of nsTextRagne by calling functions
// above


+    convertAttributeToGeckoRange(aString,  aTextEvent->rangeCount, 
aTextEvent->rangeArray);
+    // XXX ftang: hack to work around a problem
+    if ( (aTextEvent->rangeCount == 1) && (NS_TEXTRANGE_RAWINPUT ==
aTextEvent->rangeArray[0].mRangeType) )
+      aTextEvent->rangeCount = 0;

gee.. I wish I remember

Comment 27

[Kathleen :Brade]

Attachment: patch with cleaned up comments from ftang

reviews/feedback?

Comment 28

[Simon Fraser [no longer active]]

+#define MAX_BUFFER_SIZE 32
+- (void)insertText:(id)insertString;
+{
+  unsigned int len = [tmpStr length];
+  PRUnichar buffer[MAX_BUFFER_SIZE];
+  PRUnichar *bufPtr = (len >= MAX_BUFFER_SIZE) ? buffer : new PRUnichar[len + 1];

Conrad is checking in an nsAutoBuffer implementation that we should use here.
I'm not sure when it's going in.

+// note: having this method present prevents these warnings:
+// nsChildView.mm:3079: warning: incomplete implementation of class `ChildView'
+// nsChildView.mm:3079: warning: method definition for `-doCommandBySelector:'
not found
+// nsChildView.mm:3079: warning: class `ChildView' does not fully implement the
`NSTextInput' protocol
+- (void) doCommandBySelector:(SEL)aSelector;
+{
+  if ( [super respondsToSelector:@selector(doCommandBySelector)] )
+    [super doCommandBySelector:aSelector];
+}

The NSTextInput protocol docs say that your impl of this method should *not*
call super. This should probably just call [self performSelector:aSelector
withObject:nil afterDelay:0].

Fix those and r=sfraser

Comment 29

[Kathleen :Brade]

Attachment: updated patch to fix doCommandBySelector call

this patches fixes the doCommandBySelector implementation as suggested by
Simon; I will fix the memory allocation stuff if/when Conrad is able to land
that stuff

Comment 30

[Simon Fraser [no longer active]]

Comment on attachment 94993 [details] [diff] [review]
updated patch to fix doCommandBySelector call

sr=sfraser

Comment 31

[Kathleen :Brade]

The above patch has been checked in; remaining issues will be covered in bug 162666.