Summary: | The focus ring is not shown while navigating on some pages, css handling bug is possible | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anatoly Borodin <anatoly.borodin> | ||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alp, anatoly.borodin | ||||||||||
Priority: | P2 | Keywords: | Cairo, GoogleBug, Gtk | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Other | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 15608 | ||||||||||||
Attachments: |
|
Description
Anatoly Borodin
2008-09-01 12:35:50 PDT
Created attachment 23100 [details]
correct behavior
Created attachment 23101 [details]
incorrect behavior
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. Created attachment 23684 [details]
Fix focus ring painting
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.
(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? Don't we use a special keyword for the focus ring color? Doesn't the value of that color come from the RenderTheme? (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. (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.) (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? 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. 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. Created attachment 23771 [details]
Fix focus ring painting, take two
Fixed to use the color passed in to drawFocusRing()
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. Comment on attachment 23771 [details]
Fix focus ring painting, take two
Seems fine, r=me
Landed in r37521. |