Closed Bug 120525 Opened 24 years ago Closed 21 years ago

Windows classic skin button focus outlines 1px too big

Categories

(SeaMonkey :: Themes, defect)

Product:
SeaMonkey
Component:
Themes
Platform:
x86
Windows 98
Type:
defect
Priority:
Not set
Severity:
trivial

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: ian, Assigned: mbockelkamp)

Details

Attachments

(1 file, 10 obsolete files)

Patch v4a
1.28 KB, patch
neil
: review+
jag+mozilla
: superreview+
asa
: approval1.8a2+
Details | Diff | Splinter Review
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.
-> shliang
Assignee: hewitt → shliang
Target Milestone: --- → Future
Taking.
Assignee: shliang → mbockelkamp
Attached image Screenshot (obsolete) —
First is a Windows button, second is a Mozilla button.
Attached patch Patch (obsolete) — — Splinter Review
Attachment #131277 - Flags: superreview?(jag)
Attachment #131277 - Flags: review?(jag)
Attachment #131277 - Flags: review?(jag) → review?(mkaply)
can you post a screenshot after your fix?
Attached image Screenshot (obsolete) —
From top to bottom: - Mozilla without the patch - Mozilla with the patch - Windows button
Attachment #131276 - Attachment is obsolete: true
Comment on attachment 131277 [details] [diff] [review] Patch r=mkaply any idea how easy it would be to round the corners like Windows?
Attachment #131277 - Flags: review?(mkaply) → review+
> any idea how easy it would be to round the corners like Windows? For that the behaviour of "dotted" had to be changed.
Comment on attachment 131277 [details] [diff] [review] Patch sr=jag
Attachment #131277 - Flags: superreview?(jag) → superreview+
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.
Additional padding is not necessary. Screenshot and Patch will follow.
Attached image Screenshot (obsolete) —
Left column: Windows buttons Right column: Mozilla buttons
Attachment #134913 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — — Splinter Review
Margin moved to button-box.
Attachment #131277 - Attachment is obsolete: true
Attachment #139668 - Flags: superreview?(jag)
Attachment #139668 - Flags: review?(neil.parkwaycc.co.uk)
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
Attached image Screenshot of v2 (obsolete) —
Same font and PNG.
Attachment #139667 - Attachment is obsolete: true
Attached patch Patch v2a (obsolete) — — Splinter Review
Better patch. Also corrects the buttons height. Neil: Would this cause regressions?
Attached image Screenshot of v2a (obsolete) —
Attachment #139668 - Flags: superreview?(jag)
Attachment #139685 - Attachment description: Screenshot → Screenshot of v2
Attachment #139687 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attached patch Patch v3 (obsolete) — — Splinter Review
Attachment #139668 - Attachment is obsolete: true
Attachment #139687 - Attachment is obsolete: true
Attachment #140987 - Flags: review?(neil.parkwaycc.co.uk)
(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.
Attachment #139687 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #139668 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #139685 - Attachment is obsolete: true
Attachment #139688 - Attachment is obsolete: true
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.
(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?
Must be another bug in the appearance code, then...
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.
Attachment #140987 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attachment #140987 - Attachment is obsolete: true
Attachment #140987 - Flags: review-
Attached patch Patch v4 (obsolete) — — Splinter Review
new attempt without changing the width of the buttons
Attachment #151539 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 151539 [details] [diff] [review] Patch v4 I just saw a case where it doesn't work as expected...
Attachment #151539 - Attachment is obsolete: true
Attachment #151539 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v4a — — Splinter Review
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
Attachment #151546 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #151546 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #151546 - Flags: superreview?(jag)
Comment on attachment 151546 [details] [diff] [review] Patch v4a I take it screenshot of v2a still applies? sr=jag
Attachment #151546 - Flags: superreview?(jag) → superreview+
(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?
Attachment #151546 - Flags: approval1.8a2?
Comment on attachment 151546 [details] [diff] [review] Patch v4a a=asa (on behalf of drivers) for checkin to 1.8a2
Attachment #151546 - Flags: approval1.8a2? → approval1.8a2+
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Can we fix this in toolkit/themes/winstripe/global/button.css as well?
Where can I see that file in action?
(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.
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: