User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Build ID: 20150518070114 Steps to reproduce: GtkColorSelectionDialog is deprecated according to https://developer.gnome.org/gtk3/stable/GtkColorSelectionDialog.html and compile deprecation warnings with GTK3 toolkit and it should be replaced by GtkColorChooserDialog.
Created attachment 8652318 [details] [diff] [review] 002_gtk3_color_chooser.patch Patch replacing deprecated GtkColorSelectionDialog with GtkColorChooserDialog when compiled with GTK3 toolkit. It is compatible with GTK2. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a162ff197e4a
Attachment #8652318 - Flags: review?(karlt)
Comment on attachment 8652318 [details] [diff] [review] 002_gtk3_color_chooser.patch nsIColorPicker only supports opaque colors as described at https://html.spec.whatwg.org/multipage/infrastructure.html#simple-colour so the “use-alpha” property on GtkColorChooser should be set to FALSE. >+ return (int)(color_component * 255); color_component * 255 + 0.5 >+ return (gdouble)(color_component / 255); return color_component / 255.0; >+ if (parent_window) >+ gtk_window_set_destroy_with_parent(GTK_WINDOW(color_chooser), TRUE); Mozilla style is to brace. (Jump statements are sometimes accepted as exceptions.) >+void nsColorPicker::ReadSelectedColor(GdkRGBA* color) Please call this SetColor(const GdkRGBA* color) to clarify that color is not an outparam but used to set the color rather than the method looking for a selected color anywhere.
Attachment #8652318 - Flags: review?(karlt) → review-
Created attachment 8653344 [details] [diff] [review] 003_gtk3_color_chooser.patch Thanks for the feedback Karl, I modified it. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11bd4281d73b
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Gtk
Ever confirmed: true
Product: Firefox → Core
Comment on attachment 8653344 [details] [diff] [review] 003_gtk3_color_chooser.patch >+void nsColorPicker::ReadSelectedColor(GdkRGBA* color) Please call this SetColor(const GdkRGBA* color) to clarify that color is not an outparam but used to set the color in the nsColorPicker rather than the method looking for a selected color anywhere.
Attachment #8653344 - Flags: review?(karlt) → review-
Created attachment 8655263 [details] [diff] [review] 004_gtk3_color_chooser.patch I'm sorry about that Karl, I modified the patch.
Comment on attachment 8655263 [details] [diff] [review] 004_gtk3_color_chooser.patch >+void nsColorPicker::SetColor(GdkRGBA* color) Thanks. r+ with (const GdkRGBA* color) here and in the declaration.
Attachment #8655263 - Flags: review?(karlt) → review+
Created attachment 8655369 [details] [diff] [review] patch for check-in Added (const GdkRGBA* color) to definition and declaration. Copying r+ from previous comment.
Assignee: nobody → pjasicek
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
I'd be happy if you'd like to back this out until we have solution, if you prefer, Arnaud. I'd guess it is possible to customize the dialog, but I don't know how much work that would be. GDK_VERSION_MIN_REQUIRED can be used to suppress deprecated warnings.
(In reply to Karl Tomlinson (back Oct 8 ni?:karlt) from comment #12) > I'd be happy if you'd like to back this out until we have solution, if you > prefer, Arnaud. I'm really ambivalent about this... I don't like the idea of loosing coherency over other Gtk applications (which will almost all use eventually the new color picker) but "picking a color on a screen" was one of the nicest feature of input type color. I've opened a GTK bug to discuss the issue: https://bugzilla.gnome.org/show_bug.cgi?id=756132
Seems like GTK people are not against having this feature integrated  though they still need someone motivated to implement this. I would suggest to rollback this until the problem is fixed on GTK side. More precisely, as some good work has been done here, maybe we should just keep this code and add a define to disable it (with a comment explaining the reason and a bug number) to be sure we don't loose it? Also, another issue with this color picker is that "color-activated" signal isn't working (at least for me) so the color button isn't updated (thus no input event) when the value is being changed by the user. This would prevent a web developer to do some "preview" of the result achieved by changing the color (example of what I mean: ) But our code looks good, so I have no idea what is wrong... Petr, would that be OK for you? Or do you have any reason to prefer the new color picker over the old one? : https://bugzilla.gnome.org/show_bug.cgi?id=756132#c5 : http://jsfiddle.net/arnaudbienner/fe050234/1/
I am fine with your suggestion, Arnaud.
Created attachment 8674584 [details] [diff] [review] bug1198256_deactivate_gtk3_color_picker.patch Deactivate the new code for now using another macro check. karlt: I tried your suggestion for removing the deprecated warnings but didn't managed to get it work :( I guess it might work if gtk.h is included in .cpp files compiled independently, which is not the case here since several .h files including gtk.h header are in nsWidgetFactory.cpp (otherwise this would impact all the compilation unit I guess). Otherwise I don't understand how this can work. I tried to look at examples on our codebase but didn't find anything :( so if you have any ideas, they are welcome :) (but maybe it's not such a big deal to have these warnings?)
Assignee: pjasicek → arnaud.bienner
Status: RESOLVED → REOPENED
Attachment #8674584 - Flags: review?(karlt)
Resolution: FIXED → ---
(In reply to Arnaud Bienner from comment #16) > I tried to look at examples on our codebase but didn't find anything :( so > if you have any ideas, they are welcome :) (but maybe it's not such a big > deal to have these warnings?) The issue is not specific to this code, but see https://bugzilla.mozilla.org/show_bug.cgi?id=1214953#c1
(In reply to Karl Tomlinson (ni?:karlt) from comment #17) > The issue is not specific to this code, but see > https://bugzilla.mozilla.org/show_bug.cgi?id=1214953#c1 OK, I thought you wanted to deactivate the warnings for the nsColorPicker.* files only, but regarding your last comment you want to turn off deprecated GTK3 warnings everywhere IIUC, and this is what bug 1214953 is about. So I guess my patch is OK like this. It just misses a proper commit message: I will reupload another patch.
Created attachment 8675285 [details] [diff] [review] bug1198256_deactivate_gtk3_color_picker_2.patch Same patch, but with a proper commit message
bug1198256_deactivate_gtk3_color_picker_2.patch needs to be checked in.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago → 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Now this has been integrated, I think we should backport this to beta, don't you think? To not deliver to final users a version with the new GTK3 color picker, and have the next version using the old color picker again.
Bug 1207310 already disabled GTK3 builds for 42 Beta, but yes, it makes sense to uplift to 43 (currently Aurora). The deactivate patch would have been better in a different bug so that it could have its own status/tracking flags.
status-firefox44: fixed → disabled
Comment on attachment 8675285 [details] [diff] [review] bug1198256_deactivate_gtk3_color_picker_2.patch Approval Request Comment [Feature/regressing bug #]: Remove new feature added from the first patch of this bug. New feature was to use new GTK3 color picker instead of GTK2 color picker for input type color, but GTK3 color picker is missing some features, so use GTK2 color picker for now. [User impact if declined]: According to comment 25, no impact since GTK3 builds are deactivated for 43, but it would better for coherency to have the same code everywhere. [Describe test coverage new/current, TreeHerder]: [Risks and why]: Low: this just reactivate old, tested code [String/UUID change made/needed]:
Attachment #8675285 - Flags: approval-mozilla-aurora?
Agreed with Karl, this is a bit confusing to keep in the same bug. I'll call 43 "affected" for now. GTK3 builds aren't deactivated for 43 by the way; they are for 42. We can set it to "disabled" after the patch lands on aurora if that will make the most sense (if we need to revisit this again)
status-firefox43: fixed → affected
Comment on attachment 8675285 [details] [diff] [review] bug1198256_deactivate_gtk3_color_picker_2.patch Approved for uplift to aurora. This reverses some of the changes landed in September when 43 was in the nightly channel. Keeping GTK2's color picker for now.
Attachment #8675285 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox43: affected → fixed
You need to log in before you can comment on or make changes to this bug.