Closed Bug 1158076 Opened 9 years ago Closed 7 years ago

Use light GTK theme for web content

Categories

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

Product:
Core
Component:
Widget: Gtk
Version:
37 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: piotr.halasiewicz, Assigned: stransky)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

(Whiteboard: tpi:+)

Attachments

(10 files, 13 obsolete files)

Screenshot of the system color preferences
265.68 KB, image/png
Details
gtk3bug.png
215.33 KB, image/png
Details
patch
121.14 KB, patch
karlt
: feedback+
Details | Diff | Splinter Review
dark theme patch for dev edition v2
96.18 KB, patch
Details | Diff | Splinter Review
dark_theme_v2.patch
119.25 KB, patch
Details | Diff | Splinter Review
dark-theme-complete.patch
165.79 KB, patch
Details | Diff | Splinter Review
WIP light theme for content process
5.96 KB, patch
Details | Diff | Splinter Review
ff5101_ubuntu_1304_gtk3x.png
46.26 KB, image/png
Details
Bug 1158076 - add prefs to enable GTK dark themes in each process,
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1158076 - postpone nsLookAndFeel module initialization,
59 bytes, text/x-review-board-request
karlt
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150415140819

Steps to reproduce:

I have Fedora 22 (beta), with default Adawita dark theme in Gnome 3.16.1 
The Firefox is from Fedora Repository 37.0.2


Actual results:

On some pages, in some text fields, white fonts appear on white background.
It happens in facebook.com Update status field and duckduckgo.com search field among others.


Expected results:

