Closed Bug 1461307 Opened 7 years ago Closed 7 years ago

[IME] It is hard to read text because of the color of the background while converting the Roman character (or Roman alphabet) into Kanji (or Chinese character).

Categories

(Core :: Widget: Gtk, defect, P3)

Product:
Core
Component:
Widget: Gtk
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- fixed
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- verified

People

(Reporter: cgp, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(3 files)

Kazam_screencast_00027.mp4
55.63 KB, video/mp4
Details
fx.log
77.43 KB, text/plain
Details
Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView
59 bytes, text/x-review-board-request
karlt
: review+
lizzard
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr60+
Details
Attached video Kazam_screencast_00027.mp4 —
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID: 20180503143129 Steps to reproduce: Download and run the Firefox(ver60.0 64bit Linux). And type some Hiragana script into an element which can receive the user key-input like textboxes, and hit space key to complement. This will be caused in the environment not only in Japanese, but more some languages like Chinese. Linux debian 4.9.0-4-amd64 #1 SMP Debian 4.9.65-3+deb9u1 (2017-12-23) x86_64 GNU/Linux Actual results: When I begin converting the Roman character (or Roman alphabet) into Kanji (or Chinese character), the background of the converting text turns black. And it'll be hard to read the complement candidates. Please look through the attached videos for details. Expected results: The background color should be lighter as the Firefox I used before updates does.
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0 20180323154952 Works for me, testing on the search box at https://en.wikipedia.org Pressing Spacebar doesn't select the text or change its background color. Setting platform to Linux. Bug 560071 is possibly related.
Has STR: --- → yes
Component: Untriaged → Layout: Text
Flags: needinfo?(masayuki)
Keywords: inputmethod
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64
Summary: It is hard to read text because of the color of the background while converting the Roman character (or Roman alphabet) into Kanji (or Chinese character). → [IME] It is hard to read text because of the color of the background while converting the Roman character (or Roman alphabet) into Kanji (or Chinese character).
(In reply to Gingerbread Man from comment #1) > Bug 560071 is possibly related. Yeah, but basically, this is bug of the IME or its settings. Currently, we use colors given by IME to render composition string (bug 299603). IME specifies only background color of the selected clause (your selection background color?) or specifies both colors but as you see. Although for making sure contrast between foreground color and background color, IME should specify foreground color too. If the cause is latter, we cannot do anything since we need to trust Look and Feel information coming from IME.. With default settings on Debian 9, I don't see this bug. So, reporter, - Did you customize IME's Look and Feel settings? - Is the background color same as selected text's background color of your theme? If yes, what color is used for foreground color of selected text? - Do you use non-default theme for your desktop?
Flags: needinfo?(masayuki) → needinfo?(cgp)
- Did you customize IME's Look and Feel settings? I think I didn't do that. - Is the background color same as selected text's background color of your theme? If yes, what color is used for foreground color of selected text? No, the selected text's background color is blue. - Do you use non-default theme for your desktop? No. I'm using the default theme of LXDE.
Flags: needinfo?(cgp)
(In reply to cgp from comment #3) > - Do you use non-default theme for your desktop? > No. I'm using the default theme of LXDE. Hmm, I installed LXDE into my Debian 9.x environment, but I cannot reproduce it. In my environment, the selection background color is light-gray. Could you attach a log file of our IME handler on your environment? 1. Run: export MOZ_LOG=nsGtkIMModuleWidgets:5,sync export MOZ_LOG_FILE=~/fx.log ./firefox 2. Click searchbar to type something with IME. 3. Type something with IME and hit Space to reproduce this bug. 4. When you see the bug, close the window with clicking close button of the window. 5. Attach fx.log file in your home directory as attachment of this bug. Note that it logs all of what you typed. So, please don't enter something your privacy, e.g., passwords, etc during creating the log file.
Flags: needinfo?(cgp)
Attached file fx.log —
Flags: needinfo?(cgp)
Attachment #8976051 - Attachment mime type: text/x-log → text/plain
Thank you for the log. But according to the log, we've been ordered as rendered with the background color (#2e3436) by your IME. So, basically, we shouldn't include any hack for not restricting IME's behavior. However, if a lot of our uses meet this bug, we need some hack unfortunately. What colors do you see in other GTK applications like Gedit? Which IM do you use? Fcitx? ibus? XIM? (If you're not sure about IM, attaching same log with Nightly build including the information though.)
Flags: needinfo?(cgp)
The same bug didn't occur in Gedit. And I'm using Fcitx. Should I check some config files?
Flags: needinfo?(cgp)
Aside to that, the background color in Gedit is skyblue.
Thank you #23e436 is very dark color of skyblue. I guess that your IM might set alpha channel to the background color because we don't have alpha channel of IME selection color. I'll try to create a test build. > Should I check some config files? Not sure where it is. And I'd like you to test my test build with current environment.
The same phenomenon still occurs in that builds.
Flags: needinfo?(cgp)
Oh, thank you for your test... I'll try to look for another idea...
Priority: -- → P3
Since my bug was closed as a duplicate of this, I thought I'd add my two cents: 1. I'm experiencing the same bug with Firefox 61 with fcitx and KDE on Debian 9 "Stretch." 2. According to https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide there's supposed to be a bunch of prefs for setting IME colors in Firefox. They do nothing. 3. "Did you customize IME's Look and Feel settings?" The bug occurs with the default settings. The bug still occurs with a dummy all-red skin I made for testing. Only the IME box is affected by changing the fcitx skin, even in other applications. 4. "Is the background color same as selected text's background color of your theme?" No. My theme's selected color is white on blue. (Probably the same as cgp.) Firefox is giving me grey on black. GIMP tells me it's #31363b on #000000. (See image in https://bugzilla.mozilla.org/show_bug.cgi?id=1474181) 5. "Do you use non-default theme for your desktop?" No, I'm using the default Breeze theme. 6. "What colors do you see in other GTK applications like Gedit?" Other GTK applications give me the correct white on blue. Firefox is the only application that's wrong. 7. To me, the best solution would be to revive those prefs that presently don't do anything -- if they're set, display the colors designated in the prefs; if they're not set, fall back to the present method of trying to guess what the IME wants.
Sorry, I forgot something: 8. Changing my systemwide GTK theme (not KDE theme) to Breeze-Dark affects the preedit text -- it becomes a perfectly readable white on black. Unfortunately, I hate everything else about Breeze-Dark. I've tried dozens of ways to run Firefox with a specific GTK theme independent of the systemwide one (in hopes that I could then tinker with a custom theme until I came up with something that fixed this issue without looking awful), but none of them worked.
(In reply to cwbussard from comment #15) > Sorry, I forgot something: > > 8. Changing my systemwide GTK theme (not KDE theme) to Breeze-Dark affects > the preedit text -- it becomes a perfectly readable white on black. > Unfortunately, I hate everything else about Breeze-Dark. What theme do you use? https://www.gnome-look.org/content/show.php/Breeze+Dark?content=170330 or others?
Flags: needinfo?(cwbussard)
(In reply to Makoto Kato [:m_kato] from comment #16) > (In reply to cwbussard from comment #15) > > Sorry, I forgot something: > > > > 8. Changing my systemwide GTK theme (not KDE theme) to Breeze-Dark affects > > the preedit text -- it becomes a perfectly readable white on black. > > Unfortunately, I hate everything else about Breeze-Dark. > > What theme do you use? > https://www.gnome-look.org/content/show.php/Breeze+Dark?content=170330 or > others? I normally use the default Breeze theme. Breeze-Dark was just for testing this issue. I do not normally use it. (Note also that, because my desktop environment is KDE, most things are governed by the KDE theme rather than the GTK theme.)
Flags: needinfo?(cwbussard)
(In reply to cwbussard from comment #14) > 2. According to > https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide there's > supposed to be a bunch of prefs for setting IME colors in Firefox. They do > nothing. Yeah, pref values are used only when our IME handler cannot (or do not) retrieve composition string style from IME. (And some of the document is outdated, I should update it when I have much time...) > 3. "Did you customize IME's Look and Feel settings?" > The bug occurs with the default settings. > The bug still occurs with a dummy all-red skin I made for testing. Only the > IME box is affected by changing the fcitx skin, even in other applications. Oh. When I tried to create same environment as the reporter, I couldn't understand the fcitx-settings since I couldn't change any style of composition string with it. Then, the settings must be hidden by user actually, sigh.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #19) > Fcitx did something only for Firefox... > https://github.com/fcitx/fcitx/blob/0c87840dc7d9460c2cb5feaeefec299d0d3d62ec/ > src/frontend/gtk2/fcitximcontext.c#L763-L814 I went and downloaded some source packages. The block of code *is* in the source for the version of fcitx that ships with debian testing, but it is *not in* the version that ships with debian stable (which is what both cgp and I are using). Does anyone know offhand if fcitx can be upgraded without pulling in a bunch of system-critical dependencies?
Hmm, it seems that this is really shameful bug...
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #23) > test builds here: > x86: > https://queue.taskcluster.net/v1/task/TNbUX_XPTDiOW0cMz_kRgA/runs/0/ > artifacts/public/build/target.tar.bz2 > x86_64: > https://queue.taskcluster.net/v1/task/VcTejrnoRFaKjBFcmOC-JA/runs/0/ > artifacts/public/build/target.tar.bz2 > > Could you try this? I am sorry to say that it did not work.
Oh, okay, thanks. I'll keep investigating...
Does somebody let me know the original selection background color code and foreground color code, and composiiton background color code and composition foreground color code on Firefox? As far as the code of fcitx, they try to show composition string's selected clause as same as normal selection color. However, we fail to convert it to our internal CSS color. So, I'd like to investigate how we fail the conversion.
I chatted with Xuetian Weng who is hacker of fcitx about this issue. https://twitter.com/d_toybox/status/1017342876949217280 According to his explanation, this is theme's bug. We retrieve selection colors from TextEntry. However, fcitx retrieves selection colors from focused widget (window if on Firefox) to support any customized widget. So, if theme sets odd colors as selection colors of *window*, fcitx notifies us of the odd colors as selected clause color. I'll try to write a patch to overwrite selection colors of window.
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Component: Layout: Text → Widget: Gtk
Ever confirmed: true
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #28) > Could you somebody check one of these builds? > x86: > https://queue.taskcluster.net/v1/task/W5Q7EvP8SjKMpr4uLwfwUw/runs/0/ > artifacts/public/build/target.tar.bz2 > x86_64: > https://queue.taskcluster.net/v1/task/JIAP5U_8QtiItHjRrTKShQ/runs/0/ > artifacts/public/build/target.tar.bz2 The bug seems not to be fixed yet.
Thank you for the test, and sorry for the failure. Now, I succeeded to create an environment which shows similar composition string color. And I succeeded to fix this bug on this environment. So, I'd like to you check next coming test builds before patch review. (The fix depends on theme's style because current GTK widgets' look and feel are declared by CSS... So, overriding style *might* loose default style.)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #31) > x86: > https://queue.taskcluster.net/v1/task/e-CyQAjlS5erI0l8TNdYwg/runs/0/ > artifacts/public/build/target.tar.bz2 > x86_64: > https://queue.taskcluster.net/v1/task/epxY6jKRTn6o5rO0L-CQ1Q/runs/0/ > artifacts/public/build/target.tar.bz2 This build fixes the problem for me. Thank you for all of your hard work.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #31) > x86: > https://queue.taskcluster.net/v1/task/e-CyQAjlS5erI0l8TNdYwg/runs/0/ > artifacts/public/build/target.tar.bz2 > x86_64: > https://queue.taskcluster.net/v1/task/epxY6jKRTn6o5rO0L-CQ1Q/runs/0/ > artifacts/public/build/target.tar.bz2 (In reply to cwbussard from comment #32) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #31) > > x86: > > https://queue.taskcluster.net/v1/task/e-CyQAjlS5erI0l8TNdYwg/runs/0/ > > artifacts/public/build/target.tar.bz2 > > x86_64: > > https://queue.taskcluster.net/v1/task/epxY6jKRTn6o5rO0L-CQ1Q/runs/0/ > > artifacts/public/build/target.tar.bz2 > > This build fixes the problem for me. Thank you for all of your hard work. Hmm... the bug isn't fixed yet in my environment. I thought it's failed because I've tested only x86_64 version ever, so I tried x86 version but got an error. > XPCOMGlueLoad error for file /home/cgp/Downloads/firefox/libmozgtk.so: > libgtk-3.so.0: cannot open shared object file: No such file or directory > Couldn't load XPCOM. Is this also a little bug?
Sorry! I too only tested the x64 build. I just tested the x86 version and I got the same error message. In retrospect, this is not surprising because I don't have the x86 version of libgtk installed. In order to test the x86 firefox build, I think you'd need to set up a full x86 debian installation for the test environment.
Thank you for your testing! Currently, recommended release build for Linux is only x86_64. I just put link to x86 build for somebody who needs to run it. So, it's not problem if you still see the bug *only* with mismatched architecture build because such user must be rare but this bug is serious, so, this bug should be fixed. cgp: Do you still see this bug even with both x86 and x86_64 builds?
Flags: needinfo?(cgp)
To be clear: 1. The x64 test build fixes the bug for me. 2. I suspect that the bug *is* also fixed in the x86 test build; I just can't test it. I cannot run the x86 test build because an x64 Debian system does not meet the dependencies to run x86 Firefox. (Specifically, that error message is telling me that I don't have an x86 version of libgtk. Which is correct; I don't.)
> Do you still see this bug even with both x86 and x86_64 builds? As I mentioned before, I wasn't able to test the x86 version build (the error message doesn't seem to be a new bug?), and with regards to the x86_64 build, the bug isn't fixed yet in my environment...
Flags: needinfo?(cgp)
cgp: How about this build on your environment? I give higher priority to the style than the previous build: x86_64: https://queue.taskcluster.net/v1/task/V-VTYzy_RBaB4kDJuKBKuA/runs/0/artifacts/public/build/target.tar.bz2
Flags: needinfo?(cgp)
I'm sorry, I noticed that the installed version of Firefox is executed when I ran the received program. How can I stop searching installed Firefox in my system? The bug might have been fixed.
Flags: needinfo?(cgp)
(In reply to cgp from comment #39) > I'm sorry, I noticed that the installed version of Firefox is executed when > I ran the received program. > How can I stop searching installed Firefox in my system? > The bug might have been fixed. Copy the contents of /opt/firefox/ (or wherever your default installation resides) to a temporary directory. Overwrite /opt/firefox/ with the contents of the archive. To test, invoke your ordinary shortcut. (The test build will have the Firefox Nightly look-and-feel.) To revert, overwrite /opt/firefox/ with the contents of the temporary directory. (I recommend testing via your ordinary desktop shortcut so that you can be sure that all the environment your graphical desktop normally passes to Firefox gets passed during the test.)
(In reply to cgp from comment #39) > I'm sorry, I noticed that the installed version of Firefox is executed when > I ran the received program. > How can I stop searching installed Firefox in my system? > The bug might have been fixed. Ah, okay. I'll request reviews for the patch. The simplest and safest way to test new binary of Firefox is, unzip the package, then, execute "firefox" of the unzipped directory from terminal with --no-remote argument. For example, "./firefox -P test --no-remote". This command means that launch "firefox" with profile "test" and ignore communication with other Firefox instances and shell (e.g., won't open .html files even if default action of .html is set as "open with Firefox"). Anyway, if you close Firefox with Ctrl-q and double click firefox binary in the unzipped directory, that is enough. The former steps ensures that new firefox binary won't communicate with zombie process.
>cgp: >How about this build on your environment? I give higher priority to the style than the previous build: >x86_64: https://queue.taskcluster.net/v1/task/V-VTYzy_RBaB4kDJuKBKuA/runs/0/artifacts/public/build/target.tar.bz2 Thank you for giving me the way to test properly. I tested https://queue.taskcluster.net/v1/task/V-VTYzy_RBaB4kDJuKBKuA/runs/0/artifacts/public/build/target.tar.bz2 and made sure that the bug is fixed now. I can't test https://queue.taskcluster.net/v1/task/epxY6jKRTn6o5rO0L-CQ1Q/runs/0/ (the previous build) because of the disappearance of the destination of the URL, but I think that the bug is fixed in the previous build as cwbussard said. Thank you for your good work!
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView https://reviewboard.mozilla.org/r/258036/#review264974 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: widget/gtk/IMContextWrapper.cpp:318 (Diff revision 1) > + NS_GET_R(selectionBackgroundColor), > + NS_GET_G(selectionBackgroundColor), > + NS_GET_B(selectionBackgroundColor), alpha); > + } > + style.AppendLiteral("}"); > + gtk_css_provider_load_from_data(mProvider, style.get(), -1, NULL); Warning: Use nullptr [clang-tidy: modernize-use-nullptr] gtk_css_provider_load_from_data(mProvider, style.get(), -1, NULL); ^~~~~ nullptr
FYI: The share of fcitx in Nightly users is roughly 27%. I'm not sure how much user uses desktop themes which provides odd selection colors.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #27) > I chatted with Xuetian Weng who is hacker of fcitx about this issue. > According to his explanation, this is theme's bug. That explanation is helpful, thank you, but calling this the theme's bug is plain wrong. > [...] However, fcitx retrieves selection colors from > focused widget (window if on Firefox) to support any customized widget. The code used prior to https://github.com/fcitx/fcitx/commit/47764a98a41b58baa180e4bf774d7ccd17f154ce#diff-0d8c736d05403100895c44044eebf6b1R759 was designed for GTK2. Even for GTK2, it was not right because the "text" color should not be used with "bg". With GTK3, that code still returns values, but themes have no reason provide any particular values to such code because this is not the way that GTK gets selection colors from the theme. Based on the release tags that github shows on the above commit, this is fixed in fcitx 4.2.9.5. fcitx now has GTK3-specific code and the version added in https://github.com/fcitx/fcitx/commit/78b98d9230dc9630e99d52e3172bdf440ffd08c4 will now return the same values irrespective of the kind of widget, but it is still disabled for Firefox.
(In reply to cgp from comment #0) > The background color should be lighter as the Firefox I used before updates > does. Can you tell us the last version of Firefox that you know had a lighter background please?
Flags: needinfo?(cgp)
Keywords: regression
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView https://reviewboard.mozilla.org/r/258036/#review265942 ::: commit-message-5a810:5 (Diff revision 2) > +Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GTK Entry r?karlt > + > +IME (e.g., fcitx) may refer selection colors of widget under window which > +is associated with IM context to support any colored widget. So, IME > +expects good selection colors which have sufficient constranst between s/contranst/contrast/ ::: commit-message-5a810:6 (Diff revision 2) > +foreground and background, and also selection background color and > +widget background color like Entry widget of GTK. However, some desktop Why is it important to have selection background color like that of GtkEntry, if nsTextFrame will swap as necessary? Similarly, why is the unselected widget background color important? Would any selection colors with sufficient contrast work? I understand that colors from the theme would be preferable to random colors with sufficient contrast, but this comment implies that there is another reason. The purpose of using colors from GtkEntry would then be that GtkEntry is a widget which can be expected to have suitable selection colors set. This patch is actually using the colors from a GtkTextView. That is fine, but the comments should align. ::: commit-message-5a810:10 (Diff revision 2) > +IMContextWrapper expects that IME specifies composition string colors > +is useful in Entry widget of GTK because even if our editor is customized > +by web app's style sheet, nsTextFrame swaps foreground and background > +color of selection if selection background color and editor background > +color are similar. I don't understand this paragraph. Doesn't the fact that nsTextFrame swaps foreground and background colors when background colors are similar mean that IMContextWrapper has no specific expectations about where the composition colors would be useful? ::: commit-message-5a810:10 (Diff revision 2) > +IMContextWrapper expects that IME specifies composition string colors > +is useful in Entry widget of GTK because even if our editor is customized > +by web app's style sheet, nsTextFrame swaps foreground and background > +color of selection if selection background color and editor background > +color are similar. ::: widget/gtk/IMContextWrapper.cpp:227 (Diff revision 2) > + * IME (e.g., fcitx) may refer selection colors of widget which is related to > + * the window associated with the IM context to support any colored widgets. s/refer/look up/ comma after widget. comma after context. ::: widget/gtk/IMContextWrapper.cpp:227 (Diff revision 2) > const static bool kUseSimpleContextDefault = false; > > /****************************************************************************** > + * SelectionStyleProvider > + * > + * IME (e.g., fcitx) may refer selection colors of widget which is related to This is a workaround for something specific to fcitx, and so please identify the affected fcitx version(s). I expect this approach will be fine, but it is quite possible that one day other code may have different preferences for the window/container colors, and so we'll want to know when this can be removed. ::: widget/gtk/IMContextWrapper.cpp:229 (Diff revision 2) > + * IMContextWrapper can assume that colors specified by IME is always redable > + * in our editor because nsTextFrame swaps background and foreground colors of > + * composition string if backgroud color of composition string does not have > + * sufficient contrast with background color of text node in the editor. IIUC having nsTextFrame swap background and foreground colors doesn't change the readability of the text because that depends on the contrast of the background and foreground colors of the composition, not the background color of the text node? If that is the case, then I suggest these edits: s/is always redable/are always visually distinct/ s/backgroud/background/ ::: widget/gtk/IMContextWrapper.cpp:233 (Diff revision 2) > + * Therefore, if IME specifies colors of composition string as it's in Entry > + * widget of GTK. Therefore, we should override selection colors of the I don't understand this sentence. The "if" suggests a condition, but there is no "then". I wonder whether you mean "the IME specifies colors of composition string as if it's in a GtkEntry widget", but, if so, I don't understand why that should follow from the sentence about swapping in nsTextFrame. I don't see where the IME specifies colors as if it's being used with a GtkEntry widget. ::: widget/gtk/IMContextWrapper.cpp:254 (Diff revision 2) > + return sInstance; > + } > + > + static void Shutdown() > + { > + if (sInstance && sInstance->mProvider) { No need for null check on mProvider. gtk_css_provider_new() will not return null. This would be clearer with the g_object_unref() in the destructor to balance with the constructor. ::: widget/gtk/IMContextWrapper.cpp:264 (Diff revision 2) > + MozContainer* container = aWindow->GetMozContainer(); > + MOZ_ASSERT(container, "container is null"); > + // This window is associated with IM context. > + GdkWindow* gdkWindow = gtk_widget_get_window(GTK_WIDGET(container)); nsWindow is used here only to find the client window, but the caller also finds the client window. Instead, pass the client window from the caller. ::: widget/gtk/IMContextWrapper.cpp:269 (Diff revision 2) > + MozContainer* container = aWindow->GetMozContainer(); > + MOZ_ASSERT(container, "container is null"); > + // This window is associated with IM context. > + GdkWindow* gdkWindow = gtk_widget_get_window(GTK_WIDGET(container)); > + GtkWidget* widget = nullptr; > + // gdk_window_get_user_data() is typically returns pointer to Delete "is". ::: widget/gtk/IMContextWrapper.cpp:281 (Diff revision 2) > + void OnThemeChanged() > + { Please indicate what exactly fcitx is doing, so that we know what exactly needs to be done to provide the desired colors. These are the key lines GtkStyle* style = gtk_widget_get_style(widget); fg = style->text[GTK_STATE_SELECTED]; bg = style->bg[GTK_STATE_SELECTED]; The bug there is that style->text should be used with style->base. style->fg should be used with style->bg. fcitx is confusing the two pairs of colors. gtk_style_update_from_context() will set these colors using the widget's GtkStyleContext and so the colors can be controlled by a ":selected" CSS rule. ::: widget/gtk/IMContextWrapper.cpp:283 (Diff revision 2) > + // Unfortunately, we don't know how important the style rules specified > + // by the theme. We should specify pseudo-classes as far as possible > + // here to give higher priority to our style rules. It would be reasonable to assume that theme styles have priority GTK_STYLE_PROVIDER_PRIORITY_THEME. https://developer.gnome.org/gtk3/stable/GtkStyleProvider.html#GTK-STYLE-PROVIDER-PRIORITY-FALLBACK:CAPS implies that higher priority will override regardless of specificity, which is consistent with usual CSS cascade. ::: widget/gtk/IMContextWrapper.cpp:286 (Diff revision 2) > + void OnThemeChanged() > + { > + // Unfortunately, we don't know how important the style rules specified > + // by the theme. We should specify pseudo-classes as far as possible > + // here to give higher priority to our style rules. > + nsAutoCString style("window widget:selected," Whether the widget owning the window is a GtkWindow or MozContainer depends on the parameter passed to gtk_widget_set_has_window() here: https://searchfox.org/mozilla-central/source/widget/gtk/nsWindow.cpp#3890 When the MozContainer does not have its own window, gdk_window_get_user_data() will return the GtkWindow. The style context will not have a "widget" object name. "Note that a style provider added by this function only affects the style of the widget to which context belongs" and so there is no need to match the object name. Instead just match ":selected". ::: widget/gtk/IMContextWrapper.cpp:294 (Diff revision 2) > + "window widget:hover:selected," > + "window widget:active:focus:selected," > + "window widget:active:hover:selected," > + "window widget:focus:hover:selected," > + "window widget:active:focus:hover:selected{"); > + // FYI: LookAndFeel always returns selection colors of Entry widget. LookAndFeel returns colors for GtkTextView ::: widget/gtk/IMContextWrapper.cpp:302 (Diff revision 2) > + LookAndFeel::eColorID_TextSelectForeground, > + &selectionForegroundColor))) { > + double alpha = > + static_cast<double>(NS_GET_A(selectionForegroundColor)) / 0xFF; > + style.AppendPrintf("color:rgba(%u,%u,%u,%f);", > + NS_GET_R(selectionForegroundColor), NS_GET_R casts to uint8_t, so this should be cast to unsigned for %u. ::: widget/gtk/IMContextWrapper.cpp:324 (Diff revision 2) > + } > + > +private: > + static SelectionStyleProvider* sInstance; > + static bool sHasShutDown; > + GtkCssProvider* mProvider; const, and use a member initializer in the constructor. ::: widget/gtk/IMContextWrapper.cpp:475 (Diff revision 2) > > void > IMContextWrapper::Init() > { > + // Overwrite selection colors of the window before associating the window > + // with IM context since IME may refer selection colors via IM context to s/refer/look up/ ::: widget/gtk/IMContextWrapper.cpp:584 (Diff revision 2) > mSimpleContext, mDummyContext, > gtk_im_multicontext_get_context_id(GTK_IM_MULTICONTEXT(mContext)), > PR_GetEnv("XMODIFIERS"))); > } > > +void Please add /* static */ as this differs from Init().
Attachment #8993264 - Flags: review?(karlt) → review-
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView https://reviewboard.mozilla.org/r/258036/#review265942 > Why is it important to have selection background color like that of GtkEntry, > if nsTextFrame will swap as necessary? > > Similarly, why is the unselected widget background color important? > > Would any selection colors with sufficient contrast work? I understand that > colors from the theme would be preferable to random colors with sufficient > contrast, but this comment implies that there is another reason. > > The purpose of using colors from GtkEntry would then be that GtkEntry is a > widget which can be expected to have suitable selection colors set. > > This patch is actually using the colors from a GtkTextView. > That is fine, but the comments should align. Right. I update the commit message. > I don't understand this paragraph. > > Doesn't the fact that nsTextFrame swaps foreground and background colors when > background colors are similar mean that IMContextWrapper has no specific > expectations about where the composition colors would be useful? nsTextFrame expects that default editor's background color (coming from LookAndFeel) and composition colors (coming from IME) must have enough contrast (and also of course composition string colors are readable). Okay, I update the comment. > This is a workaround for something specific to fcitx, and so please identify the affected fcitx version(s). > > I expect this approach will be fine, but it is quite possible that one day > other code may have different preferences for the window/container colors, and > so we'll want to know when this can be removed. Sure. Looks like that ~4.2.8.3 referred our widget's selection colors, and perhaps, after fixing this bug, fcitx will restart to refer selection colors of our widget too (I'll request it). > I don't understand this sentence. The "if" suggests a condition, but there is > no "then". I wonder whether you mean "the IME specifies colors of composition > string as if it's in a GtkEntry widget", but, if so, I don't understand why > that should follow from the sentence about swapping in nsTextFrame. > > I don't see where the IME specifies colors as if it's being used with a GtkEntry widget. Oh, sorry. I meant: Therefore, if IME specifics colors of composition as it's in GtkTextView, composition string must be readable in our editor. However, anyway, those comment is not good, I rewrite it. > Please indicate what exactly fcitx is doing, so that we know what exactly > needs to be done to provide the desired colors. These are the key lines > > GtkStyle* style = gtk_widget_get_style(widget); > fg = style->text[GTK_STATE_SELECTED]; > bg = style->bg[GTK_STATE_SELECTED]; > > The bug there is that style->text should be used with style->base. > style->fg should be used with style->bg. fcitx is confusing the two pairs of > colors. > > gtk_style_update_from_context() will set these colors using the widget's > GtkStyleContext and so the colors can be controlled by a ":selected" CSS > rule. Oh. I didn't realize the wrong pair issue. I'll file an issue for that (the latest code still have GTK2 code). > It would be reasonable to assume that theme styles have priority > GTK_STYLE_PROVIDER_PRIORITY_THEME. > > https://developer.gnome.org/gtk3/stable/GtkStyleProvider.html#GTK-STYLE-PROVIDER-PRIORITY-FALLBACK:CAPS > implies that higher priority will override regardless of specificity, which is > consistent with usual CSS cascade. Ah, I meant that "priority" is in CSS rules: https://www.w3.org/TR/selectors/#specificity Perhaps, I should've used "specificity" here. > NS_GET_R casts to uint8_t, so this should be cast to unsigned for %u. > NS_GET_R casts to uint8_t, Yes, > so this should be cast to unsigned for %u. I don't understand this. I understand that %u is available for uint8_t, uint16_t and uint32_t. Do you mean, %u should be %hhu? > const, and use a member initializer in the constructor. If you meant|const GtkCssProvider*|, it causes compile errors with calling some APIs with mProvider. I change this as |GtkCssProvider* const|.
Mainly, I updated the comments (rewrote). However, I didn't change only one point, that is %u vs. uint8_t. Karl, could you check the new patch?
Hmm, all repositories of fcitx are archived in github... So, I cannot file new issue for the wrong usage GtkStyle.
(In reply to Karl Tomlinson (:karlt) from comment #48) > (In reply to cgp from comment #0) > > The background color should be lighter as the Firefox I used before updates > > does. > > Can you tell us the last version of Firefox that you know had a lighter > background please? Firefox 52.9.0esr.
Flags: needinfo?(cgp)
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView https://reviewboard.mozilla.org/r/258036/#review265942 > > NS_GET_R casts to uint8_t, > > Yes, > > > so this should be cast to unsigned for %u. > > I don't understand this. I understand that %u is available for uint8_t, uint16_t and uint32_t. Do you mean, %u should be %hhu? Sorry, my understanding was incorrect. I thought the variadic argument types always needed to be compatible to avoid problems like those when passing NULL as a pointer sentinel. https://ewontfix.com/11/: "As it turns out, while x86_64 (and most 64-bit archs) uses entire registers or 64-bit stack slots for all arguments, even ones smaller than 64-bit, there's no guarantee in the ABI that the unused upper bits will be cleared by the caller." However, I wasn't aware of two things: Integer promotion is applied to bool, char, and short arguments, which would convert uint8_t arguments to int. https://en.cppreference.com/w/cpp/language/variadic_arguments#Default_conversions Even though int is not compatible with unsigned, there is an exception for this case when the value is representable in both types, as it is when the int value comes from uint8_t. https://en.cppreference.com/w/cpp/utility/variadic/va_arg "If the type of the next argument in ap (after promotions) is not compatible with T, the behavior is undefined, unless: * one type is a signed integer type, the other type is the corresponding unsigned integer type, and the value is representable in both types; or * one type is pointer to void and the other is a pointer to a character type (char, signed char, or unsigned char). " So now I don't know why %hu and %hhu exist, but perhaps that is historical. The code you have here is correct and good, thank you.
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView https://reviewboard.mozilla.org/r/258036/#review265942 > nsTextFrame expects that default editor's background color (coming from LookAndFeel) and composition colors (coming from IME) must have enough contrast (and also of course composition string colors are readable). > > Okay, I update the comment. Ah, so I guess colors are not swapped in native widgets? https://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#4004-4006 > Ah, I meant that "priority" is in CSS rules: https://www.w3.org/TR/selectors/#specificity > Perhaps, I should've used "specificity" here. GTK uses a cascade in much the same way as the web uses a cascade, but there are different sources of rules involved. Specificity is only relevant for rules from the same kind of source. On the web, rules from the user agent style sheet always override author rules, whether they have higher or lower specificity. In GTK, application rules always override theme rules, whether they have higher or lower specificity.
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView https://reviewboard.mozilla.org/r/258036/#review267204 Thank you. This is much easier to understand now. ::: commit-message-5a810:12 (Diff revisions 2 - 3) > -widget background color like Entry widget of GTK. However, some desktop > -themes set selection colors of our widget to unreadable pair of colors. > +widget background color like GtkTextView. However, some desktop themes > +set our widget to different selection colors from GtkTextView which may > +be unreadable. > > -IMContextWrapper expects that IME specifies composition string colors > -is useful in Entry widget of GTK because even if our editor is customized > +nsTextFrame (which paints composition string) expects that composition > +string colors coming from IME is enough readable and background color is enough readable -> are sufficiently readable ::: widget/gtk/IMContextWrapper.cpp:286 (Diff revisions 2 - 3) > void OnThemeChanged() > { > + // fcitx refers GtkStyle::text[GTK_STATE_SELECTED] and > + // GtkStyle::bg[GTK_STATE_SELECTED] (although pair of text and *base* > + // or *fg* and bg is correct). gtk_style_update_from_context() will > + // these colors using the widget's GtkStyleContext and so the colors *set* these colors
Attachment #8993264 - Flags: review?(karlt) → review+
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView https://reviewboard.mozilla.org/r/258036/#review265942 > Ah, so I guess colors are not swapped in native widgets? > https://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#4004-4006 Yes, it is. Our computation of contrast and the threshold to decide "sufficient" came from old accessibility document of W3C. However, we met some major themes of GTK which do not clear the threshold. However, we can assume that theme's background color and selection background color are always have sufficient contrast since user accepts the colors. Therefore, when we decide the threshold, we also uses the difference of selection background color and the background color of the theme. > GTK uses a cascade in much the same way as the web uses a cascade, but there > are different sources of rules involved. > Specificity is only relevant for rules from the same kind of source. > On the web, rules from the user agent style sheet always override author > rules, whether they have higher or lower specificity. > In GTK, application rules always override theme rules, whether they have > higher or lower specificity. Thanks! I'll remove the comment and I'll change it only use ":selected".
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/6cdfa85af851 Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView r=karlt
https://hg.mozilla.org/mozilla-central/rev/6cdfa85af851
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
I confirmed that the patch can be grafted cleanly with both beta and ESR60. So, if we won't get any regression reports for a while, we should try to uplift them.
Could you verify if the latest Nightly build fixed this bug? https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly
Flags: needinfo?(cwbussard)
Flags: needinfo?(cgp)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #64) > Could you verify if the latest Nightly build fixed this bug? > https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly I made sure that the bug has been fixed.
Flags: needinfo?(cgp)
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView Approval Request Comment [Feature/Bug causing the regression]: Regression might be caused by Desktop theme's update in some environment of Linux. [User impact if declined]: Maybe hard to read inputting string which needs to be converted to proper Kanji characters. [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Not necessary. [List of other uplifts needed for the feature/fix]: Nothing. [Is the change risky?]: *Might* be. [Why is the change risky/not risky?]: An IME, fcitx, tells us color of composition string. fcitx tries to specify selection colors to a clause in composition string which is now being converted. When it retrieves selection colors, fcitx refers selection colors of a widget which is created by Gecko. We have expected that selection colors of our widget is always same as selection colors in GtkTextView (i.e., same color as input field of native GTK applications). However, such odd themes set selection colors of our widget odd. Especially, its background color and foreground color are too similar. Therefore, in some environments, users cannot read composition string of fcitx only on Firefox. The patch overwrites the selection colors with selection colors for GtkTextView by ourselves. Note that most desktop themes set selection colors of our widget to same as selection colors of GtkTextView, so, this change should not cause any regressions even with some external applications like a11y tools. [String changes made/needed]: Nothing.
Attachment #8993264 - Flags: approval-mozilla-beta?
marking verified in nightly per comment 65.
status-firefox63: fixed → verified
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView Fix for IME contrast problem, let's uplift for beta 16.
Attachment #8993264 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is this something we should be considering for ESR60 as well?
Flags: needinfo?(masayuki)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #70) > Is this something we should be considering for ESR60 as well? That's a good point. Debian stable ships with ESR by default. Right now this is only a problem for people like cgp and myself who run the current firefox build instead. When Debian stable switches over to ESR60, a whole lot more people will be affected.
Flags: needinfo?(cwbussard)
Yeah, I'll check all new bugs on Linux after I've landed it. Then, I'll request the approval.
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is important bug for a11y of IME users on specific environment. We've only reported from Debian users, and Debian users ESR. So, before they updating their ESR from 52 to 60, we should fix this bug. User impact if declined: If user uses odd desktop theme which set our widget's selection color to different color from selection colors in text editor of native GTK widget, fcitx renders composition string with our widget's selection colors. However, in the reported environment, the colors are unreadable pair since our widget's selection colors usually not used. Fix Landed on Version: 62 Beta and 63 Nightly. Risk to taking this patch (and alternatives if risky): If other a11y tools refer the selection colors of our widget, their result may become different. However, as I said above, the color combination is not readable on the reported environment. So, overwriting our widget's selection colors with selection colors of editor of native GTK widget must be safe. String or UUID changes made by this patch: No.
Flags: needinfo?(masayuki)
Attachment #8993264 - Flags: approval-mozilla-esr60?
Comment on attachment 8993264 [details] Bug 1461307 - Overwrite selection colors of widget which may be referred by IME via IM context with selection colors of GtkTextView Fixes a Linux-only issue that's likely to burn users migrating from ESR52 to ESR60. Approved for ESR 60.2.
Attachment #8993264 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Setting qe- based on comment 66
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: