1390846 - Application Menu Separator looks like it’s a gradient, should have hard edges (should be shorter / floating)

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Dão Gottwald [:dao]]

Mockup: http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1390846 - Make application menu button separator solid and equalize horizontal spacing.

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

Comment 2

[Johann Hofmann [:johannh]]

Comment on attachment 8897833 [details]
Bug 1390846 - Make application menu button separator solid and equalize horizontal spacing.

https://reviewboard.mozilla.org/r/169130/#review174532

Looks mostly good, but see concerns about the spec below.

::: browser/themes/shared/customizableui/panelUI.inc.css:98
(Diff revision 1)
>  }
>  
>  #PanelUI-button {
> -  margin-inline-start: 2px;
> +  margin-inline-start: 3px;
>    border-inline-start: 1px solid;
> -  border-image: linear-gradient(transparent, rgba(0,0,0,.1) 20%, rgba(0,0,0,.1) 80%, transparent);
> +  border-image: linear-gradient(transparent 4px, rgba(0,0,0,.1) 4px, rgba(0,0,0,.1) calc(100% - 4px), transparent calc(100% - 4px));

I believe the spec says your 4px is supposed to be 6px https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647

Comment 3

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> Comment on attachment 8897833 [details]
> Bug 1390846 - Make application menu button separator solid and equalize
> horizontal spacing.
> 
> https://reviewboard.mozilla.org/r/169130/#review174532
> 
> Looks mostly good, but see concerns about the spec below.
> 
> ::: browser/themes/shared/customizableui/panelUI.inc.css:98
> (Diff revision 1)
> >  }
> >  
> >  #PanelUI-button {
> > -  margin-inline-start: 2px;
> > +  margin-inline-start: 3px;
> >    border-inline-start: 1px solid;
> > -  border-image: linear-gradient(transparent, rgba(0,0,0,.1) 20%, rgba(0,0,0,.1) 80%, transparent);
> > +  border-image: linear-gradient(transparent 4px, rgba(0,0,0,.1) 4px, rgba(0,0,0,.1) calc(100% - 4px), transparent calc(100% - 4px));
> 
> I believe the spec says your 4px is supposed to be 6px
> https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647

That's about horizontal spacing, as I read it. I took the 4px directly from the CSS used in <http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html>.

Comment 4

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #3)
> (In reply to Johann Hofmann [:johannh] from comment #2)
> > Comment on attachment 8897833 [details]
> > Bug 1390846 - Make application menu button separator solid and equalize
> > horizontal spacing.
> > 
> > https://reviewboard.mozilla.org/r/169130/#review174532
> > 
> > Looks mostly good, but see concerns about the spec below.
> > 
> > ::: browser/themes/shared/customizableui/panelUI.inc.css:98
> > (Diff revision 1)
> > >  }
> > >  
> > >  #PanelUI-button {
> > > -  margin-inline-start: 2px;
> > > +  margin-inline-start: 3px;
> > >    border-inline-start: 1px solid;
> > > -  border-image: linear-gradient(transparent, rgba(0,0,0,.1) 20%, rgba(0,0,0,.1) 80%, transparent);
> > > +  border-image: linear-gradient(transparent 4px, rgba(0,0,0,.1) 4px, rgba(0,0,0,.1) calc(100% - 4px), transparent calc(100% - 4px));
> > 
> > I believe the spec says your 4px is supposed to be 6px
> > https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647
> 
> That's about horizontal spacing, as I read it. I took the 4px directly from
> the CSS used in
> <http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html>.

On the very left hand side of the spec there's a 6px indicator for vertical padding. But you're right, that mockup indeed uses 4px. Hm.

Comment 5

[Dão Gottwald [:dao]]

> > > I believe the spec says your 4px is supposed to be 6px
> > > https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647
> > 
> > That's about horizontal spacing, as I read it. I took the 4px directly from
> > the CSS used in
> > <http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html>.
> 
> On the very left hand side of the spec there's a 6px indicator for vertical
> padding.

That's about the buttons' hover state. This separator isn't supposed to have the same height.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #5)
> > > > I believe the spec says your 4px is supposed to be 6px
> > > > https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647
> > > 
> > > That's about horizontal spacing, as I read it. I took the 4px directly from
> > > the CSS used in
> > > <http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html>.
> > 
> > On the very left hand side of the spec there's a 6px indicator for vertical
> > padding.
> 
> That's about the buttons' hover state. This separator isn't supposed to have
> the same height.

Makes sense. Land ahead!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09b6a5ec8d7c
Make application menu button separator solid and equalize horizontal spacing. r=johannh

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/09b6a5ec8d7c

Comment 9

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=833f84d0d5c729054a3aa8b3f34735f56fe6436b&newProject=mozilla-central&newRev=1d38626ba9686d119e489db538ce84f8f9854217

Comment 10

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170920220431

I can reproduce this issue on Firefox Nightly Build ID: 20170801100311 on Windows 8.1 x64.

This issue has been verified on latest Firefox Nightly Build ID: 20170920220431 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and I cannot reproduce it. Now, the Application Menu Separator has hard edges.