1365556 - Firefox 53 ButtonText incorrect with some themes and GTK 3.10

Status: VERIFIED FIXED

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

Description

[i.dani]

Attachment: brokenff53.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170419190606

Steps to reproduce:

Update from Firefox 52 to 53


Actual results:

Firefox overrides the default window manager font color in some of its pop up dialog and buttons (included buttons rendered in webpages)
This leads to unreadable buttons text.

See the top of the attached Screenshot1.png for an example: The "Quit" "Save" "Save and Quit" text is white (should be black). Hovering over the "Do no ask next time" also turns that text white.

Problem appeared after update of firefox:amd64 (from 52.0.2+build1-0ubuntu0.14.04.1 to 53.0+build6-0ubuntu0.14.04.1).

Using the default Firefox theme or the other 2 new "compact" provided themes produces the same result.

Confirmed on:
Ubuntu GNOME 14.04.5
Linux Mint 17.1 Rebecca - Cinnamon 2.4.8
On Xubuntu 14.04 with XFCE 4.10:
In the snapshot above the XFCE theme is the standard "Adwaita" but the problem can be reproduced with other themes thought it is not present for example with the similar XFCE theme "Orion".


Expected results:

Use the correct color, as it was in Firefox 52

Comment 1

[i.dani]

Also see https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1685439

Comment 2

[Loic]

Is it reproducible with the official Firefox build from Mozilla?

Comment 3

[i.dani]

If have just tested with the latest official nightly build (v55)[1] and the problem is the same.

[1] https://www.mozilla.org/de/firefox/channel/desktop/

Comment 4

[i.dani]

Just searched some more.. Also see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1360821
https://bugzilla.mozilla.org/show_bug.cgi?id=1358091
https://bugzilla.mozilla.org/show_bug.cgi?id=1354526

Comment 5

[i.dani]

I've just tried some more stuff:
- Safe mode -> Same Problem
- Refresh FF (https://support.mozilla.org/en-US/kb/refresh-firefox-reset-add-ons-and-settings) -> Same Problem
- Purge FF, clean up, Fresh install -> Same Problem

Here is what "about:support" says:


Application Basics
------------------

Name: Firefox
Version: 53.0.2
Build ID: 20170509210304
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0)
Gecko/20100101 Firefox/53.0
OS: Linux 3.19.8-ckt22-openslx
Multiprocess Windows: 0/1 (Disabled by add-ons)
Safe Mode: false

Crash Reports for the Last 3 Days
---------------------------------

All Crash Reports

Extensions
----------

Name: Application Update Service Helper
Version: 2.0
Enabled: true
ID: [e-mail]

Name: Multi-process staged rollout
Version: 1.14
Enabled: true
ID: [e-mail]

Name: Pocket
Version: 1.0.5
Enabled: true
ID: [e-mail]

Name: Ubuntu Modifications
Version: 3.2
Enabled: true
ID: [e-mail]

Name: Web Compat
Version: 1.0
Enabled: true
ID: [e-mail]

Graphics
--------

Features
Compositing: Basic
Asynchronous Pan/Zoom: none
WebGL Renderer: Intel Open Source Technology Center -- Mesa DRI Intel(R)
Haswell Desktop
WebGL2 Renderer: WebGL creation failed: * WebGL 2 requires support for
the following features: transform_feedback2 * Exhausted GL driver options.
Audio Backend: pulse
GPU #1
Active: Yes
Description: Intel Open Source Technology Center -- Mesa DRI Intel(R)
Haswell Desktop
Vendor ID: Intel Open Source Technology Center
Device ID: Mesa DRI Intel(R) Haswell Desktop
Driver Version: 3.0 Mesa 10.1.3

Diagnostics
AzureCanvasAccelerated: 0
AzureCanvasBackend: skia
AzureContentBackend: skia
AzureFallbackCanvasBackend: none
CairoUseXRender: 0
Decision Log
HW_COMPOSITING:
blocked by default: Acceleration blocked by platform
OPENGL_COMPOSITING:
unavailable by default: Hardware compositing is disabled




Important Modified Preferences
------------------------------

