Closed Bug 1027138 Opened 5 years ago Closed 5 years ago

--enable-default-toolkit=cairo-gtk3 requires Gtk+ 3.6


(Firefox Build System :: General, defect)

Not set


(firefox33 fixed)

Tracking Status
firefox33 --- fixed


(Reporter: glandium, Assigned: stransky)




(1 file, 2 obsolete files)

Because of gtk_menu_button_new.

Same concern as bug 1027000. We may or may not want that depending on what systems we target.
Blocks: 984075
Debian wheezy (current stable) has 3.4
Fedora 16 (nov 2011) had 3.2 Fedora 17 (may 2012) had 3.4. Fedora 18 (jan 2013) came with 3.6.
OpenSuSE 11.4 (mar 2011) had 3.0. 12.1 (nov 2011) had 3.2, 12.2 (jul 2012) had 3.4, 12.3 (mar 2013) has 3.6.
RHEL7/Centos7 have 3.8.
Blocks: try-gtk3
Ubuntu 12.04 (currently LTS, and what we have on test slaves) has 3.4, 13.04 has 3.6,
Martin, could you take a look at this? Backing out bug 984075 is not really thrilling, as it breaks arrows.
Assignee: nobody → stransky
We can use combo box arrow as a workaround for gtk 3.4 system - seems to work as expected.
Attached patch patch (obsolete) — Splinter Review
Karl, what about this one? It's not ideal but it works as a workaround.
Attachment #8445869 - Flags: review?(karlt)
I'd rather not have the behaviour depend on the version of GTK you're building against, especially when most people are going to do local builds with versions greater than 3.6 and automation won't.
The problem in bug 984075 was with the arrow size, which was zero because the widget was not visible.

I suggest reverting the changes from bug 984075, and instead using gtk_widget_show(), like GtkMenuButton, to make the widget visible, so it will have a size.

That may still leave an issue because Adwaita 3.10.0 styles GtkMenuButtons differently from GtkToggleButtons so as to remove the border radius beside the menu and make the background color match the menu.
I don't know of any native menu buttons in Firefox, so that may not be a problem.
If there are native menu buttons, then gtk_widget_path_iter_set_name() may be useful.
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #9)
> If there are native menu buttons, then gtk_widget_path_iter_set_name() may
> be useful.

No it won't.  names are different from types.

The GType system can be used at run time to check for availability of and make GtkMenuButtons, if this is necessary, but that can probably wait.
Attached patch patch, v.2 (obsolete) — Splinter Review
This one reverts the fix from Bug 984075 and adds widget_show(). I didn't see any problem on Fedora 20 with the toogle button.
Attachment #8445869 - Attachment is obsolete: true
Attachment #8445869 - Flags: review?(karlt)
Attachment #8446421 - Flags: review?(karlt)
Comment on attachment 8446421 [details] [diff] [review]
patch, v.2

Please explain how the arrow rendering is fixed in the checkin comment.
(It is particularly hard to work out from the patch when the fix is combined with the backout of the previous fix.)
Attachment #8446421 - Flags: review?(karlt) → review+
Thanks. There's the one with updated comment. 

No need to run try as it modifies gtk3 only file gtk3drawing.c.
Keywords: checkin-needed
Actually, please do a try first. With this applied:
Keywords: checkin-needed
(In reply to Martin Stránský from comment #15)
> ok, there's the try here -

You're good to go.
Keywords: checkin-needed
Attachment #8446421 - Attachment is obsolete: true
That looks incredibly orange-tastic to me.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> That looks incredibly orange-tastic to me.

That's because gtk3 support is still very orange-tastic. But before that patch, it was red-tastic. And normal builds are not using the gtk3 code as mentioned in comment 13.
Keywords: checkin-needed
Fair enough. Pushed as a ride-along to fx-team.

Note that our bug marking tool doesn't like the commit message on this (due to the "revert" in it), so there's a very good chance this bug isn't going to be resolved when merged to m-c unless whoever does the merge happens to notice and does so manually.
Keywords: checkin-needed
MCMerge claimed this was a security bug, which is the only reason it caught my eye while doing the merge... :\
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1034064
No longer blocks: 1034064
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.