299603 - Use IM specified style for preedit string (It is better to use PANGO_ATTR_BACKGROUND for XIMReverse)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

User-Agent:       Mozilla/5.0 (X11; Linux i686; U;) Gecko/20050527 Kazehakase/0.2.7
Build Identifier: Mozilla/5.0 (X11; Linux i686; U;) Gecko/20050527 Kazehakase/0.2.7

If gtk_immodule sets the text foreground color in preediting text, Gecko makes
the text reversed (i.e. NS_TEXTRANGE_SELECTEDCONVERTEDTEXT or
NS_TEXTRANGE_SELECTEDRAWTEXT).
But I think the foreground color is set for improving visibility rather than
background color, so it's better to use PANGO_ATTR_BACKGROUND for figuring out
XIMReverse condition.


Reproducible: Always

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: using PANGO_ATTR_BACKGROUND

Comment 2

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

Hi, Ikezoe-san.

Katakai-san, Would you check the patch?

Comment 3

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

FYI:

http://lxr.mozilla.org/mozilla/source/widget/src/gtk2/nsWindow.cpp#5318
> 5318     /*
> 5319      * Depend on gtk2's implementation on XIM support.
> 5320      * In aFeedback got from gtk2, there are only three types of data:
> 5321      * PANGO_ATTR_UNDERLINE, PANGO_ATTR_FOREGROUND, PANGO_ATTR_BACKGROUND.
> 5322      * Corresponding to XIMUnderline, XIMReverse.
> 5323      * Don't take PANGO_ATTR_BACKGROUND into account, since
> 5324      * PANGO_ATTR_BACKGROUND and PANGO_ATTR_FOREGROUND are always
> 5325      * a couple.
> 5326      */

If we use the patch, we need to chenge this comment.

Comment 4

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

Now, we can fix this bug completely, I implemented the way for TSF. I'll work for this after bug 520732.

Comment 5

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

Attachment: Patch v1.0

simplest patch. but looks like there are a lot of a11y problems.

> data:text/html,<input style="background-color: blue;"><input style="background-color: black;"><input style="background-color: red;"><input style="background-color: green;"><br><input style="color: white; background-color: blue;"><input style="color: white; background-color: black;"><input style="color: white; background-color: red;"><input style="color: white; background-color: green;">

Comment 6

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

Attachment: Patch v1.1

fix a mistake.

Comment 7

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

Attachment: Patch v2.0

This improves some issues. However, seems that bug 553975 is a major a11y bug for this patch.

And also I need to fix a regression of bug 472410, first.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6c69799bdf3

Comment 9

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

Attachment: part.1 IMContextWrapper should have a method to initialize a TextRange

First, move the code initializing TextRange for a clause of composition string to a separated method since we need to reduce the indent level for avoiding redundant line breaking with 80 character per line rule.

This patch doesn't change any logic.

# If you don't like some variable/argument names, please give r+ with suggesting better names. I'll replace them in additional patch.

Comment 10

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

Attachment: part.2 IMContextWrapper::SetTextRange() should initialize the range offsets and fail if the range is collapsed

First, let's move the code computing offsets for using early return style. Additionally, empty range doesn't make sense except NS_TEXTRANGE_CARETPOSITION. So, in such case, SetTextRange() should do nothing.

Comment 11

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

Attachment: part.3 IMContextWrapper::SetTextRange() shold set the style of the range which is specified by the IME

This is the main patch of this bug. SetTextRange() should initialize TextRange::mRangeStyle with the visual information provided by PangoAttrs.

Comment 12

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

Attachment: part.4 Some global methods in nsTextFrame.cpp should be members of nsTextFrame class

This is preparation of next patch. nsTextFrame.cpp has some local static methods. However, following patch (and maybe future changes) needs forward declaration of the methods. For avoiding that and reducing nsIFrame argument, they should be members of nsTextFrame class.

This patch just cleans up the code, is not changing any logic.

Comment 13

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

Attachment: part.5 nsTextFrame should use system foreground or background color when painting IME selections if whose style defines only one of foreground color or background color

Some IMEs for Linux have a UI to customize the visual of composition string. IIRC, it may provide only foreground color or background color. Then, the color may be similar to the frame's foreground color or background color. If so, it's a serious a11ys problem, users may not be able to read the text due to insufficient contrast (foreground color vs. background color). For avoiding this scenario, nsTextFrame should assume that IME specifying style always assumes that the color which is not specified by IME is always the native widget's default color. Then, we can always provide sufficient contrast between foreground color and background color for each clause of IME selections.

Comment 14

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

Attachment: part.6 Guess the meaning of each clause in the composition string with caret position

This is an additional fix.

Currently, we decude TextRange::mRangeType with its visual style. However, this is not good for IME developers because they cannot set visual style as they want. So, we should decide the meaning of each clause with caret offset and composition string length.

The redundant static_cast are ugly, but they will be fixed by following patch.

Comment 15

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

Attachment: part.7 IMContextWrapper::CreateTextRange() should convert the caret offset from offset in characters to offset in UTF-16

When I wrote the previous patch, I realized this bug.

gtk_im_context_get_preedit_string() returns the caret position as offset in *characters*. But we treate it as offset in *UTF-16 string*. Therefore, if there is surrogate pairs before the caret, the caret is shown at wrong offset.

So, we should compute the offset in UTF-16 before using the caret offset returned by gtk_im_context_get_preedit_string().

FYI: We should use glib's methods for computing UTF-8 offset and converting UTF-8 to UTF16 since the cursor_position is assumed that the offset is used with them.

With this change, we can make the caret offset of SetTextRange() unsigned.