browser.cache.disk.capacity: 1048576
browser.cache.disk.filesystem_reported: 1
browser.cache.disk.smart_size.first_run: false
browser.cache.frecency_experiment: 1
browser.download.importedFromSqlite: true
browser.places.smartBookmarksVersion: 8
browser.sessionstore.upgradeBackup.latestBuildID: 20170509210304
browser.startup.homepage_override.buildID: 20170509210304
browser.startup.homepage_override.mstone: 53.0.2
browser.tabs.remote.autostart.2: true
browser.urlbar.daysBeforeHidingSuggestionsPrompt: 3
browser.urlbar.lastSuggestionsPromptDate: 20170522
extensions.lastAppVersion: 53.0.2
media.gmp.storage.version.observed: 1
network.cookie.prefsMigrated: true
places.history.expiration.transient_current_max_pages: 60313
plugin.disable_full_page_plugin_for_types: application/pdf

Important Locked Preferences
----------------------------

Places Database
---------------

JavaScript
----------

Incremental GC: true

Accessibility
-------------

Activated: false
Prevent Accessibility: 0

Library Versions
----------------

NSPR
Expected minimum version: 4.13.1
Version in use: 4.13.1

NSS
Expected minimum version: 3.29.5
Version in use: 3.29.5

NSSSMIME
Expected minimum version: 3.29.5
Version in use: 3.29.5

NSSSSL
Expected minimum version: 3.29.5
Version in use: 3.29.5

NSSUTIL
Expected minimum version: 3.29.5
Version in use: 3.29.5

Experimental Features
---------------------

Sandbox
-------

Seccomp-BPF (System Call Filtering): true
Seccomp Thread Synchronization: true
User Namespaces: true
Media Plugin Sandboxing: true

Comment 6

[velenir]

Have you tried the workaround I posted at https://bugzilla.mozilla.org/show_bug.cgi?id=1358091#c3 ?

Also, for UI buttons you can create userChrome.css in that same chrome folder with the following:

.button-text,
.button-text:hover,
.button-text:active:hover,
.button-text:-moz-focusring::-moz-focus-inner,
toolbarbutton,
toolbarbutton:hover,
toolbarbutton:active:hover {
  color: black !important;
}


I don't think it'll get all the instances of the problem but surely most of them.
Still, it being a workaround, I'd much prefer an actual bugfix.

Comment 7

[i.dani]

(In reply to velenir from comment #6)
> Have you tried the workaround I posted at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1358091#c3 ?
> 
> Also, for UI buttons you can create userChrome.css in that same chrome
> folder with the following:
> 
> .button-text,
> .button-text:hover,
> .button-text:active:hover,
> .button-text:-moz-focusring::-moz-focus-inner,
> toolbarbutton,
> toolbarbutton:hover,
> toolbarbutton:active:hover {
>   color: black !important;
> }
> 
> 
> I don't think it'll get all the instances of the problem but surely most of
> them.
> Still, it being a workaround, I'd much prefer an actual bugfix.

I've tried this - sadly there are still quite a lot of missing parts.
So we really do not want to use this in our production environment.. 

Is there a fix comming anytime soon? It's quite annoying not beeing able to update because of this bug.

Comment 8

[Jim Mathies [:jimm]]

(In reply to i.dani from comment #0)
> Confirmed on:
> Ubuntu GNOME 14.04.5
> Linux Mint 17.1 Rebecca - Cinnamon 2.4.8
> On Xubuntu 14.04 with XFCE 4.10:
> In the snapshot above the XFCE theme is the standard "Adwaita" but the
> problem can be reproduced with other themes thought it is not present for
> example with the similar XFCE theme "Orion".

You can repro this on a standard Ubuntu install with Unity? I'm testing in that environment and I'm not seeing the issue. So I wonder what's different.

Comment 9

[i.dani]

Allright i've setup multiple fresh and clean instances of Ubuntu.
I did the following step's on all of them:

1) Download and clean install
2) Install all updates and reboot
3) Run firefox

-> No extra packages or anything. Just a clean Ubuntu with the newest version of Firefox (v53.0.3)

- Ubuntu (Default - Unity) -- 14.04.05 LTS -- http://releases.ubuntu.com/14.04/ubuntu-14.04.5-desktop-amd64.iso
-> The font color is correct! No Problem exists.

