373696 - style toolbar separators natively

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

I'll submit some screenshots later.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Firefox before/after/reference screenshots

I was actually about to remove the horizontal borders entirely in my personal setup, as they hardly fulfil a practical function. I found them visually distracting. Then I noticed that the Explorer is using a ThreeDLightShadow-like color, and I think that should be the default appearance for Moz apps under WinXP / Luna. It's probably also the right thing for Vista; I'm not sure about Windows Classic.

Comment 2

[Dão Gottwald [:dao]]

Attachment: trunk layout/rendering bug

There's a corrupted line in the toolbar right above the tab bar. That's not really related to this change, though it becomes more obvious.
I was unable to find its origin with DOMi. Maybe it's a core bug -- I'm thinking of bug 371434.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Thunderbird 2 screenshots

(In reply to comment #2)
> Created an attachment (id=258373) [details]
> trunk layout/rendering bug

This applies to Thunderbird as well.

Comment 4

[Dão Gottwald [:dao]]

Requesting blocking in order to get this on the radar. I'm fine with a WONTFIX, but somebody should actually make that decision.

Comment 5

[Dão Gottwald [:dao]]

I think this was fixed by bug 337771.

Comment 6

[Dão Gottwald [:dao]]

Ah, not entirely fixed. Although toolbar shadows use the light shadow, toolbarseparators still use the dark one.

Comment 7

[Simon Bünzli]

Attachment: style toolbar separators natively

Pretty simple patch, except that there's another magic number creeping in (the default code path in GetMinimumWidgetSize somehow returns either too large or too small a value for what we need to look right) and that the classic CSS version has to use overridable borders instead of margins (those come included with the natively themed separator).

Comment 8

[Dão Gottwald [:dao]]

updating summary to what this bug is actually about now.

Comment 9

[[e-mail]]

Comment on attachment 273238 [details] [diff] [review]
style toolbar separators natively

> toolbarseparator {
>-  margin : 2px 0.2em;
>-  border-right : 1px solid ThreeDHighlight;
>-  border-left : 1px solid ThreeDShadow;
>-  width : 2px;
>+  -moz-appearance: separator;
>+  border-top: 2px solid transparent;
>+  border-bottom: 2px solid transparent;
>+  border-left: 3px solid transparent;
>+  border-right: 3px solid transparent;
>+  -moz-border-left-colors  : transparent transparent ThreeDShadow;
>+  -moz-border-right-colors : transparent transparent ThreeDHighlight;
> }
I'm assuming here that the way the theming works is that you're not doing anything on Windows 2000 or XP Classic and so this CSS takes effect instead.

Comment 10

[Ere Maijala (slow)]

Comment on attachment 273238 [details] [diff] [review]
style toolbar separators natively

r=emaijala

Comment 11

[Nickolay_Ponomarev]

Attachment: updated to trunk

Comment 12

[Nickolay_Ponomarev]

mozilla/widget/src/windows/nsNativeThemeWin.cpp      3.92
mozilla/toolkit/themes/winstripe/global/toolbar.css  1.16

Comment 13

[aja+bugzilla]

FWIW, the resizer (between location bar and search) look bad since this landed.
I assume this patch may have predated addition of the resizer?

Comment 14

[Simon Bünzli]

(In reply to comment #13)
> FWIW, the resizer (between location bar and search) look bad since this landed.
It really shouldn't look any _worse_ than before. Anyway, should bug 391128 not take care of your issue, please just file another polish bug.

Comment 15

[atzaus]

After this patch landed I have no toolbar separators at all with non MS themes. And with MS themes the separators are not padded at the bottom.

Comment 16

[atzaus]

Attachment: No separators

Comment 17

[Simon Bünzli]

Ali: Please file a follow up bug and make sure to include screenshots from Windows Explorer(!) for comparison (separators are indeed expected to look differently than before this bug, so there's no need to add screenshots from older Minefield builds).

Comment 18

[Simon Bünzli]

> Ali: Please file a follow up bug
Never mind - that's bug 394142.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 (.NET CLR 3.5.30729) ID:2009042316