615589 - -moz-mac-focusring color should be keyboardFocusIndicatorColor (NSColor)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stefan [:stefanh]]

(this came up when doing bug 613358)

Using -moz-mac-focusring for a box-shadow with blur doesn't match the focusrings drawn by nsNativeThemeCocoa. Also, our current focusring color doesn't match the webkit-focusring-color (it's keyboardFocusIndicatorColor).

tree focusring colors might be necessary to hardcode in the css file, since we use a solid border there, the keyboardFocusIndicatorColor might be too dark. But that is atm the only disadvantage I can think of.

Using keyboardFocusIndicatorColor will also get us the graphite color for free.

Comment 1

[Markus Stange [:mstange]]

Hmm, I thought I had looked at keyboardFocusIndicatorColor when writing the patch for bug 481853... are you sure it's correct?

Comment 2

[Stefan [:stefanh]]

Attachment: Focusring test

(In reply to comment #1)
> Hmm, I thought I had looked at keyboardFocusIndicatorColor when writing the
> patch for bug 481853... are you sure it's correct?

Comparing the 2 colors (webkit/moz) one can see that they differ.

Comment 3

[Stefan [:stefanh]]

Attachment: try-out patch

Applying this patch, makes the colors look the same. One will need to look through all the existing css files and see what the effect are, though.

Comment 4

[Stefan [:stefanh]]

(In reply to comment #2)
> Comparing the 2 colors (webkit/moz) one can see that they differ.

erm, "Comparing the webkit color with our current focusring color", of course.

Comment 5

[Markus Stange [:mstange]]

I guess what I really meant was "Is keyboardFocusIndicatorColor really more correct than the hardcoded color?" I thought I had taken the color values directly from a native focus ring, at the most opaque point I could find.

Comment 6

[Stefan [:stefanh]]

Well, it's much more correct if you want to use the color to create a blurred focusring (using box-shadow, for example). My idea was that we probably want to do this more often than just using a solid color.

Comment 7

[Karsten Düsterloh]

(In reply to comment #5)
> I guess what I really meant was "Is keyboardFocusIndicatorColor really more
> correct than the hardcoded color?" 

Yes, because if you look at the screenshots of me and Stefan in bug 613358, you'll see that the hardcoded colours may differ from the natively drawn focusring colours - whenever a hardcoded colour fit for me, it didn't for Stefan, and vice versa. 
Using Stefan's patch here will make the colour fit for both.

Comment 8

[Stefan [:stefanh]]

If you use the current -moz-mac-focusring color with blur it doesn't fit with the OS color, I think. You can hardcode the color in your css, but that seems to result in color variations between different systems.

Comment 9

[Markus Stange [:mstange]]

(In reply to comment #7)
> (In reply to comment #5)
> > I guess what I really meant was "Is keyboardFocusIndicatorColor really more
> > correct than the hardcoded color?" 
> 
> Yes, because if you look at the screenshots of me and Stefan in bug 613358,

Oh, I should have read that bug before commenting, sorry about that.
Interesting! Are you both on 10.6, or is one of you on 10.5? I don't understand how a hardcoded color can result in such stark differences, except if our inverse color correction doesn't work correctly...

Comment 10

[Stefan [:stefanh]]

(In reply to comment #9)

> Interesting! Are you both on 10.6, or is one of you on 10.5?

Both are on 10.6.

Comment 11

[Markus Stange [:mstange]]

Hrm... does playing with the gfx.color_management.mode setting make any difference?

Comment 12

[Stefan [:stefanh]]

Attachment: test-extension to compare focus rings

Just to clear out any misunderstandings, I've made an extension to compare focus rings drawn by nsNativeThemeCocoa and box-shadow made rings. The extension will add a menuitem in the Tools menu, selecting that will load a window with 2 textboxes. The code looks like this:

<?xml version="1.0"?>
<!DOCTYPE window SYSTEM "chrome://splitter-test/locale/textbox.dtd">

<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        width="600"
        height="400">
  <hbox style="padding-top: 50px;">
    <textbox/>
  </hbox>
  <hbox style="padding-top: 2px;">
    <textbox style="-moz-appearance: none; border-width: 0; margin: 2px; padding: 2px;
                    box-shadow: 0 0 1px 2px -moz-mac-focusring, inset 0 0 1px 1px -moz-mac-focusring;"/>
  </hbox>
  
  </window>

Do you mean that the box-shadow made ring has the same tone as the "native" one for you?

Comment 13

[Stefan [:stefanh]]

Attachment: Screenshot of how it looks for me with todays nightly

Comment 14

[Stefan [:stefanh]]

Attachment: Screenshot of how it looks when using keyboardFocusIndicatorColor

Comment 15

[Karsten Düsterloh]

(In reply to comment #9)
> Interesting! Are you both on 10.6

Yes, 10.6.5 in my case.

> except if our inverse color correction doesn't work correctly...

Whatever that means. ;-) 
(I'm a foreigner in the country of graphics.)

(In reply to comment #11)
> Hrm... does playing with the gfx.color_management.mode setting make any
> difference?

No.

Comment 16

[Stefan [:stefanh]]

The problem with Karsten and me getting different colors is related to this report mainly because we found out that using -moz-mac-focusring as a box-shadow color doesn't produce the same color as the box-shadow color in, for example, a natively drawn textbox.

Comment 17

[Markus Stange [:mstange]]

> (In reply to comment #11)
> > Hrm... does playing with the gfx.color_management.mode setting make any
> > difference?
> 
> No.

Stefan, does it make a difference for you?

(In reply to comment #16)
> The problem with Karsten and me getting different colors is related to this
> report mainly because we found out that using -moz-mac-focusring as a
> box-shadow color doesn't produce the same color as the box-shadow color in, for
> example, a natively drawn textbox.

The fact that -moz-mac-focusring is different from the color of native focus rings isn't what's worrying me here. What's worrying me is that the same hardcoded RGB color results in different RGB values on screenshots made on different machines. I don't understand how this can happen.

Comment 18

[Stefan [:stefanh]]

(In reply to comment #17)
> Stefan, does it make a difference for you?

No it doesn't.

> The fact that -moz-mac-focusring is different from the color of native focus
> rings isn't what's worrying me here. What's worrying me is that the same
> hardcoded RGB color results in different RGB values on screenshots made on
> different machines. I don't understand how this can happen.

Yeah, I don't understand that either. But we should file a new bug for that unless you think the colors in the different textfields in my attached extension (without my patch) match (just focus the upper textfield and look at the lower one). That said, if you think they match, the issue with color variations between systems grow.

Comment 19

[Stefan [:stefanh]]

Fwiw, bug 403169 has a similar topic and some info. I made some comments there, but I never got a grip of the cause and if the css color issue still exists.

Comment 20

[Markus Stange [:mstange]]

Today bug 616232 surfaced, which suggests that hardware acceleration may make a difference here.

But to get back to the original issue, now that I've tested your extension and tried out the patch I agree that keyboardFocusIndicatorColor is definitely a whole lot better.
You'll need to make some changes to widget/tests/test_platform_colors.xul
(maybe skip -moz-mac-focusring entirely?) before we can take the patch.

Comment 21

[Stefan [:stefanh]]

(In reply to comment #20)
> Today bug 616232 surfaced, which suggests that hardware acceleration may make a
> difference here.
Ah, ok.

> But to get back to the original issue, now that I've tested your extension and
> tried out the patch I agree that keyboardFocusIndicatorColor is definitely a
> whole lot better.
> You'll need to make some changes to widget/tests/test_platform_colors.xul
> (maybe skip -moz-mac-focusring entirely?) before we can take the patch.

OK, I'll take a look and see what I can do ;-)

Comment 22

[Stefan [:stefanh]]

Hmm, how do I run that test manually?

Comment 23

[Markus Stange [:mstange]]

Hmm... I can get the test to start using
TEST_PATH=widget/tests/test_platform_colors.xul make mochitest-chrome
in the objdir, but it looks like it doesn't actually run...

Comment 24

[Stefan [:stefanh]]

(In reply to comment #23)
> Hmm... I can get the test to start using
> TEST_PATH=widget/tests/test_platform_colors.xul make mochitest-chrome
> in the objdir, but it looks like it doesn't actually run...

Heh, looks like it's because of this:
JavaScript error: chrome://mochitests/content/chrome/widget/tests/test_platform_colors.xul, line 46: unterminated string literal

Fixing that make the tests run. And then I get "failed | platform color menu is wrong: regb(255, 254, 254)"

Comment 25

[Stefan [:stefanh]]

I think we can keep the focusring test. The tests are a bit doubtful, but instead of just removing the focusring test, I rather fix it (and the menu test) and then we could discuss the tests somewhere else if we want.

I pushed a patch to try, just to ensure that the tests pass on Leopard.

Comment 26

[Stefan [:stefanh]]

Attachment: Patch committed to try

JFTR, here's the patch I commited to try

Comment 27

[Stefan [:stefanh]]

Interestingly, the 10.6.2 debug box seems to fail - the color returned seems to be rather different:
s: talos-r3-snow-005
15614 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_platform_colors.xul | platform color -moz-mac-focusring is wrong: rgb(93, 154, 216)

Comment 28

[Stefan [:stefanh]]

New push to try: http://hg.mozilla.org/try/rev/8ee692213e17

Comment 29

[Stefan [:stefanh]]

Hmm, so the leopard boxens returns rgb(75, 137, 208) and the Snow Leopard one (10.6.2) returns rgb(93, 154, 216). Could it be that the color depends on OS version? Looks like it.

Comment 30

[Stefan [:stefanh]]

(logs are at http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8ee692213e17/)

Comment 31

[Stefan [:stefanh]]

Attachment: V1.0

So, it seems the color varies between OS versions - I tested the Leopard try build and I get the same color as the test indicates. I've added the colors reported from the tests. No need to care about the graphite colors, I think.

We should possibly find a better way to test these colors (or maybe remove this particular test, but I suspect there are others that exhibit the same variations).

Ah, right - I fixed the test so it runs and I also added a color to the "menu" test, otherwise it fails on my system.

Comment 32

[Stefan [:stefanh]]

(In reply to comment #31)
> Created attachment 495896 [details] [diff] [review]
> V1.0
> 
> So, it seems the color varies between OS versions - I tested the Leopard try
> build and I get the same color as the test indicates. I've added the colors

I should also say that it looks like the nsNativeTheme drawn focus ring (textbox) differs from the 10.6.5 one.

Comment 33

[Markus Stange [:mstange]]

So, the color that's reported on 10.6 tryserver is different from the color reported on your machine, so the new test fails for you locally? That's not good, tests should pass locally on developers' machines, too. I think it would be better to just remove the -moz-mac-focusring test (but please keep the bugfix for "menu" :-) ).

Comment 34

[Markus Stange [:mstange]]

Answering to your comment on IRC, I still think we should drop the test. :-)
If somebody else runs the test locally on his machine, he may get *another* different color that we didn't account for (or he might be using graphite...).

Comment 35

[Stefan [:stefanh]]

Yeah, I guess you're right. I'll put up a new version tomorrow.

Comment 36

[Stefan [:stefanh]]

Attachment: Now with disabled test

I just commented out the test.

Comment 37

[Markus Stange [:mstange]]

Comment on attachment 496969 [details] [diff] [review]
Now with disabled test

Thanks!

Comment 38

[Stefan [:stefanh]]

Do I need any other review (ui-r or something)?

Comment 39

[Markus Stange [:mstange]]

Better ask josh for another rubber stamp review, just to be on the safe side. Obvious bug fixes like this don't need UI review.

Comment 40

[Stefan [:stefanh]]

Comment on attachment 496969 [details] [diff] [review]
Now with disabled test

(In reply to comment #39)
> Better ask josh for another rubber stamp review, just to be on the safe side.
> Obvious bug fixes like this don't need UI review.

OK, thanks!

Comment 41

[Stefan [:stefanh]]

Comment on attachment 496969 [details] [diff] [review]
Now with disabled test

This should be very safe. What we do here is changing our -moz-mac-focusring color to be more in line with the native color. We now also match webkit's -webkit-focus-ring-color.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 496969 [details] [diff] [review]
Now with disabled test

a=beltzner, nice polish work!

Comment 43

[Stefan [:stefanh]]

http://hg.mozilla.org/mozilla-central/rev/9b02aa0de19c