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

Hi, Ikezoe-san. Katakai-san, Would you check the patch?

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.

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

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;">

fix a mistake.

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.

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.

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.

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

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.

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.

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.

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.

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.

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 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 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.

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!

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1992d30ee3ff10c30577f5a9fff8fa97c1272c changeset: 1a1992d30ee3ff10c30577f5a9fff8fa97c1272c user: Masayuki Nakano <masayuki@d-toybox.com> 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 <masayuki@d-toybox.com> 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 <masayuki@d-toybox.com> 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 <masayuki@d-toybox.com> 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 <masayuki@d-toybox.com> 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 <masayuki@d-toybox.com> 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 <masayuki@d-toybox.com> 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 <masayuki@d-toybox.com> date: Wed Aug 19 16:37:39 2015 +0900 description: Bug 299603 part.8 Rename aLastDispatchedData with aCompositionString in IMContextWrapper::CreateTextRangeArray() r=m_kato

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