- Ubuntu GNOME -- 14.04.05 LTS -- http://cdimage.ubuntu.com/ubuntu-gnome/releases/trusty/release/ubuntu-gnome-14.04.5-desktop-amd64.iso
-> The font color is wrong! Problem exists.

- Xubuntu (Xfce) with Greybird Theme (Default) -- 14.04.05 LTS -- http://cdimage.ubuntu.com/xubuntu/releases/14.04.2/release/xubuntu-14.04.5-desktop-amd64.iso
-> The font color is correct! No Problem exists.

- Xubuntu (Xfce) with  Adwaita Theme -- 14.04.05 LTS -- http://cdimage.ubuntu.com/xubuntu/releases/14.04.2/release/xubuntu-14.04.5-desktop-amd64.iso
-> The font color is wrong! Problem exists.

- Ubuntu GNOME -- 16.04.02 LTS -- http://cdimage.ubuntu.com/ubuntu-gnome/releases/16.04/release/ubuntu-gnome-16.04.2-desktop-amd64.iso
-> The font color is correct! No Problem exists.


----

Do you need anything more?

Comment 10

[Jim Mathies [:jimm]]

Karl, any suggestions? How do we handle minor theme conflict issues under Linux?

Comment 13

[Karl Tomlinson (:karlt)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1358091#c1 identifies ButtonText, thanks.

Adwaita is GTK's default theme (though not Ubuntu's override), and Ubuntu 14.04 is a LTS release, so this probably affects a moderate proportion of users.

The bug appears to be in reading the incorrect colour from the GTK theme, rather than a conflict with Firefox styling.

A regression window would provide the first clues.
I'm assuming this reproduces on any Ubuntu 14.04 install after switching to the Adwaita GTK theme.

Comment 14

[Jim Mathies [:jimm]]

Over to Tracy for regression range work.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 54 but we could still take a fix in 55.

Comment 16

[i.dani]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Too late for 54 but we could still take a fix in 55.

Can we expect a fix for FF55? This is affecting multiple users and it gets more and more critical when using an older version. Especially as there are multiple security vulnerabilities fixed in the latest version.. Thanks for taking a look at it!

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracy, any luck with the regression range? Do you want to pass it over to SV?

Comment 18

[Tracy Walker [:tracy]]

Sorry for delay here.  This works for me on Ubuntu 16.04.02. I do not have an affected machine (VM). 

Which of the distros in comment #9 do we support?

Comment 19

[Abe - QA (:Abe_LV)]

I can reproduce this using the build from comment 9, Ubuntu GNOME.

The regression range is:
Last good revision: 741a720c98cdb92c229376be0badbf036f653bff
First bad revision: 8f1e420699832d42753c6503aa31861be6f5b186
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=741a720c98cdb92c229376be0badbf036f653bff&tochange=8f1e420699832d42753c6503aa31861be6f5b186

Many bugs were fixed in this range, but it seems to be caused by Bug 1320860.

Screen capture if available here: https://testing-1.tinytake.com/sf/MTczNTc2NV81NzM0MzAz

needinfo me if additional testing is required.

Comment 20

[i.dani]

I've just tested with the latest Firefox v55b7 on Ubuntu 14 with Gnome.
Some stuff is fixed, some stuff is still broken (sadly :-/)

- Closing Firefox with Tab's open:
https://image.prntscr.com/image/lvtWG3HbTLaRYY8po0eJNw.png

- Firefox asking to set default browser:
https://image.prntscr.com/image/s2KlAHM_S5qTxlujDFX_aw.png

- "x" to close a tab:
https://image.prntscr.com/image/GJv4ASWpScCtIEaKNkqsIw.png

- Menü buttons are fixed! :-)
https://image.prntscr.com/image/CXwN0WulTrOCJSe_0MVVgw.png

Can we expect this to be fixed in the stable release?

Comment 21

[i.dani]