Comment 16

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

Attachment: part.8 Rename aLastDispatchedData with aCompositionString in IMContextWrapper::CreateTextRangeArray()

The reason of the name of CreateTextRangeArray()'s aLastDispatchedData is, we dispatched NS_COMPSOTION_UPDATE before calling CreateTextRangeArray(). Therefore, the last data was current composition string.

However, we don't dispatch NS_COMPOSITION_UPDATE from widget. It's automatically dispatched in XP level when widget dispatches NS_COMPOSITION_CHANGE (or NS_COMPOSITION_COMMIT). Therefore, now, aLastDispatchedData is odd name. We should rename it to aCompositionString. Then, the code becomes more natural.

Comment 17

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

Note that bug 560071 hasn't been fixed yet, however, same issue is existing on Windows because TSF mode uses TextRange::mRangeStyle. So, there is no reason that we won't support native look and feel of composition string on Linux.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8649277 [details] [diff] [review]
part.3 IMContextWrapper::SetTextRange() shold set the style of the range which is specified by the IME

Review of attachment 8649277 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/IMContextWrapper.cpp
@@ +93,5 @@
>    }
>    virtual ~GetWritingModeName() {}
>  };
>  
> +class GetTextRangeStyleText : public nsAutoCString

add final attribute?

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8649288 [details] [diff] [review]
part.7 IMContextWrapper::CreateTextRange() should convert the caret offset from offset in characters to offset in UTF-16

Review of attachment 8649288 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/IMContextWrapper.cpp
@@ +1523,5 @@
> +                MOZ_LOG(gGtkIMLog, LogLevel::Warning,
> +                    ("GTKIM: %p   CreateTextRangeArray(), WARNING, failed to "
> +                     "convert to UTF-16 string before the caret "
> +                     "(cursor_pos_in_chars=%d, caretOffset=%d)",
> +                     this, cursor_pos_in_chars, caretOffset));

In this place, it is possible that utf16StrBeforeCaret isn't nullptr.  Then we have to call g_free() at this place or after if condition, if not nullptr.

Comment 20

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

Thank you, roc and Makoto-san!

> ::: widget/gtk/IMContextWrapper.cpp
> @@ +1523,5 @@
>> +                MOZ_LOG(gGtkIMLog, LogLevel::Warning,
>> +                    ("GTKIM: %p   CreateTextRangeArray(), WARNING, failed to "
>> +                     "convert to UTF-16 string before the caret "
>> +                     "(cursor_pos_in_chars=%d, caretOffset=%d)",
>> +                     this, cursor_pos_in_chars, caretOffset));
> 
> In this place, it is possible that utf16StrBeforeCaret isn't nullptr.  Then we have to call
> g_free() at this place or after if condition, if not nullptr.

Wow, nice catch!

Comment 21

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1992d30ee3ff10c30577f5a9fff8fa97c1272c
changeset:  1a1992d30ee3ff10c30577f5a9fff8fa97c1272c
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.1 IMContextWrapper should have a method to initialize a TextRange r=m_kato

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/02ab221ff043df27e820b2630a412d796c8f1e8d
changeset:  02ab221ff043df27e820b2630a412d796c8f1e8d
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.2 IMContextWrapper::SetTextRange() should initialize the range offsets and fail if the range is collapsed r=m_kato

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/66c718b99d9482c8a272a00f14c154aa5347625a
changeset:  66c718b99d9482c8a272a00f14c154aa5347625a
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.3 IMContextWrapper::SetTextRange() shold set the style of the range which is specified by the IME r=m_kato

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd78357356b6e1e1ecd5b7e984d8914996b6f6c
changeset:  3dd78357356b6e1e1ecd5b7e984d8914996b6f6c
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.4 Some global methods in nsTextFrame.cpp should be members of nsTextFrame class r=roc

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f1bed2226ef68ee75e0684f710d75c2c58c93923
changeset:  f1bed2226ef68ee75e0684f710d75c2c58c93923
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.5 nsTextFrame should use system foreground or background color when painting IME selections if whose style defines only one of foreground color or background color r=roc

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3429abbd285209ad137af28dd6e8eeebf61448d5
changeset:  3429abbd285209ad137af28dd6e8eeebf61448d5
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.6 Guess the meaning of each clause in the composition string with caret position r=m_kato

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4d4ffa12b981258b98b6e8b9c0fbd737ce3deb
changeset:  5c4d4ffa12b981258b98b6e8b9c0fbd737ce3deb
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.7 IMContextWrapper::CreateTextRange() should convert the caret offset from offset in characters to offset in UTF-16 r=m_kato

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/706b23a03d1cc3fab884e1e9c3d25d5fe6ceeff0
changeset:  706b23a03d1cc3fab884e1e9c3d25d5fe6ceeff0
user:       Masayuki Nakano <[e-mail]>
date:       Wed Aug 19 16:37:39 2015 +0900
description:
Bug 299603 part.8 Rename aLastDispatchedData with aCompositionString in IMContextWrapper::CreateTextRangeArray() r=m_kato

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a1992d30ee3
https://hg.mozilla.org/mozilla-central/rev/02ab221ff043
https://hg.mozilla.org/mozilla-central/rev/66c718b99d94
https://hg.mozilla.org/mozilla-central/rev/3dd78357356b
https://hg.mozilla.org/mozilla-central/rev/f1bed2226ef6
https://hg.mozilla.org/mozilla-central/rev/3429abbd2852
https://hg.mozilla.org/mozilla-central/rev/5c4d4ffa12b9
https://hg.mozilla.org/mozilla-central/rev/706b23a03d1c