120525 - Windows classic skin button focus outlines 1px too big

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Hixie (not reading bugmail)]

The focus ring on buttons in the Classic skin on Windows are 1px too large
compared to native buttons. The focus ring is otherwise correct. The result is
that a focused button that isn't in the :active state looks like it has a double
thick border, when it should look like an outset border followed by a gap
followed by a focus ring.

This should be a pretty easy fix.

Comment 1

[Joe Hewitt (gone)]

-> shliang

Comment 2

[Matthias Bockelkamp]

Taking.

Comment 3

[Matthias Bockelkamp]

Attachment: Screenshot

First is a Windows button, second is a Mozilla button.

Comment 4

[Matthias Bockelkamp]

Attachment: Patch

Comment 5

[Mike Kaply [:mkaply]]

can you post a screenshot after your fix?

Comment 6

[Matthias Bockelkamp]

Attachment: Screenshot

From top to bottom:
- Mozilla without the patch
- Mozilla with the patch
- Windows button

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 131277 [details] [diff] [review]
Patch

r=mkaply

any idea how easy it would be to round the corners like Windows?

Comment 8

[Matthias Bockelkamp]

> any idea how easy it would be to round the corners like Windows?

For that the behaviour of "dotted" had to be changed.

Comment 9

[jag (Peter Annema)]

Comment on attachment 131277 [details] [diff] [review]
Patch

sr=jag

Comment 10

[[e-mail]]

Assuming that the 1px margin is correct on a focussed button, can you please put
the margin on the .button-box and adjust all the padding appropriately for
normal, active and disabled but not focus. Otherwise the styles will conflict.

Comment 11

[Matthias Bockelkamp]

Additional padding is not necessary. Screenshot and Patch will follow.

Comment 12

[Matthias Bockelkamp]

Attachment: Screenshot

Left column: Windows buttons
Right column: Mozilla buttons

Comment 13

[Matthias Bockelkamp]

Attachment: Patch v2

Margin moved to button-box.

Comment 14

[[e-mail]]

Comment on attachment 139668 [details] [diff] [review]
Patch v2

I like this patch :-) but it would be nice to see the comparison using the same
font, and in a .gif or .png instead of a .jpeg :-P

Comment 15

[Matthias Bockelkamp]

Attachment: Screenshot of v2

Same font and PNG.

Comment 16

[Matthias Bockelkamp]

Attachment: Patch v2a

Better patch. Also corrects the buttons height. 
Neil: Would this cause regressions?

Comment 17

[Matthias Bockelkamp]

Attachment: Screenshot of v2a

Comment 18

[[e-mail]]

Comment on attachment 139687 [details] [diff] [review]
Patch v2a

Spookily similar, but I don't really like you using a negative margin, I'd
prefer it if you reduced the padding instead (yes, I know that means a bigger
diff because the .button-box has several styles that set the padding). Also
unless it's a -moz-appearance bug it would be neat if you figured out the
correct outer border colour too.

Comment 19

[Matthias Bockelkamp]

Attachment: Patch v3

Comment 20

[Matthias Bockelkamp]

(In reply to comment #18)
> diff because the .button-box has several styles that set the padding). Also
> unless it's a -moz-appearance bug it would be neat if you figured out the
> correct outer border colour too.

What do you mean? On my PC all colours are correct.

Comment 21

[[e-mail]]

Comment on attachment 139688 [details]
Screenshot of v2a

The border of the bottom left button is black but that of the bottom right
button is dark grey.

Comment 22

[Matthias Bockelkamp]

(In reply to comment #21)
> The border of the bottom left button is black but that of the bottom right
> button is dark grey.

OK, but changes to the colours will only take effect if I remove the
"-moz-appearance: button;". Any ideas?

Comment 23

[[e-mail]]

Must be another bug in the appearance code, then...

Comment 24

[[e-mail]]

Comment on attachment 140987 [details] [diff] [review]
Patch v3

It was only the *negative* margin that I asked you to remove... if you need the
1px margin for the focus effect then have it as it was in the last patch.

Comment 25

[Matthias Bockelkamp]

Attachment: Patch v4

new attempt without changing the width of the buttons

Comment 26

[Matthias Bockelkamp]

Comment on attachment 151539 [details] [diff] [review]
Patch v4

I just saw a case where it doesn't work as expected...

Comment 27

[Matthias Bockelkamp]

Attachment: Patch v4a

fixed new attempt

- removed negative margin by changing paddings
- decreased the button height by one (which is the default height of a windows
button)
- _no_ correction of the button width

Comment 28

[[e-mail]]

Comment on attachment 151546 [details] [diff] [review]
Patch v4a

It's certainly an improvement although I get the impression that classic
buttons are still too big.

Comment 29

[jag (Peter Annema)]

Comment on attachment 151546 [details] [diff] [review]
Patch v4a

I take it screenshot of v2a still applies? sr=jag

Comment 30

[Matthias Bockelkamp]

(In reply to comment #29)
> (From update of attachment 151546 [details] [diff] [review])
> I take it screenshot of v2a still applies? sr=jag

The size of the buttons is slightly smaller now.
Do you want an updated screenshot?

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 151546 [details] [diff] [review]
Patch v4a

a=asa (on behalf of drivers) for checkin to 1.8a2

Comment 32

[[e-mail]]

Fix checked in.

Comment 33

[:deanis74]

Can we fix this in toolkit/themes/winstripe/global/button.css as well?

Comment 34

[Matthias Bockelkamp]

Where can I see that file in action?

Comment 35

[:deanis74]

(In reply to comment #34)
> Where can I see that file in action?

It's the FireFox theme.  I filed bug 251233 for porting the fix.