- about:support info with Firefox v55b7:
https://pastebin.com/tWt9ftnt

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to Abe - QA (:Abe_LV) from comment #19)
> I can reproduce this using the build from comment 9, Ubuntu GNOME.
> 
> The regression range is:
> Last good revision: 741a720c98cdb92c229376be0badbf036f653bff
> First bad revision: 8f1e420699832d42753c6503aa31861be6f5b186
> Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=741a720c98cdb92c229376be0badbf036f653bff&tochange=8f1e
> 420699832d42753c6503aa31861be6f5b186
> 
> Many bugs were fixed in this range, but it seems to be caused by Bug 1320860.
> 
> Screen capture if available here:
> https://testing-1.tinytake.com/sf/MTczNTc2NV81NzM0MzAz
> 
> needinfo me if additional testing is required.

Thank you for looking into this.

https://hg.mozilla.org/mozilla-central/rev/a6aabcb07a6e from that range for
bug 1320860 links to
https://archive.mozilla.org/pub/firefox/nightly/2016/12/2016-12-01-03-02-05-mozilla-central/
for "first release with".

However, I can't reproduce the bad foreground button text color with
data:text/html,<button>Button</button> nor the bad hover color on "You are
about to close 3 tabs. Are you sure you want to continue?", after attempting
to closing the window.  (I don't know how to get the "Quit" / "Save and Quit"
dialog.)

Also, a6aabcb07a6e causes other regressions and was backed out before
relanding in a different form.  I think the transparent menu background was a
different issue.

Are you able to find a regression range or ranges for foreground button text
color with data:text/html,<button>Button</button> and bad hover text color on
"You are about to close 3 tabs. Are you sure you want to continue?", please?

Comment 23

[Karl Tomlinson (:karlt)]