It should be either dark font on white background or white font on dark background.
It was suggested by Cork on Firefox IRC channel, that it is most likely a gtk3 theming bug
Blocks: gtk3
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
(In reply to piotr.halasiewicz from comment #1)
> It was suggested by Cork on Firefox IRC channel, that it is most likely a
> gtk3 theming bug

That is probably the case: when use of system colors is disabled (browser.display.use_system_colors is set to false), the fonts are no longer of the wrong color. You can use this as a temporary workaround.
(In reply to Rastus Vernon from comment #2)
> (In reply to piotr.halasiewicz from comment #1)
> > It was suggested by Cork on Firefox IRC channel, that it is most likely a
> > gtk3 theming bug
> 
> That is probably the case: when use of system colors is disabled
> (browser.display.use_system_colors is set to false), the fonts are no longer
> of the wrong color. You can use this as a temporary workaround.

Thanks for your help!
I tried it: In about:config I set browser.display.use_system_colors to false and even after browser restart, the problem with fonts stays the same :(
(In reply to piotr.halasiewicz from comment #3)
> (In reply to Rastus Vernon from comment #2)
> > (In reply to piotr.halasiewicz from comment #1)
> > > It was suggested by Cork on Firefox IRC channel, that it is most likely a
> > > gtk3 theming bug
> > 
> > That is probably the case: when use of system colors is disabled
> > (browser.display.use_system_colors is set to false), the fonts are no longer
> > of the wrong color. You can use this as a temporary workaround.
> 
> Thanks for your help!
> I tried it: In about:config I set browser.display.use_system_colors to false
> and even after browser restart, the problem with fonts stays the same :(

What happens if you open the preferences, open the colors dialog in the content tab and uncheck the checkbox to use system colors? This is how I initially fixed the problem, but I was later able to toggle it one way and the other through browser.display.use_system_colors in about:config.
There is no change in behavior, when I toggle this check in Preferences, nor there is when I do it through about:config.
If you mean the checkbox marked on the screenshot attached.
(In reply to piotr.halasiewicz from comment #5)
> There is no change in behavior, when I toggle this check in Preferences, nor
> there is when I do it through about:config.
> If you mean the checkbox marked on the screenshot attached.

Yes, that is the checkbox I was talking about. From what I understand so far, there are two problems, only one of which is fixed by disabling system colors.

If you do a search on Google or Startpage (this does not happen with DuckDuckGo), you'll notice that the description of search results is white on white. Disabling system colors fixes this. However, it does not fix the problem of white on white in text fields.
Yes, indeed, the same bahavior here.
Comboboxes are covered by Bug 1161056.
Karl, what do you think here? Should be the font color settings (system vs. user-defined) applied to elements like entry boxes too?

Some web pages (duckduckgo for instance) defines white background for imput elements but rely that text color is always black - which is not true for dark themes. The text color for those fields can't be controlled by the font color settings now.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(karlt)
(In reply to Martin Stránský from comment #10)
> Karl, what do you think here? Should be the font color settings (system vs.
> user-defined) applied to elements like entry boxes too?

I would expect user-defined settings to apply to entry boxes
(though I'm not very familiar with those settings).

> Some web pages (duckduckgo for instance) defines white background for imput
> elements but rely that text color is always black - which is not true for
> dark themes. The text color for those fields can't be controlled by the font
> color settings now.

That might be an evangelism bug, but perhaps a workaround for such sites is possible.  An ideal solution would not require setting browser.display.use_system_colors.

Perhaps, if the foreground color has not been set by the page, then system colors should only be used if they contrast with the background, or perhaps only if the background color has not been set by the page.

ISTR some logic do disable native themes on widgets that are styled by the web page.  Perhaps this just needs to be extended to also disable the system foreground color on page-styled widgets.
Flags: needinfo?(karlt)
(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> > Some web pages (duckduckgo for instance) defines white background for imput
> > elements but rely that text color is always black - which is not true for
> > dark themes. The text color for those fields can't be controlled by the font
> > color settings now.
> 
> That might be an evangelism bug, but perhaps a workaround for such sites is
> possible.  An ideal solution would not require setting
> browser.display.use_system_colors.
> 
> Perhaps, if the foreground color has not been set by the page, then system
> colors should only be used if they contrast with the background, or perhaps
> only if the background color has not been set by the page.

Actually the page presumption has some reason. According to the http://www.w3.org/TR/html5/rendering.html#the-css-user-agent-style-sheet-and-presentational-hints the default color is expected to be black and background color transparent.

So it looks correct to me to apply GTK styles to xul only...web designers don't expect black combo/entry boxes on the web pages and it looks a bit weird....
Attached image gtk3bug.png — 
Is this only limited to the default distro dark themes?  
I tried Numix dark theme and on youtube, facebook and text does appear in a ghostly white.

Also tooltips, such as the pop-up's when you mouse-over a bug number on bugzilla, have the text on a transparent background.(Fine on Adwaita Dark Theme though)
I just put a 15$ bounty on bountysource on this issue :P I know it's not much but I want to contribute the way I can. https://www.bountysource.com/issues/20492493-white-fonts-on-white-background-in-gnome-3-16-1-dark-theme/backers
The same issues happens on Windows with dark themes (I tested FF38 on Win7) so it's not Linux only. 

The thing is that -moz-FieldText / -moz-Field is fetched from GtkView (white text on black background) and that color is used by XUL/HTML where the background color is redefined by web page to some white variant.

As a workaround you can try http://forums.fedoraforum.org/showpost.php?p=1703251&postcount=3
(In reply to Martin Stránský from comment #15)
> As a workaround you can try
> http://forums.fedoraforum.org/showpost.php?p=1703251&postcount=3

Awesome! Thank you! It works :D I will post here if I have any issues with this workaround.
(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> Perhaps, if the foreground color has not been set by the page, then system
> colors should only be used if they contrast with the background, or perhaps
> only if the background color has not been set by the page.
> 
> ISTR some logic do disable native themes on widgets that are styled by the
> web page.  Perhaps this just needs to be extended to also disable the system
> foreground color on page-styled widgets.

I believe we should not style widgets rendered in HTML pages - it breaks the page layout a web designers can't anticipate various OS themes.
Black form widgets really look bit out of place on light websites.

Moreover, even if web designer tries to intentionally set white background for form fields, that will make it impossible to distinguish select boxes from text inputs, as the arrows in select boxes are white and therefore not visible on light background.

I'm really struggling to use Firefox right now. Every other web site with forms is broken.
(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> ISTR some logic do disable native themes on widgets that are styled by the
> web page.  Perhaps this just needs to be extended to also disable the system
> foreground color on page-styled widgets.

IMHO it's possible to use a fixed (light) theme for web pages and os-based theme for XUL. There may be two widget styles, one for XUL and one for HTML and rendering can be switched by actual frame which provides the "in-html" info. Foreground text is controlled by CSS so we may have fixed CSS rules for in-html elements. The dark themes are pretty common in Gtk3 so it makes sense to fix that...
Attached patch wip patch (obsolete) — — Splinter Review 
There's a possible solution. This patch uses light theme for HTML elements (buttons and entry boxes now) plus sets css colors accordingly for HTML. Karl, what do you think about this approach? IMHO there must be two widgets (one for default and one for light theme) because the CSS provider is not affected by gtk_style_context_save/restore.
Attachment #8625748 - Flags: feedback?(karlt)
Comment on attachment 8625748 [details] [diff] [review]
wip patch

I think this is one of a few possible approaches.  I'm not marking f+ or f- because I don't really know what will work best.  However, I'm OK if you choose this kind of approach, taking into account my comments below.

Another approach would be to use an explicit dark foreground color if no foreground color style has been explicitly set on the widget and the background color style has been set.
Or another might be to disable native theming and colors for problem widgets
and dark themes in content.

Also, fixing browser.display.use_system_colors to work for textfields would
improve the situation considerably.

If going with the approach in this patch, then the distinction should be
chrome/content rather than xul/(x)html elements, because html elements are sometimes used in chrome.

Also, the user-selected gtk theme should be used (rather than Adwaita or
default) even in content if it is not a dark theme.  If the theme is a
dark variant then the light (or default) variant of the theme should be used.

I'm not sure, but perhaps setting gtk-application-prefer-dark-theme for the
application to override user config may be an option: "Dark themes should not
be used for documents, where large spaces are white/light and the dark chrome
creates too much contrast (web browser, text editor...)."

Try to avoid boolean function parameters.  Options to consider are bit flags,
enums, and/or another member on GtkWidgetState.
Attachment #8625748 - Flags: feedback?(karlt)
Thanks for the feedback.

> Another approach would be to use an explicit dark foreground color if no
> foreground color style has been explicitly set on the widget and the
> background color style has been set.
> Or another might be to disable native theming and colors for problem widgets
> and dark themes in content.

I was considering that but there are some complications. The background color detection may not be reliable - some sites use background image (duckduckgo). It also brings inconsistency when some elements on the page are light and some dark when web designers expect all elements as black-on-white.

Also can we just set a background color of elements rendered by Gtk? AFAIK the widgets can be either rendered and themed by gtk or rendered/styled by CSS engine (which gives them the old-school look :)).

> Also, fixing browser.display.use_system_colors to work for textfields would
> improve the situation considerably.

I see. The text fields are covered by --textarea-text-color from my patch. Can be extended to honor browser.display.use_system_colors. There are also "Color look-and-feel prefs" (http://www-archive.mozilla.org/unix/customizing.html) which we may resurrect.

> If going with the approach in this patch, then the distinction should be
> chrome/content rather than xul/(x)html elements, because html elements are
> sometimes used in chrome.

Okay.

> Also, the user-selected gtk theme should be used (rather than Adwaita or
> default) even in content if it is not a dark theme.  If the theme is a
> dark variant then the light (or default) variant of the theme should be used.

Okay.

> I'm not sure, but perhaps setting gtk-application-prefer-dark-theme for the
> application to override user config may be an option: "Dark themes should not
> be used for documents, where large spaces are white/light and the dark chrome
> creates too much contrast (web browser, text editor...)."

I see. Yes, that makes sense. But I see lots of people running FF Dark theme which dims UI and stands up actual web content.

> Try to avoid boolean function parameters.  Options to consider are bit flags,
> enums, and/or another member on GtkWidgetState.

Okay.
Attached patch wip v.2 (obsolete) — — Splinter Review 
It covers almost all widgets (tested on bugzilla). Needs some polish, chrome/content check, get rid of the boolean params. I wonder if there's some good way how to handle the HTML variants....may we store them as a data of the main ones or something?
Attachment #8625748 - Attachment is obsolete: true
Attached patch patch — — Splinter Review 
Next one. The inhtml is a part of GtkWidgetState and boolean otherwise.

> If going with the approach in this patch, then the distinction should be
> chrome/content rather than xul/(x)html elements, because html elements are
> sometimes used in chrome.

It's covered by IsFrameContentNodeInHTML(). How can I get the chrome/content? I haven't found that in nsFrame/nsContent interfaces. The recent implementation causes that sliders in textareas(input) are still rendered as in XUL. All other elements seems to be correctly rendered.

> Also, the user-selected gtk theme should be used (rather than Adwaita or
> default) even in content if it is not a dark theme.  If the theme is a
> dark variant then the light (or default) variant of the theme should be used.

It brings some difficulties - a) how reliably get current theme name, b) how to detect it's a dark theme c) if it's a dark theme it may not have a light variant.

IMHO it's the best to use default Gtk3 light theme for the inhtml elements and we can ensure some common web experience across themes then. The recent gtk3 default theme is Adwaita but there may be some run time switch.
Attachment #8629337 - Attachment is obsolete: true
Attachment #8637797 - Flags: feedback?(karlt)
Attached patch dark theme patch for dev edition (obsolete) — — Splinter Review 
The patch didn't apply in the dev edition branch, so I took the liberty and the changes necessary in order for it to apply ( Mostly apply by hand ).

I'm currently building and I'll comment once it finishes if it works.
(In reply to alferpal from comment #25)
> so I took the liberty and the changes 

"So I took the liberty and MADE* the changes"...
Last patch was missing the include of nsIContentInlines.h on widget/gtk/nsNativeThemeGTK.cpp

After including that to the patch, I can confirm that it builds, and solves the problem at least on youtube

For developer edition, that is
Attachment #8639206 - Attachment is obsolete: true
I'm not entirely sure this ever worked properly before. Firefox doesn't use GTK3 for a long time yet, does it? (regarding the regression marking)
You're right, this is lot older than our GTL3 build.
No longer blocks: ship-gtk3
Keywords: regression
(In reply to Milan Sreckovic [:milan] from comment #30)
> You're right, this is lot older than our GTL3 build.

Well.... It worked well on GTK2, on GTK3 it does not.
Doesn't this make it a regression? I think I don't understand your point.
(In reply to andrei from comment #31)
> Well.... It worked well on GTK2, on GTK3 it does not.
> Doesn't this make it a regression? I think I don't understand your point.

AFAIK the distinction here is between dark and light themes.
If the GTK2 theme were changed to a dark theme then the same issue would exist.

Themes can be configured per application.
There are some hints on how to do that here:
http://unix.stackexchange.com/questions/14129/gtk-enable-set-dark-theme-on-a-per-application-basis#answer-24071

(The environment variable is not what you want because it will be inherited by
child processes.)
(In reply to Martin Stránský from comment #22)
> > Another approach would be to use an explicit dark foreground color if no
> > foreground color style has been explicitly set on the widget and the
> > background color style has been set.
> > Or another might be to disable native theming and colors for problem widgets
> > and dark themes in content.
> 
> I was considering that but there are some complications. The background
> color detection may not be reliable - some sites use background image
> (duckduckgo).

I don't think it would be necessary to detect the color, just whether a
background or image has been set.  IIUC we do something similar to decide whether to natively draw the widget background.

> It also brings inconsistency when some elements on the page
> are light and some dark when web designers expect all elements as
> black-on-white.

If designers set backgrounds on some elements but not others, then there will
be inconsistency, but I expect we already have that through choosing to
natively style the widget or not.

> Also can we just set a background color of elements rendered by Gtk? AFAIK
> the widgets can be either rendered and themed by gtk or rendered/styled by
> CSS engine (which gives them the old-school look :)).

I don't know but I suspect the similar issues with consistency if modifying
styles in themes.
Comment on attachment 8637797 [details] [diff] [review]
patch

(In reply to Martin Stránský from comment #24)
> > If going with the approach in this patch, then the distinction should be
> > chrome/content rather than xul/(x)html elements, because html elements are
> > sometimes used in chrome.
> 
> It's covered by IsFrameContentNodeInHTML(). How can I get the
> chrome/content? I haven't found that in nsFrame/nsContent interfaces.

aFrame->PresContext()->IsChrome() is easiest AFAIK.

There's also nsContentUtils::IsChromeDoc().

> The recent implementation causes that sliders in textareas(input) are still
> rendered as in XUL.

Is that a problem?  Are the scrollbars still visible and usable?

> > Also, the user-selected gtk theme should be used (rather than Adwaita or
> > default) even in content if it is not a dark theme.  If the theme is a
> > dark variant then the light (or default) variant of the theme should be used.
> 
> It brings some difficulties - a) how reliably get current theme name, b) how
> to detect it's a dark theme c) if it's a dark theme it may not have a light
> variant.
> 
> IMHO it's the best to use default Gtk3 light theme for the inhtml elements
> and we can ensure some common web experience across themes then. The recent
> gtk3 default theme is Adwaita but there may be some run time switch.

Making everyone use a fallback theme would be an overcorrection.

GTK3 has established a convention for selecting dark themes through the
variant.  gtk-application-prefer-dark-theme is probably good enough to
automatically detect for most users.  If there are really popular themes not
using this convention, then I expect some heuristics will suffice.

It should be possible to get the current theme name through the same process
as GTK.  I guess that is GtkSettings and the environment variable.

>diff --git a/layout/style/forms.css b/layout/style/forms.css

>+*|*:root {
>+%ifdef MOZ_WIDGET_GTK
>+  --button-text-color: black;
>+  --button-text-color-hover: black;
>+  --button-text-color-active: black;
>+  --textarea-text-color: black;
>+  --textarea-text-background: white;
>+  --combobox-text-color: black;
>+  --combobox-text-background: white;
>+%else
>+  --button-text-color: ButtonText;
>+  --button-text-color-hover: ButtonText;
>+  --button-text-color-active: ButtonText;
>+  --textarea-text-color: -moz-FieldText;
>+  --textarea-text-background: -moz-Field;
>+  --combobox-text-color: -moz-ComboboxText;
>+  --combobox-text-background: -moz-Combobox;
>+%endif
>+}

We need some way do differentiate chrome and content.

Perhaps there is a common css file that can override this for chrome, but
maybe LookAndFeel needs a way to handle this anyway.

>     case eIntID_ScrollArrowStyle:
>         moz_gtk_init();
>+        /* TODO - we need to honour inhtml/xul styles here */

Are there other LookAndFeel properties that need to be from the same theme as
the drawing?

>     case eIntID_ImagesInButtons:
>-        aResult = moz_gtk_images_in_buttons();
>+        // TODO - do we want to honour HTML/XUL here?

I guess this one doesn't need to use the different theme.  It doesn't affect
readability, I assume, so better to go with the user's preferred theme.

Restricting the light theme to the variant of the dark theme may be
enough to mean we can assume that some of these properties don't depend on
which theme is used.

> static GtkWidget* gButtonWidget;
>+static GtkWidget* gButtonWidgetHTML;
> static GtkWidget* gToggleButtonWidget;
>+static GtkWidget* gToggleButtonWidgetHTML;

One way to reduce the verbosity a bit here might be to make each of these an
array.  It can be accessed by an enum to distinguish between chrome and
content, and that enum can be passed as a parameter instead of the boolean.
That may also be useful to avoid duplicating code for creating the more
complicated widgets.  Both elements can hold references to the same widget, if
that helps.

I think it's worth avoiding creating duplicate widgets when it is not
necessary to use the default style provider.

The change from ensure_* to get_* (when practical) is good, thanks.

I wasn't aware that this was necessary for so many widgets.  With scrollbars,
for example, is it necessary to use a different (light) theme in content?

If it is easier, then feel free to start with a patch for only some widgets,
and add the remaining once the pattern is clear.
Attachment #8637797 - Flags: feedback?(karlt) → feedback+
(In reply to Karl Tomlinson (ni?:karlt) from comment #34)
> Perhaps there is a common css file that can override this for chrome, but
> maybe LookAndFeel needs a way to handle this anyway.
> 
> >     case eIntID_ScrollArrowStyle:
> >         moz_gtk_init();
> >+        /* TODO - we need to honour inhtml/xul styles here */
> 
> Are there other LookAndFeel properties that need to be from the same theme as
> the drawing?

The system colors like ButtonText et al need to return a color suitable for use with the native background of the theme, so that content that explicitly chooses both native background and foreground continues to be readable.
https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#System_Colors
Depends on: 1216658
Got a workaround on Fedora 22 with Gnome Shell: make a desktop file that sets the GTK3 theme to light variant and runs Firefox

$cat ~/.local/share/applications/firefox.desktop

[Desktop Entry]
Version=1.0
Name=Firefox
GenericName=Web Browser
Comment=Browse the Web
Exec=bash -c 'GTK_THEME=Adwaita:light firefox %u'
Icon=firefox
Terminal=false
Type=Application
MimeType=text/html;text/xml;application/xhtml+xml;application/vnd.mozilla.xul+xml;text/mml;x-scheme-handler/http;x-scheme-handler/https;
StartupNotify=true
Categories=Network;WebBrowser;
Keywords=web;browser;internet;
(In reply to Karl Tomlinson (ni?:karlt) from comment #34)
> >diff --git a/layout/style/forms.css b/layout/style/forms.css
> 
> >+*|*:root {
> >+%ifdef MOZ_WIDGET_GTK
> >+  --button-text-color: black;
> >+  --button-text-color-hover: black;
> >+  --button-text-color-active: black;
> >+  --textarea-text-color: black;
> >+  --textarea-text-background: white;
> >+  --combobox-text-color: black;
> >+  --combobox-text-background: white;
> >+%else
> >+  --button-text-color: ButtonText;
> >+  --button-text-color-hover: ButtonText;
> >+  --button-text-color-active: ButtonText;
> >+  --textarea-text-color: -moz-FieldText;
> >+  --textarea-text-background: -moz-Field;
> >+  --combobox-text-color: -moz-ComboboxText;
> >+  --combobox-text-background: -moz-Combobox;
> >+%endif
> >+}
> 
> We need some way do differentiate chrome and content.
> 
> Perhaps there is a common css file that can override this for chrome, but
> maybe LookAndFeel needs a way to handle this anyway.

This is the hard part here. Do you mind if I add an extra function for that? Like this one:
http://mxr.mozilla.org/mozilla-central/source/widget/LookAndFeel.h#535 with bool aUseContentColor or so. I didn't find any other way how to get the theme info to LookAndFeel.

Another option is to define extra colors, like ButtonTextChrome, ButtonBackgroundChrome and so...
Flags: needinfo?(karlt)
(In reply to Martin Stránský from comment #38)
> This is the hard part here. Do you mind if I add an extra function for that?
> Like this one:
> http://mxr.mozilla.org/mozilla-central/source/widget/LookAndFeel.h#535 with
> bool aUseContentColor or so. I didn't find any other way how to get the
> theme info to LookAndFeel.
> 
> Another option is to define extra colors, like ButtonTextChrome,
> ButtonBackgroundChrome and so...

If the intention is to change most widgets, not just a few known problem
widgets, to a light-background theme, then changing the function signature
sounds much better than adding new colors.  I think that may work best with a
single forms.css regardless of the number of new colors.

The signature of the method with the boolean parameter can be changed to
accept a flags parameter possibly containing eUseStandinsForNativeColors.
(A new overloaded method with a boolean of different name would not be
distinguishable.)  That can be in a separate patch.

http://hg.mozilla.org/mozilla-central/annotate/eec9a0bfd929/widget/LookAndFeel.h#l535

A separate flag can be added for eUseLightBackgroundTheme or
eUseThemeForForeignContent, or similar.

I'm not clear how many non-color properties (eIntID_ScrollArrowStyle
perhaps) should come from the theme that is drawing the widget.
I suspect we'll need this for some of the int properties eventually.
A flags parameter can be similarly added to GetInt().

If there are many properties that depend on the theme rather than the
GtkSettings directly, then it may be an alternative to modify
nsLookAndFeel::GetInstance() or a nsLookAndFeel version to accept a flags
parameter to determine which of two nsLookAndFeel instances to return.
Depends on: 232227
Flags: needinfo?(karlt)
(In reply to Karl Tomlinson (ni?:karlt) from comment #39)
> If there are many properties that depend on the theme rather than the
> GtkSettings directly, then it may be an alternative to modify
> nsLookAndFeel::GetInstance() or a nsLookAndFeel version to accept a flags
> parameter to determine which of two nsLookAndFeel instances to return.

I meant to say nsXPLookAndFeel::GetInstance() or an nsLookAndFeel version.
Thanks. There's a preview of the widget part. There are some aspect I'd like to clarify:

- the dark/light widgets are stored in arrays as you suggested. Only the default variant is created when dark theme is inactive. This selection is controlled by GtkWidgetThemeLight/GtkWidgetThemeNum indexes. The theme itself is also selected by array index (GtkWidgetThemeDefault/GtkWidgetThemeLight).

- The light theme is loaded as light variant of active theme in moz_gtk_init(). AFAIK this was introduced in gtk 3.12.

- do we need to clear widgets in moz_gtk_shutdown()?

I'll divide the patch to smaller pieces when those are sorted out.
Attachment #8695243 - Flags: feedback?(karlt)
See Also: → 70315
Comment on attachment 8695243 [details] [diff] [review]
dark_theme_v2.patch

New version is coming.
Attachment #8695243 - Flags: feedback?(karlt)
Just tell us when we should test nightly :)
Meet the same problem on Arch Linux with firefox 43.0-1 and 43.0-2.

Since 43.0, firefox is affected by gtk global dark theme. Not sure when it starts on other linux distributions.

In my opinion, web pages should not be affected by system themes, at least color and background-color shouldn't be affected. Inheritance is a key feature of CSS, but current behavior breaks inheritance in some way and therefore makes some web pages broken.
See Also: → 1187679
Hi Karl,

I finally manage to get it working. There are some tricky pieces here:

- light widget are created for dark-themes only (see comment 41)
- widgets needs to be released in moz_gtk_shutdown()
- the light theme is used from dtk3drawing and nsLookAndFeel so I moved it to extra files Theme.h(c)
- nsXPLookAndFeel colors are cached which needs to be handled here. I added extra cache for content colors.
- I didn't manage to get correct -moz-dialogtext/-moz-dialog content/chrome distinction so it's returned chrome only which may be correct anyway. May be case of other colors.
- there's also a bug in gtkButton code which is workarounded to get GtkLabel text color

If you agree with the overall approach I'll submit separated patches for it. Thanks!
Attachment #8706555 - Flags: feedback?(karlt)
I'm experienced this only now, using FF 45.0.1.  And only on my bank account which uses javascript forms and relies totally on javascript...  Perhaps because I've been using:

% cat .mozilla/firefox/fls5gbxr.default/chrome/userContent.css
input, textarea, select {
	-moz-appearance: none !important;
	background-color: white;
	color: black;
}

a[class="file"],
a[class="dir"],
a[class="symlink"] {
	color: #2EB8E6 !important;
}

a:visited[class="file"],
a:visited[class="dir"],
a:visited[class="symlink"] {
	color: #FF6666 !important;
}

But that's NOT working anymore on my bank account, :-(
Does this bug/patch cover the widgets whose apparence is controlled by website CSS ?

For example, okta.com uses:

>   body, input, textarea, select {
>        color: #5e5e5e;
>    }

Which makes widgets impossible to read with dark background.

Or for the Fastmail.com compose window which has:

>    body {
>        color: #1f1f1f;
>    }
> 
>    select, input, button, textarea, pre {
>        color: inherit;
>    }

If the plan is go to white background, then I guess the answer is yes ;)

Thanks for pushing this forward!
As an additional feedback:

* the websites that don't define an explicit body background color (eg. to white) are not reabable. For example http://www.servicedenuages.fr
* the Gmail loading page has the opposite side-effect: text is gray on light-gray.
Blocks: 1268338
It looks good on youtube, duck duck go, facebook, fpaste.org and also in bug #627699 like in the screenshot. I would mark this as resolved/fixed.
I still get this issue with Ubuntu GNOME 16.04 with GNOME 3.20 and Firefox version 46.0.1 with the Global Dark Theme enabled. I get it on many sites including YouTube.
(In reply to Nikita Yerenkov-Scott from comment #50)
> I still get this issue with Ubuntu GNOME 16.04 with GNOME 3.20 and Firefox
> version 46.0.1 with the Global Dark Theme enabled. I get it on many sites
> including YouTube.

Please update firefox and try again. It should be gone now.
See Also: → 1283086
FF 47.0.1
Ubuntu 13.04
GNOME 3.6.2

After the update all text in input boxes were invisible (i.e. white text on white background).
Highlighting the text makes it visible. 
This included address and search bar.
Changed FF themes (currently using "White.") and those now show. 
However on some sites the issue remains in the form input boxes.
Comment on attachment 8706555 [details] [diff] [review]
dark-theme-complete.patch

With e10s now being progressively rolled out, each process will be either
chrome or content and so a much simpler approach like
attachment 8753289 [details] [diff] [review] (with problems resolved) should be sufficient.

Sorry that I didn't think of that before.
Attachment #8706555 - Flags: feedback?(karlt)
(In reply to Karl Tomlinson (:karlt) from comment #53)
> With e10s now being progressively rolled out, each process will be either
> chrome or content ...

Blocking bug 1264543 to make sure we don't break this once OOP compositor lands.
Blocks: e10s-gpu
For a workaround on input text fields and dropdown menus: 

1) Install Stylish Addon: http://addons.mozilla.org/en-US/firefox/addon/stylish/?src=search
2) Install this style: http://userstyles.org/styles/131351/linux-firefox-text-field-correction
Blocks: 1297246
Martin, are you still working on this?
Flags: needinfo?(stransky)
(In reply to Jim Mathies [:jimm] from comment #56)
> Martin, are you still working on this?

Not now, feel free to take it.
Flags: needinfo?(stransky)
(In reply to Jim Mathies [:jimm] from comment #56)
> Martin, are you still working on this?

Actually I'm going to look at it again and update the patch for latest trunk and WidgetCache.
Assignee: nobody → stransky
Depends on: 1303310
btw. Bug 1303310 and Bug 1282753 need to be fixed first.
Depends on: 1282753
Priority: -- → P2
Whiteboard: tpi:+
Blocks: 1305431
Summary: White fonts on white background in Gnome 3.16.1 dark theme. → Use light GTK theme for web content
Comment on attachment 8840799 [details] [diff] [review]
WIP light theme for content process

> +    gboolean contentProcess = false;

Note: Just realized that the default should be "true" here to disable dark theme by default.
Comment on attachment 8840799 [details] [diff] [review]
WIP light theme for content process

Try looks to be green. Karl what do you think about this patch?
Attachment #8840799 - Flags: feedback?(karlt)
Screeenshots of Ubuntu 13.04 with theme Ambiance (fail) and high contrast (works) for download, password and about "popups" using white font on white background
Hello,

facing the same I generated a "stylish" fix which by 95% - unfortunatly the

* download popup (pressing the download button)
* password saving popup
* about popup left to url combobox

looks to be generated dynamically since I cannot recognize with DOM Insepctor and Inspector  
See attachment 8841280 [details]
ff5101_ubuntu_1304_gtk3x.png
https://bugzilla.mozilla.org/attachment.cgi?id=8841280

Question: Is it correct, that these popups get generated dynamically - and is there a chance to change them dynamically using stylish etc ?

below my temporary workaround for "stylish" - probably reusable for others facing the issue 

regards
Frank 

/********************************************************** 
   stylish modifications for 
   Ubuntu 13.04 and Firefox 46 and higher to fix GTK 3.0 
   issues regarding wrong color representation
*/

.tree-rows
{
     color: #404040; 
}

input 
{
     color: #404040;
}

select 
{
     color: #404040; 
}

textarea 
{
     color: #404040;   
} 

.topContainer 
{
	color: #404040 !important;
}

.autocomplete-richlistitem 
{
/*     padding-top:2px!important; 
     padding-bottom:2px!important;
  */
     background-color: #41403b !important;
}

.autocomplete-richlistitem[selected="true"] 
{
     color: #ffffff !important;
     background-color: #f28559 !important;
}


.aboutPageWideContainer
{
     color: #404040; 
}


#bookmarks-view, #historyTree treechildren 
{
     color:#404040!important;
}


@-moz-document url("chrome://browser/content/pageinfo/pageInfo.xul") 
{
     #topBar .paneSelector 
     {
         color: #404040; 
     }

     #viewGroup > radio 
     {
          color: #404040; 
     }

     #viewGroup > radio:hover 
     {
          color: #404040; 
     }

     #viewGroup > radio[selected="true"] 
     {
         -moz-appearance: none !important;
         color: #404040; 
     }


     #metatree .tree-rows
     {
          color: #404040; 
     }

     #imagetree .tree-rows
     {
          color: #404040; 
     }

     #permList
     {
          color: #404040; 
     }

     groupbox 
     {
         -moz-appearance: none;
          color: #404040; 
     }
}


@-moz-document url("chrome://browser/content/places/places.xul") 
{
     .tree-rows
     {
          color: #404040; 
     }
  
     .downloadTarget
     {
          color: #404040; 
     }

     .downloadDetails
     {
          color: #404040; 
     }    
}

@-moz-document url("chrome://passwordmgr/content/passwordManager.xul") 
{
     .tree-rows
     {
      color: #404040; 
     }
}

       
@-moz-document url("chrome://passwordmgr/content/passwordManagerExceptions.xul") 
{
     .tree-rows
     {
         color: #404040; 
     }
}

@-moz-document url("chrome://pippki/content/certManager.xul") 
{
     .tree-rows
     {
         color: #404040; 
     }
}

@-moz-document url("chrome://pippki/content/certManager.xul") 
{
     .tree-rows
     {
         color: #404040; 
     }

}


@-moz-document url("chrome://inspector/content/inspector.xul") 
{
     .tree-rows
     {
         color: #404040; 
     }
}


/********************************************************** /
 
shows
Comment on attachment 8840799 [details] [diff] [review]
WIP light theme for content process

No need for GetThisProcessName() because XRE_IsContentProcess() is already
available for that purpose.

The effect of dropping the BrowserTabsRemoteAutostart() check is that the
parent process does not know whether content will be in a child process and so
will use a dark theme whether it renders content or not.  (Using
GetThisProcessName() does not change that.)

I expect it is possible to keep BrowserTabsRemoteAutostart().  The best way to
do that depends on first understanding the problems that result when calling that early.

If you don't have time to investigate the problems with
BrowserTabsRemoteAutostart() and so are considering dropping that, then the
question is whether the benefit of having dark chrome for e10s outweighs the
cost of having dark content for non-e10s users.  I don't mind, so I'm happy to
go with your opinion on whether that is net win.

On a related issue, I've been thinking about dropping native theming in
content altogether.  This would solve some problems with sandboxing and reduce
the burden of maintaining native widgets.  I'm not sure, but I guess there
exists a modern set of styles for default widget rendering on Android.  If so,
perhaps the same could be used for content on Linux.
Attachment #8840799 - Flags: feedback?(karlt)
(In reply to frank.scherie from comment #63)
> Created attachment 8841280 [details]
> ff5101_ubuntu_1304_gtk3x.png
> 
> Screeenshots of Ubuntu 13.04 with theme Ambiance (fail) and high contrast
> (works) for download, password and about "popups" using white font on white
> background

If that is happening with Ambiance and no add-ons or customization, then that is a different bug, because that would need a different fix.  However, Ubuntu 13.04 end of life was 2014-01, so that's unlikely to get fixed if the problem does not demonstrate on supported Ubuntu versions.
(In reply to Karl Tomlinson (:karlt) from comment #65)
> Comment on attachment 8840799 [details] [diff] [review]
> WIP light theme for content process
> 
> No need for GetThisProcessName() because XRE_IsContentProcess() is already
> available for that purpose.
> 
> The effect of dropping the BrowserTabsRemoteAutostart() check is that the
> parent process does not know whether content will be in a child process and
> so
> will use a dark theme whether it renders content or not.  (Using
> GetThisProcessName() does not change that.)

I don't understand the mechanism here. Is that possible that web content 
is rendered by both parent (chrome) and child processes when e10s is enabled?

> If you don't have time to investigate the problems with
> BrowserTabsRemoteAutostart() and so are considering dropping that, then the
> question is whether the benefit of having dark chrome for e10s outweighs the
> cost of having dark content for non-e10s users.  I don't mind, so I'm happy
> to
> go with your opinion on whether that is net win.

The dark chrome + minimal (removed) title bar are two most mentioned 
FF UI bugs on Linux so I think it's worth to fix that.
 
> On a related issue, I've been thinking about dropping native theming in
> content altogether.  This would solve some problems with sandboxing and
> reduce
> the burden of maintaining native widgets.  I'm not sure, but I guess there
> exists a modern set of styles for default widget rendering on Android.  If
> so,
> perhaps the same could be used for content on Linux.

I think it's really good idea to use one neutral light theme for web
content and style only the chrome UI. I think some custom theme may be 
another step and we may start with something available right now.
For instance light variant of default Gtk Adwaita theme looks 
like a good candidate to me.
Flags: needinfo?(karlt)
(In reply to Martin Stránský from comment #67)
> (In reply to Karl Tomlinson (:karlt) from comment #65)
> > Comment on attachment 8840799 [details] [diff] [review]
> > WIP light theme for content process
> > 
> > No need for GetThisProcessName() because XRE_IsContentProcess() is already
> > available for that purpose.
> > 
> > The effect of dropping the BrowserTabsRemoteAutostart() check is that the
> > parent process does not know whether content will be in a child process and
> > so
> > will use a dark theme whether it renders content or not.  (Using
> > GetThisProcessName() does not change that.)
> 
> I don't understand the mechanism here. Is that possible that web content 
> is rendered by both parent (chrome) and child processes when e10s is enabled?

Ah, I think I understand now. The problem is that nsLookAndFeel::Init() for chrome process is called to early so we don't know if the e10s is even supported and there will be any web content child process. So we don't have any clue whether to disable/enable dark theme on chrome.
(In reply to Martin Stránský from comment #68)
> (In reply to Martin Stránský from comment #67)
> > (In reply to Karl Tomlinson (:karlt) from comment #65)
> > > Comment on attachment 8840799 [details] [diff] [review]
> > > WIP light theme for content process
> > > 
> > > No need for GetThisProcessName() because XRE_IsContentProcess() is already
> > > available for that purpose.
> > > 
> > > The effect of dropping the BrowserTabsRemoteAutostart() check is that the
> > > parent process does not know whether content will be in a child process and
> > > so
> > > will use a dark theme whether it renders content or not.  (Using
> > > GetThisProcessName() does not change that.)
> > 
> > I don't understand the mechanism here. Is that possible that web content 
> > is rendered by both parent (chrome) and child processes when e10s is enabled?

No.  Content will either be rendered in the parent or child.

> Ah, I think I understand now. The problem is that nsLookAndFeel::Init() for
> chrome process is called to early so we don't know if the e10s is even
> supported and there will be any web content child process. So we don't have
> any clue whether to disable/enable dark theme on chrome.

Yes, if we don't know whether e10s is supported, then we don't know where content will be rendered.  I don't know what needs to happen before BrowserTabsRemoteAutostart() will return the right answer, and whether that can be triggered from BrowserTabsRemoteAutostart().  If not, then perhaps nsLookAndFeel::Init() can be split in such a way that colors are not initialized until later.

Still, if you think that dark chrome with e10s is more important than non-e10s colors, then XRE_IsContentProcess() is fine.
Flags: needinfo?(karlt)
(In reply to Martin Stránský from comment #67)
> > On a related issue, I've been thinking about dropping native theming in
> > content altogether.  This would solve some problems with sandboxing and
> > reduce
> > the burden of maintaining native widgets.  I'm not sure, but I guess there
> > exists a modern set of styles for default widget rendering on Android.  If
> > so,
> > perhaps the same could be used for content on Linux.
> 
> I think it's really good idea to use one neutral light theme for web
> content and style only the chrome UI. I think some custom theme may be 
> another step and we may start with something available right now.
> For instance light variant of default Gtk Adwaita theme looks 
> like a good candidate to me.

OK.  Thanks.  While we continue to use GTK in content, I'd like to continue to use the light variant of the configured GTK theme.  The benefits of a single theme become greater when we can drop GTK in content altogether and the theme can be consistent with another platform.  I'm still unsure, however, about whether dropping GTK/native scrollbars is a good idea.
(In reply to Karl Tomlinson (:karlt) from comment #71)
> (In reply to Martin Stránský from comment #67)
> > > On a related issue, I've been thinking about dropping native theming in
> > > content altogether.  This would solve some problems with sandboxing and
> > > reduce
> > > the burden of maintaining native widgets.  I'm not sure, but I guess there
> > > exists a modern set of styles for default widget rendering on Android.  If
> > > so,
> > > perhaps the same could be used for content on Linux.
> > 
> > I think it's really good idea to use one neutral light theme for web
> > content and style only the chrome UI. I think some custom theme may be 
> > another step and we may start with something available right now.
> > For instance light variant of default Gtk Adwaita theme looks 
> > like a good candidate to me.
> 
> OK.  Thanks.  While we continue to use GTK in content, I'd like to continue
> to use the light variant of the configured GTK theme.  The benefits of a
> single theme become greater when we can drop GTK in content altogether and
> the theme can be consistent with another platform.  I'm still unsure,
> however, about whether dropping GTK/native scrollbars is a good idea.

Although I agree that dropping all gtk native elements might not be great, using the light variant of a theme exists. Maybe a (maybe optional) fallback to Adwaita Light would be still be a good idea.
So here we come with the initialization.

First call of nsLookAndFeel::Init() comes from:

#1  0x00007fffe5c7aac2 in nsLookAndFeel::Init() (this=0x7fffd98029d0)
    at /home/komat/tmp676-trunk-gtk3/src/widget/gtk/nsLookAndFeel.cpp:1149
[...]
#5  0x00007fffe29fcefc in mozilla::LookAndFeel::GetInt(mozilla::LookAndFeel::IntID, int) (aID=mozilla::LookAndFeel::eIntID_UseAccessibilityTheme, aDefault=0) at /home/komat/tmp676-trunk-gtk3/src/objdir/dist/include/mozilla/LookAndFeel.h:541
#6  0x00007fffe29fd7b7 in nsChromeRegistryChrome::CheckForOSAccessibility() (this=0x7ffff6be02d0)
    at /home/komat/tmp676-trunk-gtk3/src/chrome/nsChromeRegistryChrome.cpp:163
#7  0x00007fffe795e363 in ScopedXPCOMStartup::SetWindowCreator(nsINativeAppSupport*) (this=0x7ffff6bc8658, native=0x7fffd98413d0) at /home/komat/tmp676-trunk-gtk3/src/toolkit/xre/nsAppRunner.cpp:1548
#8  0x00007fffe7966661 in XREMain::XRE_mainRun() (this=0x7fffffffcad0)
    at /home/komat/tmp676-trunk-gtk3/src/toolkit/xre/nsAppRunner.cpp:4162

But prefs are loaded later in XREMain::XRE_mainRun():

#0  0x00007fffe2a226f2 in pref_SetValue(PrefValue*, PrefTypeFlags, PrefValue, PrefType) 
[...]
#9  0x00007fffe2a273ef in mozilla::Preferences::ResetAndReadUserPrefs() ()
    at /home/komat/tmp676-trunk-gtk3/src/modules/libpref/Preferences.cpp:622
#10 0x00007fffe797873f in nsXREDirProvider::DoStartup() (this=0x7fffffffcaf8)
    at /home/komat/tmp676-trunk-gtk3/src/toolkit/xre/nsXREDirProvider.cpp:1133
#11 0x00007fffe7967178 in XREMain::XRE_mainRun() (this=0x7fffffffca80)
    at /home/komat/tmp676-trunk-gtk3/src/toolkit/xre/nsAppRunner.cpp:4307

so we're reading uninitialized prefs in nsLookAndFeel::Init().
Comment on attachment 8847084 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/120128/#review123178

I'm not a suitable reviewer for this patch.
https://wiki.mozilla.org/Modules/Toolkit#Toolkit links to Firefox peers who
could review this.

However, typically toolkit-specific behaviour is implemented in
toolkit-specific code rather than via #ifdef, where practical, and
LookAndFeel makes that possible, even though it would require changing more
code.

To implement this in toolkit-specific code, add an early return from
nsLookAndFeel::GetIntImpl() when aID == eIntID_UseAccessibilityTheme, add an
mInitialized variable, remove the calls to Init() from nsXPLookAndFeel() and
RefreshImpl(), rename Init() to EnsureInit(), and call EnsureInit() from the
public methods that depend on the theme (or otherwise on EnsureInit()
running).  I could review that change.
Attachment #8847084 - Flags: review?(karlt)
Comment on attachment 8847085 [details]
Bug 1158076 - disable dark themes in content process,

https://reviewboard.mozilla.org/r/120130/#review123168

::: commit-message-58319:1
(Diff revision 1)
> +Bug 1158076 - disable dark themes in content process, r?karlt

This is not really describing what the patch is changing because dark themes are already disabled in the content process.

"Honor GTK's global dark theme for chrome when e10s is enabled", from attachment 8753289 [details] [diff] [review], would describe the change.

::: widget/gtk/nsLookAndFeel.cpp:1138
(Diff revision 1)
>      // ask Gtk to create it explicitly. Otherwise we may end up
>      // with wrong color theme, see Bug 972382
>      GtkSettings *settings = gtk_settings_get_for_screen(gdk_screen_get_default());
>  
> +    if (!mozilla::BrowserTabsRemoteAutostart() || XRE_IsContentProcess()) {
> +        // We're running in content process and e10s is enabled so we can safely

This comment says that e10s is enabled, but !mozilla::BrowserTabsRemoteAutostart() is when e10s is not enabled.

Instead, changing the comment below to "Disable dark theme in processes with web content because it interacts poorly with widget styling", like attachment 8753289 [details] [diff] [review], is probably sufficient.

If you like, "When e10s is enabled, chrome will still use the system-wide theme." can be added after that.
Attachment #8847085 - Flags: review?(karlt) → review+
Comment on attachment 8847084 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/120128/#review123178

Yes, I was considering that. OTOH I expect the e10s will be default soon so I don't want to redesign the toolkit code, just create a simple hack which will be removed soon.

According to the https://wiki.mozilla.org/Electrolysis my understanding is that e10s will be default around Firefox 57 release (where's 'only WebExtensions allowed' state) and that's actually equal to recent trunk. It also means that recent nightly should run by default as e10s and question if we actually need the mozilla::BrowserTabsRemoteAutostart() call when it's supposed to be true unless e10s force-disable is set (AFAIK).

Would be bright to sync this fix with default e10s enablement and get rid of hacks.
Comment on attachment 8847084 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/120128/#review123178

I just realized one big advantage of the fix in toolkit specific code you propposed.

If we do that the nsLookAndFeel::Init() will be called later which means we actually *can* use all internal prefences freely. So we can remove the MOZ_ALLOW_GTK_DARK_THEME env variable hack and transfom it to a propper pref value. And the best thing is that we can also add another pref to enable default light theme for content as I was propposing at https://bugzilla.mozilla.org/show_bug.cgi?id=1158076#c67.

So I'll go with the toolkit-specific code if we utilise the prefs for further theme config.
Flags: needinfo?(karlt)
Comment on attachment 8847084 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/120128/#review123178

(In reply to Martin Stránský from comment #82)
> I just realized one big advantage of the fix in toolkit specific code you
> propposed.
> 
> If we do that the nsLookAndFeel::Init() will be called later which means we
> actually *can* use all internal prefences freely. So we can remove the
> MOZ_ALLOW_GTK_DARK_THEME env variable hack and transfom it to a propper pref
> value.

Yes, a pref is better than an environment variable.

> And the best thing is that we can also add another pref to enable
> default light theme for content as I was propposing at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1158076#c67.

I'm not clear why you want to do this.  Are you concerned that a theme may
be dark, but not have a light variant, and so you want to add an option to use
a different theme for content in that situation?

Is this a common situation?

I don't see much value in having a single GTK theme for content.  The value in
supporting GTK themes is that users can choose how they like things to look.
If there is not going to be a choice, then a built-in (non-GTK) theme can be
used, and there is no need to deal with all the different versions of the GTK
theming ABI.
Flags: needinfo?(karlt)
I'd say it's a very common situation that a theme wont have a light variant, the majority of themes don't even have different variants. Adding to that there's never really a fixed standard theme between any platforms so how would you know the best default? if there even is one available (ie; only a single, non-light, theme installed).
(In reply to Paul17041993 from comment #84)
> I'd say it's a very common situation that a theme wont have a light variant,
> the majority of themes don't even have different variants. Adding to that
> there's never really a fixed standard theme between any platforms so how
> would you know the best default? if there even is one available (ie; only a
> single, non-light, theme installed).

Adwaita light should be always available because it's a part of Gtk3 package and not separated theme.
Comment on attachment 8847084 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/120128/#review123178

Yes, that's what I mean. See https://bugzilla.redhat.com/show_bug.cgi?id=1421143 - Cinnamon has a default theme (sorry I don't recall the name) which is dark/grey by default and does not have a light variant (actually it's considered as "light"). People must install extensions like this one - https://addons.mozilla.org/en-GB/firefox/addon/text-contrast-for-dark-themes/

I think we need to provide a single pref to fallback to default gtk3 theme for content if necessary. The default Adwaita theme is an integrall part of Gtk3 package and is always available and provides decent neutral look AFAIK.

> I don't see much value in having a single GTK theme for content.  The value in
> supporting GTK themes is that users can choose how they like things to look.

I think there's a big difference between browser UI and web content. From my experience users want to have browser UI integrated to system (to use system theme for browser UI, titlebars, controls and so) to have look unified with rest of the system and use system colors here.

But the same users don't want the system theme on actual webpages. Web designers do not design web pages for such various themes and themed forms/buttons/entries looks veird on web pages. For instance bank forms are good example.

I think it's the same like PDF/document viever. You want to see unmodified original document in the application but PDF reader UI integrated to the system.

> If there is not going to be a choice, then a built-in (non-GTK) theme can be
> used, and there is no need to deal with all the different versions of the GTK
> theming ABI.

I think we should provide a default modern theme for the web content only and leave browser UI themed by system. But before we have such web theme we should enable to use another system theme for the web content as a fallback when original system theme is not suitable here.
Attachment #8847085 - Attachment is obsolete: true
(In reply to Martin Stránský from comment #85)
> Adwaita light should be always available because it's a part of Gtk3 package
> and not separated theme.

Unless it's super hidden, I dont have it...
(In reply to Paul17041993 from comment #92)
> (In reply to Martin Stránský from comment #85)
> > Adwaita light should be always available because it's a part of Gtk3 package
> > and not separated theme.
> 
> Unless it's super hidden, I dont have it...

It is compiled into the Gtk3 library, your libgtk-3.so.0 on Linux.
(In reply to Paul17041993 from comment #94)
> (In reply to Christian Stadelmann from comment #93)
> > It is compiled into the Gtk3 library, your libgtk-3.so.0 on Linux.
> 
> Interesting, because apparently firefox is the only one that sees said
> theme, everything else doesn't...

I don't know where you're looking, but you're looking wrong or your tools are broken. If you are still sure you don't see Adwaita, please tell us what you did (e.g. what application or command you used to list themes). Unless you are using a willingly broken Gtk+ 3 installation, you have Adwaita in this shared library. And every application is able to use it.

Open any Gtk+ 3.x application and you'll be running Adwaita if no Gtk+ 3 theme is installed. Or (if you are using a recent version of Gtk+) open the GtkInspector and select the theme. See https://askubuntu.com/questions/597259/how-do-i-open-gtk-inspector for how to do that.
(In reply to Paul17041993 from comment #96)
> Ok apparently the other GTK apps I have use GTK2, however they look for
> Adwaita by default anyway, which is odd because it doesn't officially exist
> for GTK2...

Yes, it does exist. If you search your package manager it will probably list a package named adwaita-gtk2 (or something similiar), which is built from code from https://git.gnome.org/browse/gnome-themes-standard/tree/themes/Adwaita/gtk-2.0 . You probably should start fact checking before claiming stuff that is pretty wrong.

Anyway, we're pretty much off topic and should stop spamming this bug report.
Comment on attachment 8847084 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/120128/#review124220

::: widget/gtk/nsLookAndFeel.cpp:1138
(Diff revision 2)
> -    // Disable dark theme because it interacts poorly with widget styling in
> +        // Disable dark theme because it interacts poorly with widget styling in
> -    // web content (see bug 1216658).
> +        // web content (see bug 1216658).

Please change this comment to "Disable dark theme in processes with web content because it interacts poorly with widget styling."
Attachment #8847084 - Flags: review?(karlt) → review+
Comment on attachment 8849083 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/121920/#review124218

Looks good thanks, except that we don't need/want the eIntID_UseAccessibilityTheme that I suggested, sorry.

::: widget/gtk/nsLookAndFeel.cpp:671
(Diff revision 1)
> +    case eIntID_UseAccessibilityTheme:
> +        aResult = 0;
> +        return NS_OK;

I think we need to let the call below to nsXPLookAndFeel::GetIntImpl() complete so
that the ui.useAccessibilityTheme pref works when called from
nsPresContext::GetDocumentColorPreferences().
(I'm assuming that nsXPLookAndFeel::OnPrefChanged() will notice when the prefs
are read.)

The way you've done this, calling EnsureInit() from GetIntImpl() only in the
two cases that require it, is good, and means that this
eIntID_UseAccessibilityTheme special case is not required at all.

If you'd like to keep an explicit case, then moving it to the second switch
statement would be fine.

Whether or not you keep a eIntID_UseAccessibilityTheme case, add a comment
please to GetIntImpl() explaining that EnsureInit is not called in that case
so that it is delayed until user prefs are read.
Attachment #8849083 - Flags: review?(karlt) → review+
Comment on attachment 8849102 [details]
Bug 1158076 - add widget.allow-gtk-dark-theme pref to enable gtk dark theme,

https://reviewboard.mozilla.org/r/121930/#review124222

::: widget/gtk/nsLookAndFeel.cpp:1157
(Diff revision 1)
>          // setting when necessary.
>          const gchar* dark_setting = "gtk-application-prefer-dark-theme";
>          gboolean dark;
>          g_object_get(settings, dark_setting, &dark, nullptr);
>  
> -        if (dark && !PR_GetEnv("MOZ_ALLOW_GTK_DARK_THEME")) {
> +        bool allowDarkEnv = static_cast<bool>(PR_GetEnv("MOZ_ALLOW_GTK_DARK_THEME"));

Perhaps PR_GetEnv("MOZ_ALLOW_GTK_DARK_THEME") != nullptr, instead of static_cast<bool>() to provide some indication of the return type of PR_GetEnv.
Attachment #8849102 - Flags: review?(karlt) → review+
Comment on attachment 8849286 [details]
Bug 1158076 - Allow Gtk+ theme override for web content when e10s is enabled,

https://reviewboard.mozilla.org/r/122100/#review124236

I like the option to specify the theme for content.  Thanks!

I wonder whether it could be useful even without e10s, if people want to specify a different theme for Firefox for the sake of content, but I don't mind.

::: widget/gtk/nsLookAndFeel.cpp:1170
(Diff revision 1)
> +
> +        // Allow Gtk+ theme override for web content only.
> +        if (e10sActive) {
> +            auto contentThemeName =
> +                mozilla::Preferences::GetCString("widget.content-gtk-theme");
> +            if (contentThemeName) {

I guess the conversion to bool here goes through conversion to |const char_type*|, which would be nullptr -> false for an uninitialized pref, but true for a string of zero length.

I'd like a string of zero length to be interpreted as meaning "don't change the theme".

Even if I'm reading nsTAdoptingString_CharT incorrectly, please use !contentThemeName.IsEmpty() to be clear.

::: widget/gtk/nsLookAndFeel.cpp:1171
(Diff revision 1)
> +                GtkCssProvider *styleProvider =
> +                    gtk_css_provider_get_named(contentThemeName, NULL);
> +                gtk_style_context_add_provider_for_screen(gdk_screen_get_default(),
> +                    GTK_STYLE_PROVIDER(styleProvider),
> +                    GTK_STYLE_PROVIDER_PRIORITY_APPLICATION);

I not clear how adding a provider will interact with the theme from the “gtk-theme-name” property.

Can the “gtk-theme-name” property on |settings| be set instead of adding a provider, please?

That should be a little simpler anyway.
Attachment #8849286 - Flags: review?(karlt)
Attachment #8849083 - Attachment is obsolete: true
Attachment #8849102 - Attachment is obsolete: true
Attachment #8849286 - Attachment is obsolete: true
Looks like the mozreview merged my two first review together so I need to resubmit them.
Attachment #8847084 - Attachment is obsolete: true
Attachment #8849684 - Attachment is obsolete: true
Attachment #8849684 - Flags: review?(karlt)
Attachment #8849685 - Attachment is obsolete: true
Attachment #8849685 - Flags: review?(karlt)
Attachment #8849686 - Attachment is obsolete: true
Attachment #8849686 - Flags: review?(karlt)
Comment on attachment 8849716 [details]
Bug 1158076 - Allow Gtk+ theme override for web content when e10s is enabled,

https://reviewboard.mozilla.org/r/122492/#review124656

::: widget/gtk/nsLookAndFeel.cpp:1173
(Diff revision 1)
> +                mozilla::Preferences::GetCString("widget.content-gtk-theme");
> +            if (!contentThemeName.IsEmpty()) {
> +                // TODO: It should be enough to change theme by "gtk-theme-name"
> +                // settings but that does not have any effect here. Maybe we
> +                // call it too late?
> +                GtkCssProvider *styleProvider =

The “gtk-theme-name” property settings here has no effect (in contrast gtk-application-prefer-dark-theme works).

I don't know if it's too late to set gtk-theme-name here or it's a bug in Gtk (Gtk folks says it should work) but I'd leave that for further investigation if that's needed.

I also suspect that the "gtk-theme-name" change may not be persistent. When you change desktop theme run-time (by gnome-tweaking-tool for instance) it changes theme instantly for all applications and it may involve the gtk-theme-name update and change theme for both content and UI FF processes.

But the style provider we use has bigger style priority than the desktop theme so it's not affected by this system-wide theme change which is what we actually want.
Attachment #8849716 - Attachment is obsolete: true
Attachment #8849716 - Flags: review?(karlt)
Let's leave the custom theme to another bug and solve this one first.

I've got an user feedback about this dark theme patch and it looks like Firefox need more fixes to mix the dark/light themes smoothly (https://bugzilla.redhat.com/show_bug.cgi?id=1435964).

I suggest to always set light theme for content/web process (when e10s is enabled) and use the preferences to enable/disable dark theme for main process. It means to keep recent state and disable dark themes by default for UI even with e10s.
I definitely suggest that the light/internal theme be the default for everything, on my system I have to force FF to use adwaita for it to not look completely broken. I then just use a standard dark-like FF theme for it to match the system style, the main scrollbar however is still light but this actually matches well with the web content.
Depends on: 1234279
Depends on: 1286000
Depends on: 1351545
Depends on: 1351546
No longer depends on: 1351545
No longer depends on: 1351546
Comment on attachment 8849713 [details]
Bug 1158076 - add prefs to enable GTK dark themes in each process,

https://reviewboard.mozilla.org/r/122486/#review127484
Attachment #8849713 - Flags: review?(karlt) → review+
(In reply to Martin Stránský from comment #108)
> Looks like the mozreview merged my two first review together so I need to
> resubmit them.

Looks like mozreview got it all wrong there.
Filed bug 1351546 on that.

My best suggestion is to add this to ~/.hgrc to make it easier to preview
before publishing pushed changes:

[reviewboard]
autopublish = False

Pushes that are not published will not be visible to anyone else,
and can be discarded easily if they produce the wrong results.
Comment on attachment 8849714 [details]
Bug 1158076 - postpone nsLookAndFeel module initialization,

https://reviewboard.mozilla.org/r/122488/#review127490

::: widget/gtk/nsLookAndFeel.cpp:680
(Diff revision 1)
> +    // We use delayed initialization by EnsureInit() here
> +    // to ensure mozilla::Preferences is available (see Bug 1158076).

Something like "eIntID_UseAccessibilityTheme is requested before user preferences are read, and so EnsureInit(), which depends on preference values, is deliberately delayed until required." so that the reader doesn't need to read through this bug.
Attachment #8849714 - Flags: review?(karlt) → review+
Comment on attachment 8849715 [details]
Bug 1158076 - add widget.allow-gtk-dark-theme pref to enable gtk dark theme,

https://reviewboard.mozilla.org/r/122490/#review127498

::: commit-message-707fa:1
(Diff revision 2)
> +Bug 1158076 - add widget.allow-gtk-dark-theme pref to enable gtk dark theme, r?karlt

This patch is changing more than just adding a pref to enable the dark theme.  The changes need to be described in the commit message.

The patch is undoing much of https://reviewboard.mozilla.org/r/122486/diff/1#index_header and so I don't see any value in keeping these as separate changesets.  I think the changes proposed here would be easier to understand as a single changeset.

::: widget/gtk/nsLookAndFeel.cpp:1152
(Diff revision 2)
>  
> -    if (!mozilla::BrowserTabsRemoteAutostart() || XRE_IsContentProcess()) {
> +    const gchar* dark_setting = "gtk-application-prefer-dark-theme";
> +    gboolean darkThemeDefault;
> +    g_object_get(settings, dark_setting, &darkThemeDefault, nullptr);
> +
> +    if (e10sActive && XRE_IsContentProcess()) {

No need for the e10sActive check here because XRE_IsContentProcess() will return true only when e10s is active.

::: widget/gtk/nsLookAndFeel.cpp:1153
(Diff revision 2)
>          // Disable dark theme in processes with web content because it
>          // interacts poorly with widget styling (see bug 1216658).

The comment is confusing here because this block is only for e10s content processes, not for all processes with content.

It would make more sense before the XRE_IsContentProcess() test.

::: widget/gtk/nsLookAndFeel.cpp:1163
(Diff revision 2)
> +        bool allowDarkEnv = PR_GetEnv("MOZ_ALLOW_GTK_DARK_THEME") != nullptr;
> +        bool allowDarkPref =
> +            mozilla::Preferences::GetBool("widget.allow-gtk-dark-theme", false);

I'm not sure whether user configuration should be restricted to the browser process, but I'm happy for you to try this change if it is done in a way that can be extended to having a separate preference for content.

Please give the pref for the browser/mail process (handled in this block) a process-specific name such as "widget.allow-gtk-dark-theme.chrome".
Attachment #8849715 - Flags: review?(karlt)
(In reply to Karl Tomlinson (:karlt) from comment #118)
> (In reply to Martin Stránský from comment #108)
> > Looks like the mozreview merged my two first review together so I need to
> > resubmit them.
> 
> Looks like mozreview got it all wrong there.
> Filed bug 1351546 on that.
> 
> My best suggestion is to add this to ~/.hgrc to make it easier to preview
> before publishing pushed changes:
> 
> [reviewboard]
> autopublish = False
> 
> Pushes that are not published will not be visible to anyone else,
> and can be discarded easily if they produce the wrong results.

I already have that and review changes before pushing. 

The problem here was that mercurial command line showed me correct patchset but mozreview wrong one so I was confused which one is the final one. So I learned that the mozreview one is always the final one.
Attachment #8849715 - Attachment is obsolete: true
(In reply to Karl Tomlinson (:karlt) from comment #120)
> This patch is changing more than just adding a pref to enable the dark
> theme.  The changes need to be described in the commit message.
> 
> The patch is undoing much of
> https://reviewboard.mozilla.org/r/122486/diff/1#index_header and so I don't
> see any value in keeping these as separate changesets.  I think the changes
> proposed here would be easier to understand as a single changeset.

Merged although mozreview seems to be a bit confused :)

> No need for the e10sActive check here because XRE_IsContentProcess() will
> return true only when e10s is active.

Thanks, that's good point. I actually tired a bit different approach (described in the commit message) so we don't need the mozilla::BrowserTabsRemoteAutostart() hack any more.
 
> ::: widget/gtk/nsLookAndFeel.cpp:1163
> (Diff revision 2)
> > +        bool allowDarkEnv = PR_GetEnv("MOZ_ALLOW_GTK_DARK_THEME") != nullptr;
> > +        bool allowDarkPref =
> > +            mozilla::Preferences::GetBool("widget.allow-gtk-dark-theme", false);
> 
> I'm not sure whether user configuration should be restricted to the browser
> process, but I'm happy for you to try this change if it is done in a way
> that can be extended to having a separate preference for content.
> 
> Please give the pref for the browser/mail process (handled in this block) a
> process-specific name such as "widget.allow-gtk-dark-theme.chrome".

I chose widget.chrome.allow-gtk-dark-theme and widget.content.allow-gtk-dark-theme.
(In reply to Martin Stránský from comment #124)
> Merged although mozreview seems to be a bit confused :)

Thanks.  MozReview is tracking MozReview-Commit-IDs as expected I think.  It
doesn't know, however, that the patch has changed enough to need review again.
Attachment #8849713 - Flags: review+ → review?(karlt)
Comment on attachment 8849713 [details]
Bug 1158076 - add prefs to enable GTK dark themes in each process,

https://reviewboard.mozilla.org/r/122486/#review127792

::: commit-message-9fb5e:1
(Diff revision 2)
> +Bug 1158076 - Disable Gtk dark theme by default and allow to enable it behind pref, r?karlt

Now that the changes are merged, the dark theme is enabled by default both before and after these changes, but "Disable Gtk dark theme by default" implies a change in behavior.

Something like "add prefs to enable GTK dark themes in each process" would now be sufficient for the first line.

::: commit-message-9fb5e:9
(Diff revision 2)
> +themes in chrome and content when e10s is enabled.
> +
> +When e10s is disabled then widget.chrome.allow-gtk-dark-theme controls both chrome and web content settings.
> +That may be a bit confusing but it's going to be here for two releases only (Firefox 57 is going to have e10s enabled by default) and actually matches recent state when only one ENV pref is used for both chrome and web content.
> +
> +We also keep the recent MOZ_ALLOW_GTK_DARK_THEME env variable which match widget.chrome.allow-gtk-dark-theme.

Please state the change in behavior explicitly here.
Something like "The existing MOZ_ALLOW_GTK_DARK_THEME environment variable is still considered, but, now is like widget.chrome.allow-gtk-dark-theme, no longer affecting separate content processes."

::: widget/gtk/nsLookAndFeel.cpp
(Diff revision 2)
> -    // To avoid triggering reload of theme settings unnecessarily, only set the
> -    // setting when necessary.

Please keep this comment, after the new comments below.
Attachment #8849713 - Flags: review?(karlt) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/526b246b0cda
add prefs to enable GTK dark themes in each process, r=karlt
https://hg.mozilla.org/integration/autoland/rev/7dc38604b312
postpone nsLookAndFeel module initialization, r=karlt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/526b246b0cda
https://hg.mozilla.org/mozilla-central/rev/7dc38604b312
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
if gtk theme is adwaita dark firefox uses adwata (regular) which is the default gtk3 theme
dark system gtk theme goes with bright icons (breeze-dark) which firefox happens to use
end result: bright icons on white background
could firefox also use the system icons which go with adwaita (in my case breeze)
>not a big deal but reporting
(In reply to x3 from comment #131)
> if gtk theme is adwaita dark firefox uses adwata (regular) which is the
> default gtk3 theme
> dark system gtk theme goes with bright icons (breeze-dark) which firefox
> happens to use
> end result: bright icons on white background
> could firefox also use the system icons which go with adwaita (in my case
> breeze)
> >not a big deal but reporting

This affects e.g. the native print dialog and the native file chooser dialog. Does it affect any other part of Firefox? If no, I'd say this is a bug in Gtk+, not in Firefox.
(In reply to Christian Stadelmann from comment #132)
> Does it affect any other part of Firefox? If no, I'd say this is a
> bug in Gtk+, not in Firefox.
Yes it does. All parts where Firefox uses icons from the selected icon theme which appear to be the folder icons in Bookmarks and in the Save/Open window. I don't see how Firefox prefering Adwaita over user selected Adwaita-dark but not prefering breeze icons over user selected breeze-dark is a gtk bug. I'm not saying firefox should not prefer adwaita. I'm just reporting firefox should also prefer the appropriate icon theme variant. Idk if that can be done and it's not that important since by default gkt3 does not show icons on buttons or in menus so it's not that apparent but it should be noted.
Does the patch poll for the light theme icons too? otherwise it's incomplete...
Flags: needinfo?(stransky)
Hello, I am using firefox-trunk from https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/ppa, I do not see any changes in how, e.g., packages.ubuntu.com is displayed, it is still displayed incorrectly
, I'm using GNOME shell 3.24 , Ubuntu 17.04, Adwaita Dark GTK3 theme
(In reply to Paul17041993 from comment #134)
> Does the patch poll for the light theme icons too? otherwise it's
> incomplete...

This patch only toggles "gtk-application-prefer-dark-theme" gnome settings and nothing else. I don't know if "gtk-application-prefer-dark-theme" is supposed to also change icon theme. Anyway, feel free to file follow up bug to address the icon issue.
Flags: needinfo?(stransky)
(In reply to Mikhail Novosyolov from comment #135)
> Hello, I am using firefox-trunk from
> https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/ppa, I do not
> see any changes in how, e.g., packages.ubuntu.com is displayed, it is still
> displayed incorrectly
> , I'm using GNOME shell 3.24 , Ubuntu 17.04, Adwaita Dark GTK3 theme

This patch hits Firefox 55 and you need to adjust "widget.content.allow-gtk-dark-theme" in about:config.
From what I know of GTK and themes, no, the icons are independent of the selected theme and you've potentially made the bug worse by not forcing the icon theme...
(In reply to Martin Stránský from comment #137)
> This patch hits Firefox 55 and you need to adjust
> "widget.content.allow-gtk-dark-theme" in about:config.

Does not change anything at http://packages.ubuntu.com, still displayed incorrectly, Firefox 55 Nightly. changed this option to true/false and did restart Firefox, nothing happens.
(In reply to Mikhail Novosyolov from comment #139)
> (In reply to Martin Stránský from comment #137)
> > This patch hits Firefox 55 and you need to adjust
> > "widget.content.allow-gtk-dark-theme" in about:config.
> 
> Does not change anything at http://packages.ubuntu.com, still displayed
> incorrectly, Firefox 55 Nightly. changed this option to true/false and did
> restart Firefox, nothing happens.

Please see this thread:
https://unix.stackexchange.com/questions/14129/gtk-enable-set-dark-theme-on-a-per-application-basis

Are you able to set Adwaita:light for any other application?
(In reply to Martin Stránský from comment #140)
 restart Firefox, nothing happens.
> 
> Please see this thread:
> https://unix.stackexchange.com/questions/14129/gtk-enable-set-dark-theme-on-
> a-per-application-basis
> 
> Are you able to set Adwaita:light for any other application?

Yes, I am able to run Firefox as
$ GTK_THEME=Adwaita:light firefox
but then it's interface and context manus are light, I want them to be dark
I just retest with nightly and works fine for me although there are some catches:

- you need to set dark theme globally (for instance by gnome-tweak-tool) for all applications. Firefox can't detect local dark theme preference (like $GTK_THEME=Adwaita:dark firefox) or theme which are dark by default and does not have dark/light variant (for instance Arc themes from Cinnamon). May be we can do some detection from text/background color?

- you need to run Firefox with e10s enabled - only web content process is running with different (light) themes.
(In reply to Martin Stránský from comment #142)
>
> - you need to set dark theme globally (for instance by gnome-tweak-tool) for
> all applications. Firefox can't detect local dark theme preference (like
> $GTK_THEME=Adwaita:dark firefox) or theme which are dark by default and does
> not have dark/light variant (for instance Arc themes from Cinnamon). May be
> we can do some detection from text/background color?

So what you're basically saying is that for users of dark themes (like breeze-dark) without builtin light and dark theme variants in the same theme - this 'fix' wont' work at all. (as I can't seem to get it to)

I have my system theme that's dark and that I want to use, and I figured I could get around that limitation by specifying the GTK_THEME in the application definition, but you're also saying that overrides the work done here.

I've tried the new settings:

widget.allow-gtk-dark-theme 
widget.chrome.allow-gtk-dark-theme 
widget.content.allow-gtk-dark-theme

In various toggled states with GTK_THEME defined as "Adwaita" "Adwaita:light" "Adwaita:dark" and I see no different at all on Firefox 56 recently shipped in Fedora 27. 

Am I missing something here?
To clarify, I DO see a difference in the GTK theme applied, but no difference between chrome and content, so when I define "Adwaita:light" versus dark, I do see that difference applied. 

I find this page is helpful to test: https://www.lehigh.edu/~inwww/form-test.html
(In reply to mike from comment #143)
> In various toggled states with GTK_THEME defined as "Adwaita"
> "Adwaita:light" "Adwaita:dark" and I see no different at all on Firefox 56
> recently shipped in Fedora 27. 
> 
> Am I missing something here?

See Bug 1353147

You can add a new variable "widget.content.gtk-theme-override" and define Gtk+ theme for content only. I'd use "Adwaita:light" theme for that. The override works only when e10s is enabled (because it uses per-process theme setting) and also the fix is only partial - some Web elements are rendered by main thread (like combo box roll-ups and so) so those still have Gtk theme for chrome (Firefox UI).

Anyway, I'd recommend nightly builds for testing (you can also use flatpak ones from https://firefox-flatpak.mojefedora.cz/) because they have all features set (e10s always enabled for instance).
Why is the GTK theme even allowed to modify content? Also why is this marked as fixed if it's still an issue on 58?
(In reply to Paul [pwd] from comment #146)
> Why is the GTK theme even allowed to modify content? Also why is this marked
> as fixed if it's still an issue on 58?

Because on some platforms, Firefox renders some elements (such as buttons or checkboxes in websites) using Gtk+. I can't tell you why that is though.
HTML defines native theming you can use, which in turn uses the browser's native theme rendering. However due to this often only being partially implemented in websites, the website is then rendered with different, conflicting themes. Thus we have this problem and why I recommend full-native theming be removed entirely...
(In reply to Paul17041993 from comment #148)
> […]
> Thus we have this problem and why I recommend
> full-native theming be removed entirely...

This idea looks good to me too for several reasons:
* Remove chances of fingerprinting by toolkit/platform. (Should block on Bug 1329996 (AntiFingerprinting))
* Remove dependency on the toolkit of the content process – reduce complexity.
* Make browser behaviour more consistent on different platforms.

Is there a bug report for that (I could not find one)? If not, would you mind creating one?
There is now; #1411425
now why isn't bug linking working properly...
(In reply to Christian Stadelmann from comment #149)
> (In reply to Paul17041993 from comment #148)
> > […]
> > Thus we have this problem and why I recommend
> > full-native theming be removed entirely...
> 
> This idea looks good to me too for several reasons:
> * Remove chances of fingerprinting by toolkit/platform. (Should block on Bug
> 1329996 (AntiFingerprinting))
> * Remove dependency on the toolkit of the content process – reduce
> complexity.
> * Make browser behaviour more consistent on different platforms.
> 
> Is there a bug report for that (I could not find one)? If not, would you
> mind creating one?

As mentioned in bug 1411425, I highly doubt websites have access to the platform toolkit informations. Only User Agent give away user's OS.
Not Resolved. Still having this problem with 57.0b11 on GNOME 3.22.2 Debian 9.2
(In reply to mozilla from comment #154)
> Not Resolved. Still having this problem with 57.0b11 on GNOME 3.22.2 Debian
> 9.2

Is "widget.content.allow-gtk-dark-theme" unset or "false" and are you sure you are using content processes (see about:support)?
(In reply to Christian Stadelmann from comment #155) 
> Is "widget.content.allow-gtk-dark-theme" unset or "false" and are you sure
> you are using content processes (see about:support)?

I have no idea what any of that is, I am guessing most people don't either.

Bug: Firefox does weird stuff when changing GTK themes.

Steps to reproduce: Enable different color theme in GNOME and Firefox shows some web content in odd colors.

Expectation: The web content should look normal after changing GNOME themes without any special config tweaking in Firefox.
(In reply to mozilla from comment #156)
> (In reply to Christian Stadelmann from comment #155) 
> > Is "widget.content.allow-gtk-dark-theme" unset or "false" and are you sure
> > you are using content processes (see about:support)?
> 
> I have no idea what any of that is, I am guessing most people don't either.

That's an about:config switch (see the patches above). In case you have not done so already, you should read through the comments above yours.

In case you don't use content processes, you'll also have to check the value for the chrome and maybe even MOZ_ALLOW_GTK_DARK_THEME.
The "...allow-gtk-dark-theme" changes didn't work for me.
I'm using an "Arc Dark" theme, where the light variant has a different name.

Adding a new string: "widget.content.gtk-theme-override": "Adwaita:light" in about:config did the trick.

A proper fix is probably bug 1411425, to not use native UI toolkits in the content process.
After all most web designers expects textarea to have a white background by default.
Thank you, :jonasfj!

I created a new entry in about:config with widget.content.gtk-theme-override and Arc as a value (I generally use the Arc:Dark theme) and it works like a charm!
New entry 
"widget.content.gtk-theme-override": "Clearlooks"
works!
(16.04 and 18.04 ubuntu/wattos FF 63.0)
See Also: → 1466131
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: