1405755 - [macOS] [Photon] Light Theme: Tab Strip overflow divider is white instead of grey

Status: NEW

(Firefox :: Theme, defect, P3)

Description

[Mehmet]

Attachment: actual_left.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3232.0 Safari/537.36

Steps to reproduce:

macOS 10.12.6
58.0a1 (2017-10-04) (64-Bit)

1.) Select the Light theme
2.) Open a lot of tabs


Actual results:

The overflow divider is actual white.


Expected results:

It should be grey like in the mock from https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229786513

Comment 1

[Mehmet]

Attachment: actual_right.png

Comment 2

[Mehmet]

Attachment: mock_left.png

Comment 3

[Mehmet]

Attachment: mock_right.png

Comment 4

[Dale Harvey (:daleharvey)]

Attachment: Bug 1405755 - Move overflow border to button.

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

Comment 5

[Dale Harvey (:daleharvey)]

Comment on attachment 8918421 [details]
Bug 1405755 - Move overflow border to button.

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

Comment 6

[Dale Harvey (:daleharvey)]

So the tab borders here use before/after psuedo elements here, with the currentColor and an opacity of .3, I would prefer to use the same but the border comes from a xul:spacer here so matching the tabs means changing a bunch of athe structure to give the xul:spacer psuedo selectors. 

Happy to do that, but figured I would check if there was a simpler / better solution first, this one seems mostly fine although surprised a suitable colour setting that I could find to use here, I may have missed one

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8918421 [details]
Bug 1405755 - Move overflow border to button.

https://reviewboard.mozilla.org/r/189248/#review195830