(In reply to i.dani from comment #20)
> Can we expect this to be fixed in the stable release?

I don't know.  The button text color was still not correct when I tested with a 2017-07-01 Nightly.

I'm also seeing many scary GObject warnings with GTK 3.10 (even with other themes).  Are you getting these?:

(firefox:11284): GLib-GObject-CRITICAL **: g_object_ref: assertion 'object->ref_count > 0' failed

(firefox:11284): GLib-GObject-CRITICAL **: g_object_unref: assertion 'object->ref_count > 0' failed

Comment 24

[Abe - QA (:Abe_LV)]

> > The regression range is:
> > Last good revision: 741a720c98cdb92c229376be0badbf036f653bff
> > First bad revision: 8f1e420699832d42753c6503aa31861be6f5b186
> > Pushlog:
> > https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=741a720c98cdb92c229376be0badbf036f653bff&tochange=8f1e420699832d42753c6503aa31861be6f5b186

> I think the transparent menu background was a different issue.

Yes, just noticed. Do we have a separate bug for this? If no, we may need to file one.
 
> Are you able to find a regression range or ranges for foreground button text
> color with data:text/html,<button>Button</button> and bad hover text color on
> "You are about to close 3 tabs. Are you sure you want to continue?", please?

The regression range for this(foreground button text color) is: 
Last good revision: ba62b4bebebfc30b3863d9b1fe220cff9c42f6f1
First bad revision: 8b922bd2f864d6c850f5624f26775348becd2365
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ba62b4bebebfc30b3863d9b1fe220cff9c42f6f1&tochange=8b922bd2f864d6c850f5624f26775348becd2365

It seems to be caused by Bug 1320686

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Martin, if you have a chance, can you take a look at this, since you worked on bug 1320686?

Comment 26

[Martin Stránský [:stransky] (ni? me)]

Sure, I'll look at it.

Comment 27

[Martin Stránský [:stransky] (ni? me)]

The problem is with getting the sButtonText color from label style created by CreateStyleForWidget(). Looks like the obtained style does not have correct parent and is matched as standalone label style instead of button label style. 

A quick workaround can be to use the old code on gtk 3.10:

style = gtk_widget_get_style_context(label);
gtk_style_context_get_color(style, GTK_STATE_FLAG_NORMAL, &color);
sButtonText = GDK_RGBA_TO_NS_RGBA(color);
gtk_style_context_get_color(style, GTK_STATE_FLAG_PRELIGHT, &color);
sButtonHoverText = GDK_RGBA_TO_NS_RGBA(color);

Comment 28

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (:karlt) from comment #23)
> (In reply to i.dani from comment #20)
> > Can we expect this to be fixed in the stable release?
> 
> I don't know.  The button text color was still not correct when I tested
> with a 2017-07-01 Nightly.
> 
> I'm also seeing many scary GObject warnings with GTK 3.10 (even with other
> themes).  Are you getting these?:
> 
> (firefox:11284): GLib-GObject-CRITICAL **: g_object_ref: assertion
> 'object->ref_count > 0' failed
> 
> (firefox:11284): GLib-GObject-CRITICAL **: g_object_unref: assertion
> 'object->ref_count > 0' failed

That doesn't seem to be related to this bug at least from my recent debugging - unfortunately gdb crashes for me on Ubuntu 14.04 so I need to solve that first.

Comment 29

[Martin Stránský [:stransky] (ni? me)]

It's because GetWidget(MOZ_GTK_BUTTON) does not return a plain GtkButton (created by gtk_button_new()) as it was in original code but creates button by gtk_button_new_with_label("M").

Then we add another label style to it:
GtkStyleContext* labelStyle = CreateStyleForWidget(gtk_label_new("M"), style);

which causes the failure here.

Comment 30

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1365556 - remove text-button class from MOZ_GTK_BUTTON style,

When GtkButton is created by gtk_button_new_with_label() it has text-button class.
When any child widget is added this class is removed. We want to emulate the GtkLabel
placed inside GtkButton so remove temporary text-button class on Gtk+ <= 3.10
where it causes issues with Adwaita theme.

Review commit: https://reviewboard.mozilla.org/r/161598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161598/

Comment 31

[Karl Tomlinson (:karlt)]

Comment on attachment 8890480 [details]
Bug 1365556 - remove text-button class from MOZ_GTK_BUTTON style,

https://reviewboard.mozilla.org/r/161598/#review167246

::: commit-message-388d8:3
(Diff revision 1)
> +When GtkButton is created by gtk_button_new_with_label() it has text-button class.
> +When any child widget is added this class is removed. We want to emulate the GtkLabel
> +placed inside GtkButton so remove temporary text-button class on Gtk+ <= 3.10
> +where it causes issues with Adwaita theme.

Thank you for investigating this.

gtk_button_new_with_label() is the standard way to create a GtkLabel inside a GtkButton, and so the
text-button class that comes with that may be what we want.  Getting the style
from the child GtkLabel from gtk_button_new_with_label() may even be a
sensible approach.

I suspect the text-button class is not really the source of the problem here,
but the patch is changing invalidation or something that we don't understand.

In the Adwaita package from
https://packages.ubuntu.com/trusty/gnome-themes-standard-data
these are the only occurrences of text-button that I see:

.toolbar .button.text-button {
    padding: 2px 16px;
}

.header-bar .button.text-button {
    padding: 2px 16px;
}

Those shouldn't be involved here, and shouldn't affect the text color even if they were involved.

Comment 32

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (:karlt) from comment #31)
> Comment on attachment 8890480 [details]
> Bug 1365556 - remove text-button class from MOZ_GTK_BUTTON style,
> 
> https://reviewboard.mozilla.org/r/161598/#review167246
> 
> ::: commit-message-388d8:3
> (Diff revision 1)
> > +When GtkButton is created by gtk_button_new_with_label() it has text-button class.
> > +When any child widget is added this class is removed. We want to emulate the GtkLabel
> > +placed inside GtkButton so remove temporary text-button class on Gtk+ <= 3.10
> > +where it causes issues with Adwaita theme.
> 
> Thank you for investigating this.
> 
> gtk_button_new_with_label() is the standard way to create a GtkLabel inside
> a GtkButton, and so the
> text-button class that comes with that may be what we want.  Getting the
> style
> from the child GtkLabel from gtk_button_new_with_label() may even be a
> sensible approach.
> 
> I suspect the text-button class is not really the source of the problem here,
> but the patch is changing invalidation or something that we don't understand.
> 
> In the Adwaita package from
> https://packages.ubuntu.com/trusty/gnome-themes-standard-data
> these are the only occurrences of text-button that I see:
> 
> .toolbar .button.text-button {
>     padding: 2px 16px;
> }
> 
> .header-bar .button.text-button {
>     padding: 2px 16px;
> }
> 
> Those shouldn't be involved here, and shouldn't affect the text color even
> if they were involved.

Yes, you're right. The .text-button class is not significant here, I think any subclass breaks that. The subclass was actually introduced at Gtk+ 3.10 and indicates a special case of GtkButton (text or image) and is not used much at 3.10.

I did Adwaita/Ambiance theme check and also Adwaita 3.10 vs. 3.14 theme check and it looks like a theme bug to me.

Ambiance 3.10 and Adwaita 3.14 both defines css .button color which works with .button and also .text-button subclass. Adwaita 3.10 does not define the color explicitly for .buttons which is the difference here and rely to inheritance here which is broken by .text-button subclass. 

I expect there's a bug in Gtk 3.10 which does not expect any sub-class at .button and that also break the color inheritance from upper class, maybe some css match optimization or so.

Anyway, I don't think it's worth to deep investigate this. A simple fix would be to add color to .button which we can't expect from Gtk team. The .text-button is also a freshly new at 3.10 and not widely used by Gtk - so looks like to me a good workaround to just remove it from button class for 3.10 as it does this patch.

Comment 33

[Martin Stránský [:stransky] (ni? me)]

(In reply to Abe - QA (:Abe_LV) from comment #24)
> > > The regression range is:
> > > Last good revision: 741a720c98cdb92c229376be0badbf036f653bff
> > > First bad revision: 8f1e420699832d42753c6503aa31861be6f5b186
> > > Pushlog:
> > > https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=741a720c98cdb92c229376be0badbf036f653bff&tochange=8f1e420699832d42753c6503aa31861be6f5b186
> 
> > I think the transparent menu background was a different issue.
> 
> Yes, just noticed. Do we have a separate bug for this? If no, we may need to
> file one.
>  

Transparent menu background it a know bug of Adwaita 3.10 and Fedora 20 carries extra patch for it:
http://pkgs.fedoraproject.org/cgit/rpms/gnome-themes-standard.git/commit/?h=f20&id=3dcdc3cb629936821c39d52f5fff227aeeb014c2

Comment 34

[Karl Tomlinson (:karlt)]

The Button Boxes demo in gtk3-demo uses gtk_button_new_with_label(), which
presumably has button-text, and that renders correctly.  This is not a theme
bug.  The Adwaita rule that should provide the button color is

.background {
    color: @theme_fg_color;
    background-color: @theme_bg_color;
}

This background class is on the GtkWindow, and, as Martin implies, the color
property should be inherited by the descendant button and label, but this is
not happening.

Having the ability to reproduce a bug is an opportunity to find the cause.
Covering it would leave us not knowing when it will appear again.

'The main task of the engineering analyst is not merely to obtain “solu- 
tions” but is rather to understand the dynamic behavior of the system in 
such a way that the secrets of the mechanism are revealed, and that if it is 
built it will have no surprises left for him. Other than exhaustive physical 
experimentation, this is the only sound basis for engineering design, and 
disregard of this cardinal principle has not infrequently led to disaster.'
https://archive.org/details/AnalysisOfNonlinearControlSystemsByGrahamAndMcRuer

I'm not interested in adding code when we don't understand how it works around
a bug.  Such workarounds could just as easily introduce new regressions.
There are too many themes to test them all and use code just because it seems
to happen to make them work.

It is the invalidation here that causes the style to resolve correctly here,
not the removal of the button-text class.  Understanding exactly why and when
this invalidation is required permits us to resolve a whole class of bug,
rather than reproducing and trying patches for each bug in turn as they are
reported.  The understanding also gives us a chance to avoid making the same
mistakes in the future.

Comment 35

[Karl Tomlinson (:karlt)]

Attachment: debug session log

The StyleData for the GtkButton, used in the incorrect style resolution here
was constructed when gtk_button_construct_child() retrieved the style property
"image-spacing", during construction of the GtkButton.

When the parent of the button is set to make the button (and label) a
descendant of the GtkWindow, _gtk_style_context_queue_invalidate() is called,
but the queued event is not processed before the color is retrieved for the
label.

This is essentially the same issue as observed in
https://bugzilla.mozilla.org/show_bug.cgi?id=1272194#c7
It affects GTK versions prior to 3.18.  More recent versions do not use
_gtk_style_context_queue_invalidate().

Queuing invalidations usually provides the expected behavior for GtkWidgets
because they usually retrieve style properties during measuring and drawing,
which are queued in a way that happens after the queued invalidation.
Gecko however is retrieving the style properties immediately and caching them.
They are never corrected after the invalidation.

Comment 36

[Karl Tomlinson (:karlt)]

Attachment: bug 1365556 invalidate widget style contexts after their ancestors are set

Although this is only known to affect buttons with builtin child widgets, it
is difficult to audit all GTK widgets for similar situations, and so the same
defense is applied to all widgets.

Review commit: https://reviewboard.mozilla.org/r/163326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163326/

Comment 37

[Karl Tomlinson (:karlt)]

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

Comment 38

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8892371 [details]
bug 1365556 invalidate widget style contexts after their ancestors are set

https://reviewboard.mozilla.org/r/163326/#review168798

You rule.

Comment 39

[i.dani]

Thanks a lot for looking at this. Do you know in which release this will be published?

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96777f4f483e
invalidate widget style contexts after their ancestors are set r=stransky+263117

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96777f4f483e

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 43

[Karl Tomlinson (:karlt)]

Comment on attachment 8892371 [details]
bug 1365556 invalidate widget style contexts after their ancestors are set

Approval Request Comment
[Feature/Bug causing the regression]:
The bug has existed since bug 1193807 but was unnoticed or unreported until symptoms reported here were triggered by changes for bug 1320686.
[User impact if declined]:
Hard to read text on buttons in some themes, include GNOME's default theme in older LTS distributions.
Potentially some other natively themed widgets are also affects in some theme combinations.
[Is this code covered by automated tests?]:
No.
[Has the fix been verified in Nightly?]:
No.
[Needs manual test from QE? If yes, steps to reproduce]:
Yes.
Load data:text/html,<button>Button</button> on Ubuntu GNOME -- 14.04.05 LTS
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
Mildly risky.
[Why is the change risky/not risky?]:
There are too many different themes to test them all, and each GTK version behaves a little differently.  The patch should only correct bugs, but sometimes correcting one bug makes others visible.
[String changes made/needed]:
None.

Comment 44

[i.dani]

(In reply to Karl Tomlinson (:karlt) from comment #43)
> Comment on attachment 8892371 [details]
> bug 1365556 invalidate widget style contexts after their ancestors are set
> 
> Approval Request Comment
> [Feature/Bug causing the regression]:
> The bug has existed since bug 1193807 but was unnoticed or unreported until
> symptoms reported here were triggered by changes for bug 1320686.
> [User impact if declined]:
> Hard to read text on buttons in some themes, include GNOME's default theme
> in older LTS distributions.
> Potentially some other natively themed widgets are also affects in some
> theme combinations.
> [Is this code covered by automated tests?]:
> No.
> [Has the fix been verified in Nightly?]:
> No.
> [Needs manual test from QE? If yes, steps to reproduce]:
> Yes.
> Load data:text/html,<button>Button</button> on Ubuntu GNOME -- 14.04.05 LTS
> [List of other uplifts needed for the feature/fix]:
> None.
> [Is the change risky?]:
> Mildly risky.
> [Why is the change risky/not risky?]:
> There are too many different themes to test them all, and each GTK version
> behaves a little differently.  The patch should only correct bugs, but
> sometimes correcting one bug makes others visible.
> [String changes made/needed]:
> None.

I've just tested with the latest nightly release and for me everything is perfectly fine now. All wrong colors/buttons are now fixed and i couldn't find any other bug/wrong color. Using Default Install of Ubuntu 14.04.5 LTS.
Thanks for your work!

Comment 45

[Karl Tomlinson (:karlt)]

Thanks for testing.

Comment 46

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/96777f4f483e77572640f671bf64a24f734488f9
bug 1365556 invalidate widget style contexts after their ancestors are set r=stransky+263117

Comment 47

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8892371 [details]
bug 1365556 invalidate widget style contexts after their ancestors are set

Button/theme fix for GTK hard to read text. Let's fix this for accessibility in beta 2.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8b90cfdfc08

Comment 49

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using Gnome on Ubuntu 14.04 32bit using Firefox 53.0 RC. Verified that the issue is no longer reproducible using Firefox 56 beta 11 on the same platform.