Bug 20592 – The focus ring is not shown while navigating on some pages, css handling bug is possible

• Component: CSS
• Status: RESOLVED FIXED

[Anatoly Borodin]

The focus ring disappears on some Internet pages, for example https://www.google.com/accounts/ServiceLogin : the focus ring disappears after passing the "Sign in" button and appears again at the "create one for free" link after a few tabs.

The analyze of the page's source has given a simple test case, the focus ring is not shown on the "bad" page.

The bug appears in the gtk branch built under FreeBSD (e.g. git f676a7ee078a079bc3bd9781d4d7c9ca5fd1d79c).

[Anatoly Borodin]

Created attachment 23100 [details]
correct behavior

[Anatoly Borodin]

Created attachment 23101 [details]
incorrect behavior

[Alp Toker]

This seems to be due to the focus ring function in GraphicsContextCairo relying on the GraphicsContext to paint the focus ring, whereas other ports just use a platform focus ring paint function.

[Alp Toker]

Created attachment 23684 [details]
Fix focus ring painting

[Eric Seidel (no email)]

Comment on attachment 23684 [details]
Fix focus ring painting

This seems like the wrong fix.  Why would Gtk want to ignore the color parameter and use the stroke color instead?  Shouldn't whatever callers be fixed to pass the strokeColor() in the necessary cases for gtk?  I'm just not even sure why that change would be needed.  This seems like the wrong layer to abstract at.

[Alp Toker]

(In reply to comment #5)
> (From update of attachment 23684 [details] [edit])
> This seems like the wrong fix.  Why would Gtk want to ignore the color
> parameter and use the stroke color instead?  Shouldn't whatever callers be
> fixed to pass the strokeColor() in the necessary cases for gtk?  I'm just not
> even sure why that change would be needed.  This seems like the wrong layer to
> abstract at.
> 

The color passed in always seems to be the same which isn't what we want. It looks OK when the focus ring is rounded, thick and transparent, but the GTK+ focus ring is a single pixel dotted rectangle which usually has the text/foreground color, and is used for anchors as well as controls.

Not sure how to get the desired color passed in here except by using the stroke or fill colors. Any suggestions?

[Dave Hyatt]

Don't we use a special keyword for the focus ring color?  Doesn't the value of that color come from the RenderTheme?

[Alp Toker]

(In reply to comment #7)
> Don't we use a special keyword for the focus ring color?  Doesn't the value of
> that color come from the RenderTheme?
> 

I didn't notice a way to adjust the appearance of focused anchor elements (the most common user of drawFocusRing() since RenderTheme handles all controls itself). Looking at RenderTheme/RenderStyleConstants.h the EAppearance values are mostly concerned with widgets/controls which doesn't seem to help.

[Alp Toker]

(In reply to comment #7)
> Don't we use a special keyword for the focus ring color?  Doesn't the value of
> that color come from the RenderTheme?
> 

I missed the question! Yes, the color does come from RenderTheme but it's constant, whereas the GTK+ focus ring works best when it either uses the current foreground color of the context (ie. matching the color of the text), or otherwise contrasts with the background it's painted on. (In older applications, the focus ring would XOR, but still was never stuck to a single color.)

[Alp Toker]

(In reply to comment #9)
> (In reply to comment #7)
> > Don't we use a special keyword for the focus ring color?  Doesn't the value of
> > that color come from the RenderTheme?
> > 
> 
> I missed the question! Yes, the color does come from RenderTheme but it's
> constant, whereas the GTK+ focus ring works best when it either uses the
> current foreground color of the context (ie. matching the color of the text),
> or otherwise contrasts with the background it's painted on. (In older
> applications, the focus ring would XOR, but still was never stuck to a single
> color.)
> 

Indeed, a little testing shows that the value of strokeColor() doesn't _always_ contrast with the background, so the technique in my patch isn't perfect but passable. Any thoughts?

[Dave Hyatt]

Don't we just need to support invert when painting outlines?

http://www.w3.org/TR/2004/CR-css3-ui-20040511/#outline-color0

That seems to be the bug here.

[Dave Hyatt]

If you want to use the current foreground color, then currentColor is the right keyword.  Not sure we support that yet though.

Either way I think the CSS spec covers what you need.  We just have to add the right support for it.

[Alp Toker]

Created attachment 23771 [details]
Fix focus ring painting, take two

Fixed to use the color passed in to drawFocusRing()

[Alp Toker]

Comment on attachment 23771 [details]
Fix focus ring painting, take two

>+    // We prefer the stroke color for focus rings in GTK+.
>+    setColor(cr, color);

Above comment isn't necessary any more, of course.

[Darin Adler]

Comment on attachment 23771 [details]
Fix focus ring painting, take two

Seems fine, r=me

[Alp Toker]

Landed in r37521.