So, this border was really just supposed to add contrast to the shadow (it actually used to be part of tab-overflow-indicator.png). If we want this to be a proper border like the tab separators (I'm not sure we do), then shouldn't we make it part of the scroll buttons rather than the overflow indicators?

::: browser/themes/shared/tabs.inc.css:393
(Diff revision 2)
>    background-repeat: no-repeat;
>    border-left: 1px solid;
> -  border-image: linear-gradient(rgba(255,255,255,.2),
> -                                rgba(255,255,255,.2) calc(100% - 1px - var(--tab-toolbar-navbar-overlap)),
> -                                transparent calc(100% - 1px - var(--tab-toolbar-navbar-overlap)));
> +  border-image: linear-gradient(transparent calc(1px + var(--tabs-top-border-width)),
> +                                var(--tab-overflow-border) calc(1px + var(--tabs-top-border-width)),
> +                                var(--tab-overflow-border) calc(100% - 1px),
> +                                transparent calc(100% - 1px));

I don't understand why you're using var(--tabs-top-border-width) here, and why you removed var(--tab-toolbar-navbar-overlap).

Comment 8

[Dale Harvey (:daleharvey)]

> So, this border was really just supposed to add contrast to the shadow (it actually used 
> to be part of tab-overflow-indicator.png). If we want this to be a proper border like 
> the tab separators (I'm not sure we do), then shouldn't we make it part of the scroll 
> buttons rather than the overflow indicators?

It seems like it already is a 'proper border', it is the border between the scroll button and the tab strip right now (which is in the spec). It is only visible on scroll however which is not to spec and moving it to the scroll buttons would fix that, however we would need to fix the tab seperators collapsing with this border so we dont get double borders, this somehow already does that

> I don't understand why you're using var(--tabs-top-border-width) here, and why you 
> removed var(--tab-toolbar-navbar-overlap).

I wanted to give it the same border as was given to the tabs @ https://searchfox.org/mozilla-central/source/browser/themes/shared/tabs.inc.css#580, that has since been changed so will update to match

Moving these borders to the actual scrollbutton seems like the most correct thing here?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Dale Harvey (:daleharvey) PTO till 23rd from comment #8)
> > So, this border was really just supposed to add contrast to the shadow (it actually used 
> > to be part of tab-overflow-indicator.png). If we want this to be a proper border like 
> > the tab separators (I'm not sure we do), then shouldn't we make it part of the scroll 
> > buttons rather than the overflow indicators?
> 
> It seems like it already is a 'proper border', it is the border between the
> scroll button and the tab strip right now (which is in the spec). It is only
> visible on scroll however which is not to spec and moving it to the scroll
> buttons would fix that, however we would need to fix the tab seperators
> collapsing with this border so we dont get double borders, this somehow
> already does that

[...]

> Moving these borders to the actual scrollbutton seems like the most correct
> thing here?

Yeah, that's what I was thinking.

Comment 10

[Dale Harvey (:daleharvey)]

Hey Dao

I am a little stuck here since xul doesnt behave like flexbox, not sure what the trick is for this type of thing. I have added an ::after element to the scollbuttons for the border (so they can match the color of the other tab borders / seperators and we dont get bugs like this in future)

However I need |-moz-boz-align: stretch| to make sure the borders go full height, but that forces the xul:image to go full height and I cant seem to prevent that

Patch is @ https://pastebin.mozilla.org/9071829 and https://i.imgur.com/eRBDmPL.png is what it looks like

Cheers

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 8918421 [details]
Bug 1405755 - Move overflow border to button.

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

Comment 12

[Dale Harvey (:daleharvey)]

Clearing needinfo since its in for review

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8918421 [details]
Bug 1405755 - Move overflow border to button.

https://reviewboard.mozilla.org/r/189248/#review201472

::: browser/themes/shared/tabs.inc.css:644
(Diff revision 3)
> +  -moz-box-align: stretch;
> +  padding-inline-end: 1px;
> +}
> +
> +.tabbrowser-arrowscrollbox > .scrollbutton-up .toolbarbutton-icon,
> +.tabbrowser-arrowscrollbox > .scrollbutton-down .toolbarbutton-icon {

Please use the child selector.

::: browser/themes/shared/tabs.inc.css:652
(Diff revision 3)
> +  height: 16px;
> +  background: center center url(chrome://browser/skin/arrow-left.svg) no-repeat;
> +}
> +
> +.tabbrowser-arrowscrollbox > .scrollbutton-up::after,
> +.tabbrowser-arrowscrollbox > .scrollbutton-down::after {

With the right-scrolling arrow having a border, should we remove the last tab's right border on overflow?

::: browser/themes/shared/tabs.inc.css:658
(Diff revision 3)
> +  content: "";
>    color: inherit;
> +  border-inline-end: 1px solid;
> +  display: -moz-box;
> +  opacity: .3;
> +  width: var(--toolbarbutton-inner-padding);

Why are you setting a width here and why are you using --toolbarbutton-inner-padding for that? This doesn't seem to make any sense.

::: browser/themes/shared/tabs.inc.css:663
(Diff revision 3)
> +  width: var(--toolbarbutton-inner-padding);
>  }
>  
>  .tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl),
>  .tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr) {
>    transform: scaleX(-1);

This ends up moving the border to the wrong side in rtl.

Comment 14

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #13)
> ::: browser/themes/shared/tabs.inc.css:652
> (Diff revision 3)
> > +  height: 16px;
> > +  background: center center url(chrome://browser/skin/arrow-left.svg) no-repeat;
> > +}
> > +
> > +.tabbrowser-arrowscrollbox > .scrollbutton-up::after,
> > +.tabbrowser-arrowscrollbox > .scrollbutton-down::after {
> 
> With the right-scrolling arrow having a border, should we remove the last
> tab's right border on overflow?

Please check carefully that this will not lead to resizing issues (I can't imagine it won't).

Comment 15

[Dale Harvey (:daleharvey)]

Comment on attachment 8918421 [details]
Bug 1405755 - Move overflow border to button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189248/diff/3-4/

Comment 16

[Dale Harvey (:daleharvey)]

> With the right-scrolling arrow having a border, should we remove the last tab's right border on overflow?

The right-scrolling arrow already has a border so not sure why we would change that in this patch?

> Why are you setting a width here and why are you using --toolbarbutton-inner-padding 
> for that? This doesn't seem to make any sense.

This button used to have --toolbarbutton-inner-padding but with the border being inside it instead of an the adjacent element that doesnt make sense, giving the inner toolbar-icon the margin: var( --toolbarbutton-inner-padding) gives the intended size and is a little cleaner / more obvious I think?

Fixed the RTL issue and added the child selector, cheers

Comment 17

[Dão Gottwald [:dao]]

(In reply to Dale Harvey (:daleharvey) from comment #16)
> > With the right-scrolling arrow having a border, should we remove the last tab's right border on overflow?
> 
> The right-scrolling arrow already has a border so not sure why we would
> change that in this patch?

Because that pseudo-border is currently part of the overflow shadow and disappears when the tab bar is scrolled to the end. With your patch it won't disappear in that case anymore.

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 8918421 [details]
Bug 1405755 - Move overflow border to button.

Ah I didnt realise the overflow shadow completely dissapeared, just thought it was less visible. Will clear review and update the patch to get rid of that border, but if you have any other feedback be happy to hear

Comment 19

[Dale Harvey (:daleharvey)]

Comment on attachment 8918421 [details]
Bug 1405755 - Move overflow border to button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189248/diff/4-5/

Comment 20

[Dale Harvey (:daleharvey)]

Unassigning for parental leave

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.