Location/Search Fields currently have the pre-Yosemite glowy effect (top) should be flatter, different blue (bottom).

HTML/CSS example and effect. http://people.mozilla.org/~shorlander/bugs/Bug-1084598-yosemite-field-focus.html

(In reply to Stephen Horlander [:shorlander] from comment #1) > HTML/CSS example and effect. > > http://people.mozilla.org/~shorlander/bugs/Bug-1084598-yosemite-field-focus. > html Meant to say "example of color and effect".

Can someone assign me please?

(In reply to Brandon Cheng from comment #3) > Can someone assign me please? Of course! Thanks for helping. :-)

Just noticed that the find bar field focus styling should also be updated.

I'm running into a problem with getting the correct focus ring color and need advice on how to best handle it. -moz-mac-focusring returns rgba(59, 153, 252, 1), which is the correct system color when calling NSColor.keyboardFocusIndicatorColor(). However, the focus ring color Safari and Finder Search use seems very different. It's a much lighter blue. I've tried manipulating the opacity, but the color still doesn't match. There appears to be another color transformation on the keyboardFocusIndicatorColor going on (in addition to opacity) that Safari, Finder, and other native apps are using. Could someone that knows more about Yosemite provide insight? Thanks.

(In reply to Brandon Cheng from comment #6) > I'm running into a problem with getting the correct focus ring color and > need advice on how to best handle it. -moz-mac-focusring returns rgba(59, > 153, 252, 1), which is the correct system color when calling > NSColor.keyboardFocusIndicatorColor(). However, the focus ring color Safari > and Finder Search use seems very different. It's a much lighter blue. > > I've tried manipulating the opacity, but the color still doesn't match. > There appears to be another color transformation on the > keyboardFocusIndicatorColor going on (in addition to opacity) that Safari, > Finder, and other native apps are using. Could someone that knows more about > Yosemite provide insight? Thanks. Without having investigated this in detail, is the indicator on native apps displayed as part of the background and therefore getting blended with the translucent toolbar background or something? Does the color still wind up being different on native apps where the text field is on a plain white background? Another approach would be colorsampling a native app with textfield on a white and black background and deducing the "real" native color from there... although I'm surprised it doesn't match the thing the API provides us with. :-\ Markus might know more here, too?

Native focus rings are partially transparent, yes. And it looks like keyboardFocusIndicatorColor is the right function to call, since it uses _CopyDefaultFocusRingCGColorRef, which is also used by native focus ring drawing internally (as far as I can tell from disassembled AppKit + CoreGraphics contents). If applying an alpha value to that color isn't enough to get us to match the native focus rings, we might be missing a color space conversion, though I don't know at what point and between what color spaces. How far off are the colors?

(In reply to Markus Stange [:mstange] from comment #8) > Native focus rings are partially transparent, yes. And it looks like > keyboardFocusIndicatorColor is the right function to call, since it uses > _CopyDefaultFocusRingCGColorRef, which is also used by native focus ring > drawing internally (as far as I can tell from disassembled AppKit + > CoreGraphics contents). If applying an alpha value to that color isn't > enough to get us to match the native focus rings, we might be missing a > color space conversion, though I don't know at what point and between what > color spaces. > How far off are the colors? Brandon, can you answer this question? :-)

(In reply to :Gijs Kruitbosch from comment #9) > (In reply to Markus Stange [:mstange] from comment #8) > > How far off are the colors? > > Brandon, can you answer this question? :-) Thanks for your analysis Markus. The colors seem very far off, but I guess it may be acceptable depending on who's judging. I attached a 3-way comparison. The search bar on the bottom is Firefox with the blur radius of the shadow set to 0.

Sorry if you're the wrong person to needinfo Markus.

I'm the right person to needinfo, but I probably won't have time for more investigation of this soon.

10.11 uses this updated focus ring look as well. Is there any chance that this bug is going to be revisited at some point? Was the cause of the color discrepancy ever found? It kind of just looks like transparency issue to me... Anyway sorry to nag on this, but I thought I'd chime in and give my 2 cents in hoping this isn't forgotten about.

So I've given up on using [NSColor keyboardFocusIndicatorColor]. This patch instead creates a temporary surface, paints a focus ring into it using NSSetFocusRingStyle (which results in the correct color), and reads the pixel value from that surface. It works well on 10.10 but needs to be tested on earlier versions of OS X. Brandon, can you do that? Do you want to take the patch from here?

(note to myself to come back to the bug if there is no activity in the short term)

(In reply to Markus Stange [:mstange] from comment #15) > Created attachment 8641204 [details] [diff] [review] > wip > > So I've given up on using [NSColor keyboardFocusIndicatorColor]. This patch > instead creates a temporary surface, paints a focus ring into it using > NSSetFocusRingStyle (which results in the correct color), and reads the > pixel value from that surface. > It works well on 10.10 but needs to be tested on earlier versions of OS X. > Brandon, can you do that? Do you want to take the patch from here? I would love to! Thanks a lot for that unique workaround.

Tried to address the color discrepancy problem by updating GetColorFromNSColor(). Can you verify if this patch works on your system?

(In reply to taslterminal+dev from comment #18) > Created attachment 8642613 [details] [diff] [review] > Update GetColorFromNSColor(), browser.css > > Tried to address the color discrepancy problem by updating > GetColorFromNSColor(). I tested your change but unfortunately the color still isn't correct. (It's still too dark and too blue-ish and not enough cyanish.) I had really high hopes for this to work, too bad it didn't.

(In reply to Markus Stange [:mstange] from comment #19) > I tested your change but unfortunately the color still isn't correct. (It's > still too dark and too blue-ish and not enough cyanish.) > I had really high hopes for this to work, too bad it didn't. Too bad really! The FocusRing color is [keyboardFocusIndicatorColor, opacity: 0.5] (not 0.46 as in my patch). I'm pretty sure about that. Unfortunately, I can't replicate the problem on my mac with the patch applied. Can you try this out to see if it's any different? - Set System Display color profile to sRGB. - Turn on "Reduce Transparency" in Settings > Accessibility. Transparency seem to cause color change in the toolbar background. - Compare Firefox's -moz-mac-focusring (using HTML) with OSX's keyboardFocusIndicatorColor (using color picker in Preview.app). - Compare Firefox's search box with other text box (with no transparent background like Finder) Thanks for your time.

I think there's a larger color space conversion issue going on in the entire browser. The color rgb(59,153,252) shows a distinct difference when rendered in Firefox vs Safari. To demonstrate, I've attached an image of the two browsers showing the following data URI: data:text/html,<style>body{background-color:rgb(59,153,252);}</style> The patches of squares in the bottom left are overlays of the colors from the other browser for comparison. I found this when shifting through my display profiles in System Preferences and noticing that Firefox doesn't change any of its colors (even in the browser chrome) while Safari does. What's interesting is that Firefox starts to, but upon redraw, reverts to the color before. All this may be unique to my system, so please let me know if this is reproducible. I've searched on Bugzilla to see if this bug may have been reported in the past. The closest I've found that may be related is bug 860340, which appears to have been reported by a Google Chrome engineer. In terms of testing your patch Markus, it appears to work well on my Yosemite and Mavericks installations. I'm not sure if the finding above changes whether or not it's the best option to deploy though.

(In reply to Brandon Cheng from comment #21) > I think there's a larger color space conversion issue going on in the entire > browser. The color rgb(59,153,252) shows a distinct difference when rendered > in Firefox vs Safari. I noticed that too. Safari apply system color profile to both UI and website. Chrome apply system color profile to UI. Firefox nothing. Firefox does support color profile but it's not on by default. With my patch, I meant to make firefox to at least use the system color profile for UI colors but it doesn't seem to be working. Perhaps CreateSystemColorSpace failed somehow and fallback to deviceColor, I'll try looking into it. Concerning Markus' patch, I think it's better to leave -moz-mac-focusring as solid color (without alpha channel). It's too soon but see https://bugzilla.mozilla.org/show_bug.cgi?id=1128204 Also, it returns pretty much the same color as unpatched firefox on my system. Not sure if it's caused by El Capitan.

(In reply to taslterminal+dev from comment #22) > (In reply to Brandon Cheng from comment #21) > > I think there's a larger color space conversion issue going on in the entire > > browser. The color rgb(59,153,252) shows a distinct difference when rendered > > in Firefox vs Safari. > > I noticed that too. > Safari apply system color profile to both UI and website. > Chrome apply system color profile to UI. > Firefox nothing. Firefox does support color profile but it's not on by > default. I did a little more research thanks to your help. OS X 10.8 brought color management changes to apps. Images/colors with no profiles will default to sRGB instead of the one in the Display Profile now. This is based off of the following statement from a color meter app developer's blog. I couldn't find an official statement in OS X release notes. > From Ricci Adams in http://www.ricciadams.com/articles/osx-color-conversions/ >In OS X 10.8 Mountain Lion, color management has again changed. When an image lacks its own profile, the system now defaults to sRGB rather than the main display's color profile. This results in more frequent color conversions (and thus color meter mismatches).Note: As of 10.8 GM, Safari and Chrome once again default to the main display's profile. > Also, it returns pretty much the same color as unpatched firefox on my system. Not sure if it's caused by El Capitan. I noticed that on Yosemite too. I took your code and applied the Grayscale color profile and that seemed to do something, so I'm not sure what's up with CreateSystemColorSpace(). I'm looking into a way to grab the Display Profile's color space, but this may be work for a different bug. For this bug, I'm wondering if we should just take the CSS from Markus's patch and move forward.

(In reply to Brandon Cheng from comment #23) > I noticed that on Yosemite too. I took your code and applied the Grayscale > color profile and that seemed to do something, so I'm not sure what's up > with CreateSystemColorSpace(). > > I'm looking into a way to grab the Display Profile's color space, but this > may be work for a different bug. For this bug, I'm wondering if we should > just take the CSS from Markus's patch and move forward. from gfx/2d/QuartzSupport.mm CGColorSpaceRef CreateSystemColorSpace() { CGColorSpaceRef cspace = ::CGDisplayCopyColorSpace(::CGMainDisplayID()); if (!cspace) { cspace = ::CGColorSpaceCreateDeviceRGB(); } return cspace; } As you can see, CreateSystemColorSpace() should return the Main Display's Color Space.

Changes: - Update CSS to use correct alpha value, increase outline offset by 0.5px. - GetColorFromNSColor: rename deviceColor to displayColor.

(In reply to taslterminal+dev from comment #22) > Concerning Markus' patch, I think it's better to leave -moz-mac-focusring as > solid color (without alpha channel). Why do you think so? If the color is opaque, you need to put the real alpha value somewhere else (for example into the CSS opacity in your patch). Why not use the real alpha value from the color itself?

(In reply to Markus Stange [:mstange] from comment #26) > Why do you think so? If the color is opaque, you need to put the real alpha > value somewhere else (for example into the CSS opacity in your patch). Why > not use the real alpha value from the color itself? I know, the point is we may not be the only one using -moz-mac-focusring. Adding an alpha channel may break something somewhere for someone else. People may not expect the color palette to have an alpha channel. Since we know the alpha value is 0.5 it's ok to leave the color opaque. Also once CSS Color Module Level 4 is implemented, we can use the color() function to add alpha value to the color without the need for hack like mine.

Looking a bit harder, Finder's search bar seem to use a style different from other textboxes. So (keyboardFocusIndicatorColor, 0.5) doesn't suffice. The attached image is what I figure the original focus ring should look like if we assume the alpha value to be 0.5. As you can see, only the top border color is close to keyboardFocusIndicatorColor, the others three is lighter and has slightly different hue. There's also a very light blue bar below the top border which doesn't exist when the search-box isn't focused. Screenshot was taken in sRGB so no color profile conversion should occurred.

Enable "Increase contrast" mode show that the focused search-box have 2 different color rings put on top of each other. Normal text boxes only have 1 ring same as our current patches. I still haven't figured out what kind of dark magic was used to get the unknown colors. Kind of stuck there. Is there any suggestions?

This image show the search-box focus rings as rendered by native app. Alpha value is not uniform. Colors are slightly different from "keyboardFocusIndicatorColor()" as well. From the top down: Blue, Blue + Increase Contrast, Graphite, Graphite + Increase Contrast.

Markus, can you describe what's necessary to move forward here?

(In reply to :Gijs Kruitbosch from comment #31) > Markus, can you describe what's necessary to move forward here? I think we need to move forward with Markus's original approach for now. D hit the original problem of color spaces, which appears to be an issue across the browser. But everything I've tried to extract the color space of the display has failed. Markus and D, would you agree?

Can this be added to the [qx] project ?

(In reply to Guillaume C. [:ge3k0s] from comment #33) > Can this be added to the [qx] project ? --> Philipp?

I've narrowed this down to a bug with hardware acceleration. Once hardware acceleration is turned off, the display ICC profile in OS X preferences is properly respected. I tested this by rending pure blue and comparing with Safari. (I'm actually embarrassed I didn't try this simple preference before.) I'll be writing a patch based on D and Markus's work that incorporates the style changes without the color hacks. I'm also going to do a little more research on whether or not the hardware acceleration bug has been reported yet. To note, now that we know this is related to hardware acceleration, some people troubleshooting this issue may not have been seeing it due to their graphics card. (And thus, thought D and I were talking nonsense. I promise we're not. :])

Hi Brandon, how is your work here coming along? Is there anything I can do to help?

Thanks for checking in. I do have an update. The latest stable changes a few things. Since bug 1187322 became fixed, turning off hardware acceleration doesn't fix the colorspace issue. However, toggling layers.offmainthreadcomposition.enabled to false does. I imagine the off main thread compositor is incorrectly obtaining color spaces someone, while the main one is fine. In terms of the basic CSS changes, I'm not sure if hardcoding the color or D's workaround is better. I've played with generating a separate -moz-mac-focusring-alpha tag as well. For now, I have a git diff patch that uses D's workaround. It's nice as elegant as I'd like it to be, so feedback on alternative methods would be appreciated. I know Mozilla changed the way reviews are made, so I'll look into doing that once my LDAP access is restored in a different bug.

(In reply to Brandon Cheng from comment #37) > I know Mozilla changed the way > reviews are made, so I'll look into doing that once my LDAP access is > restored in a different bug. Well, Mozilla provides an alternative review system called 'ReviewBoard', which is entirely optional. So please feel free to use the 'old' system until you're ready to use the 'new' one!

Can we get a try build with the current patch? Shorlander suggests getting the exact color right (bug 1274036) might be of lesser importance than fixing the outline style in general (this bug), and so while we'd still like to get it right it, even a somewhat-wrong color would still be a significant improvement.

The outline method has a bug with the site identity button. It causes it to jump due to the absolute positioning hack. The patch I'm attaching now is a simplified version of Markus's patch. I'd like to move forward, but need to request assistance here. This patch changes -moz-mac-focusring to a RGBA color. I'm not aware if the additional alpha will break other parts of the code. Gijs, Justin, or Mike, would any of you be able to review this patch?

Comment on attachment 8762782 [details] [diff] [review] Patch1.diff I think Markus is probably the best reviewer for the nsLookAndFeel change, which also feels like the most significant change.

Comment on attachment 8762782 [details] [diff] [review] Patch1.diff Review of attachment 8762782 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! We'll also need something similar for the findbar textbox, and maybe a few other textboxes. Actually we should fix up all users of @focusRingShadow@. But that should be done in a new bug.

(In reply to Brandon Cheng from comment #41) > This patch > changes -moz-mac-focusring to a RGBA color. I'm not aware if the additional > alpha will break other parts of the code. I thought I had replied to comment 27, but apparently I hadn't: I don't think this will cause any problems. We don't have any rendering code that can't handle non-opaque colors, and I don't see a way how this would break existing web content that uses -moz-mac-focusring (which is probably extremely rare).

Comment on attachment 8762782 [details] [diff] [review] Patch1.diff Review of attachment 8762782 [details] [diff] [review]: ----------------------------------------------------------------- This changes both the URL and search bar, AFAICT. Assuming that works (Markus, are you saying it doesn't?) then this wfm. We can fix up anything else we notice in followup bugs as said.

I'm not saying it doesn't work. I'm assuming it works. I haven't tested it myself. I'm saying that "noticing" other things that need fixing is really easy because you can just search for @focusRingShadow@.

(In reply to Markus Stange [:mstange] from comment #46) > I'm not saying it doesn't work. I'm assuming it works. I haven't tested it > myself. Right, sorry, I misread your comment - I assumed you meant the search field, but you meant the cmd-f find-in-page findbar.

Thanks for the patch, Brandon! Can you please add appropriate checkin metadata and then re-set checkin-needed? https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F Thanks!

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/dbe8807bcf0d [OS X 10.10 and later] fix focus styling for url bar and search box, r=mstange,gijs

(Landed this manually for you, Brandon :-) )

(In reply to :Gijs Kruitbosch from comment #50) > (Landed this manually for you, Brandon :-) ) Thanks a lot Gijs! Should I mark this as resolved?

(In reply to Brandon Cheng from comment #51) > (In reply to :Gijs Kruitbosch from comment #50) > > (Landed this manually for you, Brandon :-) ) > > Thanks a lot Gijs! Should I mark this as resolved? It'll semi-automatically get updated when this merges to central. :-)

I have reproduced this bug with Firefox nightly 36.0a1 (2014-10-17)) on Windows 10, 64 Bit. The Bug's fix is now verified on latest nightly 50.0a1. Build ID 20160713030216 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 [bugday-20160713]

Should a bug be filed about all other textboxes ?

(In reply to Guillaume C. [:ge3k0s] from comment #55) > Should a bug be filed about all other textboxes ? Which textboxes?

(In reply to (unavailable 07/20-07/21) Jared Wein [:jaws] (please needinfo? me) from comment #56) > (In reply to Guillaume C. [:ge3k0s] from comment #55) > > Should a bug be filed about all other textboxes ? > > Which textboxes? I can think at least about the textboxes in arrow panels (e.g. new bookmark panel) and all content windows (e.g. library). I've seen that the find bar textboxes has been taken care of in bug 1285814. See also to have a complete list : In reply to Markus Stange [:mstange] from comment #46) > I'm saying that "noticing" other things that need fixing is really easy > because you can just search for @focusRingShadow@.

(In reply to Guillaume C. [:ge3k0s] from comment #57) > (In reply to (unavailable 07/20-07/21) Jared Wein [:jaws] (please needinfo? > me) from comment #56) > > (In reply to Guillaume C. [:ge3k0s] from comment #55) > > > Should a bug be filed about all other textboxes ? > > > > Which textboxes? > > I can think at least about the textboxes in arrow panels (e.g. new bookmark > panel) and all content windows (e.g. library). I don't think I see broken focus styling in the library, and certainly auditing for focusRingShadow doesn't show me any rules that would apply there specifically. Maybe I misunderstand what you mean? > See also to have a complete list : > > In reply to Markus Stange [:mstange] from comment #46) > > I'm saying that "noticing" other things that need fixing is really easy > > because you can just search for @focusRingShadow@. I filed bug 1287763.

(In reply to :Gijs Kruitbosch from comment #58) > (In reply to Guillaume C. [:ge3k0s] from comment #57) > > (In reply to (unavailable 07/20-07/21) Jared Wein [:jaws] (please needinfo? > > me) from comment #56) > > > (In reply to Guillaume C. [:ge3k0s] from comment #55) > > > > Should a bug be filed about all other textboxes ? > > > > > > Which textboxes? > > > > I can think at least about the textboxes in arrow panels (e.g. new bookmark > > panel) and all content windows (e.g. library). > > I don't think I see broken focus styling in the library, and certainly > auditing for focusRingShadow doesn't show me any rules that would apply > there specifically. Maybe I misunderstand what you mean? My bad. It seems that the current styling is fine.