236996 - Japanese IME preedit text should be displayed underlined, not negative/inverting

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[Katsuhiro MIHARA]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040309
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040309

The present, Mozilla displays Japanese preedit text negative.
Safari and Internet Explorer displayes Japanese preedit text underlined.
Unless reasons, same behavior is desired.


Reproducible: Always
Steps to Reproduce:
1.enter Japanese text to textfield or textarea.
2.
3.

Actual Results:  
Mozilla displays Japanese preedit negative.

Expected Results:  
Mozilla should displays Japanese preedit underlined.

Comment 1

[Hiro]

duplicate bug 170951 ?

Comment 2

[Katsuhiro MIHARA]

Sorry. [e-mail]-san, you are right.
This bug seems to be dup of bug 170951 .

Comment 3

[Katsuhiro MIHARA]

Sorry for bug spam.


*** This bug has been marked as a duplicate of 170951 ***

Comment 4

[HARUNAGA Hirotoshi]

This is slightly different from bug 170951. Mac only.

steps to reproduce:
1. turn on IME (ex. Kotoeri) on Mozilla Mac build
2. type something

Result:
The raw text is displayed as inverting and underlined in red.

Expected:
The text should be underlined in black, not inverted, like as other
applications on Mac.
And Mac users expect that if they select a syllable for converting,
the syllable is underlined in black and unselected text is underlined
in gray.

reopening.

Comment 5

[Hiro]

FYI.
About Camino, this problem fixed by following patch.

bug232406
https://bugzilla.mozilla.org/attachment.cgi?id=163309&action=view

Comment 6

[waverider]

Attachment: patch

note:
nsTextFrame.cpp part is not necessary to fix this bug.
But NS_RGB(255,198,198) is very hard to see when background color is white.
So we should changed it.

Comment 7

[waverider]

about comment#6
This patch is about Camino (cocoa widget).

Comment 8

[Mike Pinkerton (not reading bugmail)]

who should review this?

Comment 9

[Jungshik Shin]

Comment on attachment 163972 [details] [diff] [review]
patch

This patch needs to be updated to the current trunk first.

Comment 10

[waverider]

Now, I write new patch and test it.
Please wait...

Comment 11

[waverider]

Attachment: new patch

add part of MacOSX IME style.

Comment 12

[waverider]

Attachment: preview

patch is still imcomplete, but improve appearance of pending text.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 171252 [details] [diff] [review]
new patch

This patch looks good for me.

We should re-implement this patch in bug 170951 after this bug is fixed.
I will try to fix bug 170951 after 1.8 final.
But I think this bug should be fixed before 1.8 final.

waverider:
Separate this patch for layout/generic and widget/src/cocoa.
Because the reviewers of these files are different people.
You should attach two patches and you change these flags to:

layout/generic
  review&superreview = [e-mail]
widget/src/cocoa
  review = [e-mail]

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops...

waverider:

See comment 13.
You still have work.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think the color is wrong.

> +  #define IME_RAW_COLOR NS_RGB(149,149,149)  //light gray
> +  #define IME_CONVERTED_COLOR NS_RGB(149,149,149) //light gray
> +  #define IME_SELECTED_CONVERTED_COLOR NS_RGB(0,0,0) //black

I think these colors should be...

#define IME_COLOR NS_RGB(149,149,149) //light gray
#define IME_SELECTED_COLOR NS_RGB(0,0,0) //black

and

case nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT:
case nsISelectionController::SELECTION_IME_SELECTEDCONVERTEDTEXT:

should be used IME_SELECTED_COLOR and

case nsISelectionController::SELECTION_IME_RAWINPUT:
case nsISelectionController::SELECTION_IME_CONVERTEDTEXT:

should be used IME_COLOR.

Comment 16

[waverider]

Attachment: I delete widget/cocoa part and correct macro name.

re:comment#15

color is wrong?
I think you mean to say "macro name is wrong".
If not, Please tell me right color.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #16)
> re:comment#15
> 
> color is wrong?
> I think you mean to say "macro name is wrong".
> If not, Please tell me right color.

See attachment 171260 [details], the "After"'s above case underline-color differs from
"MacOSX"'s above case underline-color.
I said this.
But, currently, attachment 172446 [details] [diff] [review] is looks good.

Comment 18

[waverider]

Attachment: correct careless miss at nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT:

change FillRect(aX + startOffset+size, aY + baseline - offset,
textWidth-2*size, size);
to
FillRect(aX + startOffset+size, aY + baseline - offset, textWidth-2*size,
2*size);

Comment 19

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

Masayuki, can you make sure waverider requests reviews when he's ready? Thanks!!

Comment 20

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

or she, as the case may be

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 172665 [details] [diff] [review]
correct careless miss at nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT: 

You changed the underline height. But I disagree it. Because if underline is
thin on Mac, you should change |GetUnderline|.
http://lxr.mozilla.org/mozilla/source/gfx/src/mac/nsFontMetricsMac.cpp#295
But it is not this bug. You should open a new bug.

Note that your patch separates the code to MacOS X and others. But these codes
will be recombined by bug 170951. Therefore, these codes difference should be
only colors.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> you should change |GetUnderline|.

Or you should add new function. E.g., |GetUnderlineOfCompositionString|.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops...
http://lxr.mozilla.org/mozilla/source/gfx/src/mac/nsFontMetricsMac.cpp#295
may not be for MacOS X.
I don't know which nsFontMetrics* is for MacOS X.

Comment 24

[waverider]

re:comment#21, #22
I want to change underline height about IME's pending text.
If we |GetUnderline| changed, all underline(include hyperlink) is changed.
I'm sorry to say, but this is wrong way.
and I hope that you explain an advantages of adding new function.
If adding MacOS X's look and feel to generic/nsTextFrame.cpp is not recommended, 
How can we solve this bug?

re:comment#23
That source is used at MacOS X.

Comment 25

[waverider]

Attachment: brushed.

Comment 26

[waverider]

Attachment: brushed more.

Comment 27

[waverider]

Attachment: Please correct, the teacher.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +#ifdef XP_MACOSX // underline thickness is 2 pixel
> +             aRenderingContext.FillRect(aX + startOffset+size, aY + baseline
- offset, textWidth-size, 2*size);

textWidth-size -> textWidth-2*size

> +#else
> +             aRenderingContext.FillRect(aX + startOffset+size, aY + baseline
- offset, textWidth-size, size);
> +#endif

same.

> +           case nsISelectionController::SELECTION_IME_RAWINPUT:
> +           case nsISelectionController::SELECTION_IME_CONVERTEDTEXT:{
> +              aTextStyle.mNormalFont->GetUnderline(offset, size);
> +              aRenderingContext.SetColor(IME_UNDERLINECOLOR);
> +#ifdef XP_MACOSX // underline thicness is 2 pixel
> +              aRenderingContext.FillRect(aX + startOffset+size, aY + baseline
- offset, textWidth-size, 2*size);

same.

> +#else
> +              aRenderingContext.FillRect(aX + startOffset+size, aY + baseline
- offset, textWidth-size, size);

same.

Comment 29

[waverider]

Attachment: Please correct, the teacher.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 174821 [details] [diff] [review]
Please correct, the teacher.

looks good for me.

Please re-attache the other patch(nsChildView.mm) and you should request review
for it.

Comment 31

[waverider]

Thanks a lot.

https://bugzilla.mozilla.org/show_bug.cgi?id=280200
We already done it.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc:

Could you check-in the patch?

Comment 33

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

checked in