1452303 - App menu separators regressed with bug 1451711

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[Tim Nguyen :ntim]

Attachment: Screenshot

Bug 1451711 made that the separator looks too light on mac.

Comment 1

[Tim Nguyen :ntim]

Dão, how would you recommend fixing this ? Basically, since --panel-separator-color has an alpha channel, the color is now relative to the new ThreeDShadow background as opposed to the old semi-transparent background.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #1)
> Dão, how would you recommend fixing this ? Basically, since
> --panel-separator-color has an alpha channel, the color is now relative to
> the new ThreeDShadow background as opposed to the old semi-transparent
> background.

We should stop using background here and instead use border all the time so that --panel-separator-color completely overrides ThreeDShadow.

Comment 3

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #2)
> (In reply to Tim Nguyen :ntim from comment #1)
> > Dão, how would you recommend fixing this ? Basically, since
> > --panel-separator-color has an alpha channel, the color is now relative to
> > the new ThreeDShadow background as opposed to the old semi-transparent
> > background.
> 
> We should stop using background here and instead use border all the time so
> that --panel-separator-color completely overrides ThreeDShadow.

Which border(s) should be set ? There are vertical/horizontal separators depending on where toolbarseparator is used.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #3)
> (In reply to Dão Gottwald [::dao] from comment #2)
> > (In reply to Tim Nguyen :ntim from comment #1)
> > > Dão, how would you recommend fixing this ? Basically, since
> > > --panel-separator-color has an alpha channel, the color is now relative to
> > > the new ThreeDShadow background as opposed to the old semi-transparent
> > > background.
> > 
> > We should stop using background here and instead use border all the time so
> > that --panel-separator-color completely overrides ThreeDShadow.
> 
> Which border(s) should be set ? There are vertical/horizontal separators
> depending on where toolbarseparator is used.

Both a vertical and a horizontal border?

Comment 5

[Tim Nguyen :ntim]

Attachment: -moz-appearance: separator on macOS

Looks like macOS also supports -moz-appearance: separator, meaning that we can simply be consistent with Windows/Linux.

Comment 6

[Tim Nguyen :ntim]

Attachment: Bug 1452303 - Use -moz-appearance:separator instead of background for toolbar separators.

Review commit: https://reviewboard.mozilla.org/r/235466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235466/

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8966789 [details]
Bug 1452303 - Use -moz-appearance:separator instead of background for toolbar separators.

https://reviewboard.mozilla.org/r/235466/#review241288

::: toolkit/themes/osx/global/toolbar.css:28
(Diff revision 1)
>  toolbarseparator {
> -  -moz-appearance: none;
> +  -moz-appearance: separator;
>    margin: 3px 4px;
> -  background-color: ThreeDShadow;
> -  padding: 0;
>    width: 1px !important;

Can width be dropped?

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8966789 [details]
Bug 1452303 - Use -moz-appearance:separator instead of background for toolbar separators.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235466/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/485b7f8c9d9a
Use -moz-appearance:separator instead of background for toolbar separators. r=dao

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/485b7f8c9d9a