1031662 - outline-style:auto doesn't work on GTK in some environments

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

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).

Comment 1

[Mats Palmgren (inactive)]

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.

Comment 2

[Karl Tomlinson (:karlt)]

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.

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Karl Tomlinson (:karlt)]

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.

Comment 5

[Mats Palmgren (inactive)]

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.

Comment 6

[Karl Tomlinson (:karlt)]

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.

Comment 7

[Karl Tomlinson (:karlt)]

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?

Comment 8

[Mats Palmgren (inactive)]

(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.

Comment 9

[Mats Palmgren (inactive)]

(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.

Comment 10

[Karl Tomlinson (:karlt)]

(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.

Comment 11

[Astley Chen (inactive)]

(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[1] from Firefox 42.
For now, I only saw a broken border(left-top border is rendered) on present Firefox builds.

[1] https://www.phoronix.com/scan.php?page=news_item&px=Firefox-GTK3-Nightly-Builds

Comment 12

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

If you see that please provide your distro/Gtk+ version/theme and also a screenshot of that issue. Thanks!

Comment 13

[Mats Palmgren (inactive)]

Attachment: Screenshot

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"

Comment 14

[Astley Chen (inactive)]

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.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1031662 - correct outset positioning of GTK auto-style outline. r=mats,karlt

The x and y tweaks weren't getting used, because they were not being copied to
the local x and y variables too. With that fixed, this seems to work
nicely.

I want to enable outline-style: auto both for parity with other browsers but
also because it unblocks stuff like bug 1583381 / bug 1311444.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1031662 - Use more const in gtk3drawing. r=karlt

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cd557d590e2
correct outset positioning of GTK auto-style outline. r=karlt

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34b6ced1e552
Use more const in gtk3drawing. r=karlt

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5cd557d590e2
https://hg.mozilla.org/mozilla-central/rev/34b6ced1e552

Comment 20

[Karl Tomlinson (:karlt)]

The effect of gtk_render_focus() on a GtkLabel can be seen by tabbing through the "Links" gtk3-demo.

Adwaita's rendering (version 3.24.14) looks a bit weird to me, but I assume that's how the theme has decided to draw the focus ring.