Closed Bug 1031662 Opened 7 years ago Closed 1 year ago
outline-style:auto doesn't work on GTK in some environments
682 bytes, image/png
47 bytes, text/x-phabricator-request
|Details | Review|
47 bytes, text/x-phabricator-request
|Details | Review|
STEPS TO REPRODUCE 1. load about:config 2. set the layout.css.outline-style-auto.enabled pref to true 3. load data:text/html,<span style="outline:auto">Test EXPECTED RESULTS "Test" surrounded by an outline that looks like it has focus ACTUAL RESULTS An outline that looks like it has focus with a blank interior, i.e. "Test" is not rendered. PLATFORMS AND BUILDS TESTED Bug occurs on Ubuntu 14.04, with default settings. Bug does NOT occur in Kubuntu 14.04 (with KDE).
It's implemented as part of moz_gtk_entry_paint: http://mxr.mozilla.org/mozilla-central/source/widget/gtk/gtk2drawing.c?rev=3ad397becd2b#1568 when 'draw_focus_outline_only' is true. The idea was to skip the gtk_paint_flat_box() call on line 1644 and only draw the border/shadow parts. I don't have a build environment on the Ubuntu machine so I don't know what goes wrong.
I guess gtk_paint_shadow() is painting a background for some themes. GTK would normally put the text content on a child window, which would not be affected by gtk_paint_shadow() on the entry window. I assume something similar could be achieved by changing the order of drawing, but I guess the idea is to paint the outline last, so that it is on top of any content or borders. I expect gtk_paint_focus() would work better because it is frequently used to paint a focus ring after the widget has been drawn. That's probably also better than including a frame shadow in the outline. I was going to suggest using a button widget for gtk_paint_focus() but then I saw a comment saying that some themes will do nothing: http://mxr.mozilla.org/mozilla-central/source/widget/gtk/gtk2drawing.c?rev=3ad397becd2b#979 In selecting a widget for gtk_paint_focus(), GtkLabel is probably a good candidate because there is no other drawing where the theme could cheat and draw the focus elsewhere. Or gCheckboxWidget may work fine.
I tried checkbutton, checkbutton-with-label and label. Neither of those seems to work on my system (Kubuntu). Fwiw, I noticed some weird state dependencies between calls, so that for example: gtk_paint_check() followed by gtk_paint_focus() paints the grey button background and a focus border (as expected), but if you skip the gtk_paint_check() then nothing is painted. And just doing the gtk_paint_check() paints the grey background without the focus border (also as expected). So it seems gtk_paint_focus() depends on some internal state from a preceding GTK call... Anyway, the situation might be different on Ubuntu (without KDE). If you want to experiment with it, you might want to change the "aState->depressed = TRUE" here: http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsNativeThemeGTK.cpp#235 which I used as a hint to moz_gtk_entry_paint() to only paint the focus border: http://mxr.mozilla.org/mozilla-central/source/widget/gtk/gtk2drawing.c#1579 I guess for buttons that part of the state might be used, so I changed it to "state->curpos = SOME_MAGIC_NUMBER" instead for these experiments. (curpos is only used for scrollbars I think) You can control which GTK widget type is used here: http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsNativeThemeGTK.cpp#371
draw_focus() in oxygen-gtk looks broken for links in GtkLabels. It uses state, as described in comment 3, specifically for Gecko apps, but that's not why it is broken. The only other focus it draws is for toplevel GtkWindows. https://projects.kde.org/projects/playground/artwork/oxygen-gtk/repository/revisions/b3a5134bf285947d14069f79b1fe8a0e57423f9c/entry/src/oxygenstylewrapper.cpp#L3313 The gtk-theme-name setting (see MOZ_gdk_display_close) could be used to add an exception for oxygen-gtk. I would probably make ThemeSupportsWidget() return false (if it is important that something is drawn), but I guess a GtkWindow or GtkEntry widget can be used if you like.
Karl, is this something you could help fix? or know someone who could help? If at least we could *detect* which platforms has this bug then we could do some sort of fallback for those.
I started looking at GTK+ 3. The label bug also exists in oxygen-gtk3, which doesn't draw anything in render_focus for any widgets https://projects.kde.org/projects/playground/artwork/oxygen-gtk/repository/revisions/69e849cac18a42d607fef9fc46c8ee4f5c2a387f/entry/src/oxygenthemingengine.cpp#L2248 And render_layout doesn't draw the focus either: https://projects.kde.org/projects/playground/artwork/oxygen-gtk/repository/revisions/69e849cac18a42d607fef9fc46c8ee4f5c2a387f/entry/src/oxygenthemingengine.cpp#L2266 However GTK+ 3 no longer uses theming engines. GTK+ 3.14 docs say "* GtkThemingEngine has been deprecated in GTK+ 3.14 and will be * ignored for rendering. The advancements in CSS theming are good * enough to allow themers to achieve their goals without the need * to modify source code." I see changesets indicating that theming engines are used less in 3.14, but I'm not sure that they are avoided altogether until this changeset for 3.16: https://git.gnome.org/browse/gtk+/commit/gtk?id=4d9d655b4efe9fd23ad18449d3e45fb8cd4d9cf0 Anyway, we shouldn't need to workaround the oxygen-gtk bug for GTK+ 3. That seems to support using GtkLabel for the gtk_paint_focus/gtk_render_focus() call and selecting a workaround for oxygen-gtk with GTK+ 2. ThemeSupportsWidget() returning false, or using a GtkWindow would be my preferred workarounds.
I don't know if or when I'll by able to make a patch for this, and I don't really understand the use cases well enough to guess who would like to make this a priority. NS_THEME_FOCUS_OUTLINE looks like it would be useful as a native exterior focus for the currently focus element (currently drawn with a dotted outline), but AIUI it is currently used only for outline:auto. I don't understand how outline:auto works together with the browser's focus indicators. Don't two elements end up looking focused, or one element have a double focus ring when both the browser and the document are trying to manage drawing focus? Is the document expected to remove the outline:auto style when the document's window is no longer active? Can ThemeSupportsWidget() return false for NS_THEME_FOCUS_OUTLINE to indicate fallback to the same dotted outline used for browser element focus until this bug is fixed?
(In reply to Karl Tomlinson (:karlt) from comment #7) > NS_THEME_FOCUS_OUTLINE looks like it would be useful as a native exterior > focus for the currently focus element (currently drawn with a dotted > outline), > but AIUI it is currently used only for outline:auto. Yes, the primary use case is outline:auto. I think it could be used for implementing drawFocusRing() on a HTML canvas too. I think it's essential for implementing any type of web component that should look like a form control. > I don't understand how outline:auto works together with the browser's focus > indicators. It should work well if you use outline:auto in a :focus selector. It's up to the web developer how they use it though. > Don't two elements end up looking focused, or one element have a double focus > ring when both the browser and the document are trying to manage drawing > focus? If you use it on a focused form control that still renders with the native theme, yes. > Is the document expected to remove the outline:auto style when the > document's window is no longer active? No, outline:auto is simply a drawing tool, it's not correlated with window/element focus at all, unless the author use :focus. > Can ThemeSupportsWidget() return false for NS_THEME_FOCUS_OUTLINE to > indicate fallback to the same dotted outline used for browser element focus > until this bug is fixed? I guess so, but Linux is a Tier-1 platform so I think we should try to get it working.
(In reply to Karl Tomlinson (:karlt) from comment #6) > That seems to support using GtkLabel for the > gtk_paint_focus/gtk_render_focus() call and selecting a workaround for > oxygen-gtk with GTK+ 2. Sounds good to me. I couldn't figure out how to detect when it's not supported though.
(In reply to Mats Palmgren (:mats) from comment #9) > (In reply to Karl Tomlinson (:karlt) from comment #6) > > That seems to support using GtkLabel for the > > gtk_paint_focus/gtk_render_focus() call and selecting a workaround for > > oxygen-gtk with GTK+ 2. > > Sounds good to me. I couldn't figure out how to detect when > it's not supported though. I don't think it is practical to test whether a theme draws something, but the oxygen-gtk theme can be detected through gtk-theme-name as in this code here: https://hg.mozilla.org/mozilla-central/annotate/37d60e3b8be6/toolkit/xre/nsAppRunner.cpp#l2725 We need only check in GTK 2 builds.
(In reply to Mats Palmgren (:mats) from comment #0) > ACTUAL RESULTS > An outline that looks like it has focus with a blank interior, > i.e. "Test" is not rendered. The issue seems to exist no more after porting to GTK+ 3 from Firefox 42. For now, I only saw a broken border(left-top border is rendered) on present Firefox builds.  https://www.phoronix.com/scan.php?page=news_item&px=Firefox-GTK3-Nightly-Builds
If you see that please provide your distro/Gtk+ version/theme and also a screenshot of that issue. Thanks!
This is what the testcase looks like for me, using Firefox 56.0.2 (official Mozilla build) on Kubuntu 17.04 with the default theme (Breeze). Errors: 1. the right and bottom borders are missing 2. the top border is at least 5px too long, it should stop right after the "t"
Same to me. Thansk for Mats to attach screenshot here! My test conditions: Firefox 58.0a1 on Ubuntu 14.04/16.04 with default theme (Ambiance), Fedora 23 with default theme, and Debian GNU Linux 8 with default theme.
Attachment #9117169 - Attachment description: Bug 1031662 - Fix GTK auto-style outline painting. r=mats,karlt → Bug 1031662 - correct outset positioning of GTK auto-style outline. r=mats,karlt
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/5cd557d590e2 correct outset positioning of GTK auto-style outline. r=karlt
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/34b6ced1e552 Use more const in gtk3drawing. r=karlt
You need to log in before you can comment on or make changes to this bug.