Remove border between tab strip and the navigation toolbar on Windows 10

VERIFIED FIXED in Firefox 40

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: dao, Assigned: jaws)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 42
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox39 disabled, firefox40 verified, firefox41 verified, firefox42 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
We'll need updated tab images for this.
(Reporter)

Updated

3 years ago
Flags: needinfo?(shorlander)

Comment 1

3 years ago
FYI, the tab stroke can be removed without any new images.
This CSS works for me : 
.tab-background-end::after, .tab-background-start::after {
	content: none !important;
}
It seems like ::after is used for the border/stroke, and ::before is used for the background shape.

I may submit a patch sometime this weekend if I have some time.

Updated

3 years ago
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
Priority: -- → P1
(Reporter)

Updated

3 years ago
Depends on: 1173725
Leaving as P1 as one of the few remaining important theme update bits.

Comment 3

3 years ago
Tim, are you actively working on this? What's still missing, can we help, and/or should someone else take over? Our window for landing is quite small if we want this in Firefox 40.
Flags: needinfo?(ntim.bugs)

Comment 4

3 years ago
(In reply to :Gijs Kruitbosch from comment #3)
> Tim, are you actively working on this? What's still missing, can we help,
> and/or should someone else take over? Our window for landing is quite small
> if we want this in Firefox 40.

I'm waiting for bug 1173725 to land first.
Flags: needinfo?(ntim.bugs)

Comment 5

3 years ago
(In reply to Tim Nguyen [:ntim] (away 11-14 July) from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Tim, are you actively working on this? What's still missing, can we help,
> > and/or should someone else take over? Our window for landing is quite small
> > if we want this in Firefox 40.
> 
> I'm waiting for bug 1173725 to land first.

While we need that bug landed before this can land, it'd be useful if we could iterate on the patch here beforehand, because of the tight scheduling... For all I know it's going to be a week before everything lines up in that bug. :-\

Comment 6

3 years ago
I won't be able to make a patch until Tuesday, but someone wants to take this, here's what I had planned :
- Use the css in comment 1
- remove the tab-background-middle background tab stroke (The simplest way would probably consist in adjusting the background-size property (set the size to 0 on the wanted bg)
- remove top border from navbar
Flags: qe-verify+
Hey, I think we'll need new graphics that don't have the stroke if we want to do this right.

Simply removing the stroke makes the bottom part of the curve too abrupt. Here's a screenshot: http://screencast.com/t/nXJJ8apwSmTr

Is it possible to get some new tab graphics while Stephen is out on PTO?
Flags: needinfo?(philipp)

Comment 8

3 years ago
Heh, I'm glad both of us were looking at this. I think you should probably just take it, though, Jared (I would, but seeing as I'm going to be gone soon...)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Hey, I think we'll need new graphics that don't have the stroke if we want
> to do this right.

Hmm. But we use the clip-path with a background to do this; wouldn't we just want a different clip-path? Note also that the design specs a change in tab-bar height that I think we should go without for 40 because complexity. But yeah.

> Simply removing the stroke makes the bottom part of the curve too abrupt.
> Here's a screenshot: http://screencast.com/t/nXJJ8apwSmTr

FWIW, I didn't notice this so much when playing around with it on my machine, but that might be because I made the border transparent (because of the 1px overlap, otherwise the navbar is cutting off the tabs)? Maybe that's the difference?

> Is it possible to get some new tab graphics while Stephen is out on PTO?

The other thing is that we need a solution for hovered tabs. They're using background images right now, so we need different images for those, or some kind of hacky solution for the SVG.
Flags: needinfo?(jaws)

Comment 9

3 years ago
(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #8)
> FWIW, I didn't notice this so much when playing around with it on my
> machine, but that might be because I made the border transparent (because of
> the 1px overlap, otherwise the navbar is cutting off the tabs)? Maybe that's
> the difference?

Obviously, alternatively, get rid of the now useless overlap, but that was my WIP patch and then I hit the hovered tabs.
(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #8)
> (because of
> the 1px overlap, otherwise the navbar is cutting off the tabs)? Maybe that's
> the difference?

Ah, yes, I needed to remove the margin-top:-1px. Thanks!
Assignee: ntim.bugs → jaws
Flags: needinfo?(jaws)
Created attachment 8635359 [details] [diff] [review]
Patch

This patch leaves the stroke on the background tabs/hovered tabs. We'll need new graphics for those, as the stroke is built in to the graphic.

I don't think it looks particularly bad with the stroke on the background tabs, and would be fine removing the stroke in a follow-up patch, even if that follow-up doesn't make it in Firefox 40.
Attachment #8635359 - Flags: review?(dao)
(Reporter)

Comment 12

2 years ago
Comment on attachment 8635359 [details] [diff] [review]
Patch

>+        .tab-background-end::after,
>+        .tab-background-start::after {
>+          content: none !important;
>+        }

nit: I'd prefer display: none

>+        .tab-background-middle[visuallyselected=true] {
>+          background-size: 0 0, auto 100%, auto 100%;
>+        }

Please document what exactly you're doing here

>+        #nav-bar {
>+          border-top-width: 0 !important;
>+        }

I think we may still want this border for popups. Not sure. In any case, can you merge this logic into the existing rules dealing with that border? http://hg.mozilla.org/mozilla-central/annotate/15155971639c/browser/themes/windows/browser.css#l313

>+        #TabsToolbar:not([collapsed="true"]) + #nav-bar {
>+          margin-top: 0px;
>+        }

I believe setting --tab-toolbar-navbar-overlap to 0 is the right way to do what we want here
Attachment #8635359 - Flags: review?(dao) → review-

Comment 13

2 years ago
Comment on attachment 8635359 [details] [diff] [review]
Patch

Review of attachment 8635359 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/browser.css
@@ +2045,5 @@
> +    @media not all and (-moz-os-version: windows-win7) {
> +      @media not all and (-moz-os-version: windows-win8) {
> +        .tab-background-end::after,
> +        .tab-background-start::after {
> +          content: none !important;

Does it work without !important ?

Comment 14

2 years ago
(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 8635359 [details] [diff] [review]
> Patch
> 
> >+        .tab-background-end::after,
> >+        .tab-background-start::after {
> >+          content: none !important;
> >+        }
> 
> nit: I'd prefer display: none
Even though display: none; is more explicit, it keeps the ::after node in the anonymous tree, while content: none; removes it, which is better for performance.
Created attachment 8635452 [details] [diff] [review]
Patch

I wasn't too sure what you meant by merging it with the rules at line 313, but I believe I did what you were asking for.

We don't need !important on the contnet:none; property, I just needed to make the selector more specific.
Attachment #8635359 - Attachment is obsolete: true
Attachment #8635452 - Flags: review?(dao)
Also, I tested this with popups on Windows 10 and it doesn't look bad without the border since the navbar color matches the window frame color.

Comment 17

2 years ago
Comment on attachment 8635452 [details] [diff] [review]
Patch

Review of attachment 8635452 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/browser.css
@@ +2063,5 @@
> +          background-size: 0 0, auto 100%, auto 100%;
> +        }
> +
> +        :root {
> +          --tab-toolbar-navbar-overlap: 0px;

nit : 0 instead of 0px
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Hey, I think we'll need new graphics that don't have the stroke if we want
> to do this right.
> 
> Simply removing the stroke makes the bottom part of the curve too abrupt.
> Here's a screenshot: http://screencast.com/t/nXJJ8apwSmTr
> 
> Is it possible to get some new tab graphics while Stephen is out on PTO?

Sorry Jared, I've been on PTO on Friday as well.
Redirecting this to Stephen since he should be back now as well.
Flags: needinfo?(philipp) → needinfo?(shorlander)
(Reporter)

Comment 19

2 years ago
Comment on attachment 8635452 [details] [diff] [review]
Patch

>+@media (-moz-os-version: windows-xp),
>+       (-moz-os-version: windows-vista),
>+       (-moz-os-version: windows-win7),
>+       (-moz-os-version: windows-win8) {
>+  #nav-bar {
>+    border-top: 1px solid @toolbarShadowColor@ !important;
>+  }
>+}
>+
> #nav-bar {
>-  border-top: 1px solid @toolbarShadowColor@ !important;
>   background-clip: padding-box;
>   background-image: linear-gradient(@toolbarHighlight@, transparent);
>   box-shadow: 0 1px 0 @toolbarHighlight@ inset;
> }
> 
> @media not all and (-moz-windows-compositor) {
>   #TabsToolbar[collapsed="true"] + #nav-bar {
>     border-top-style: none !important;

please restructure as follows:

#nav-bar {
  border-top: 1px solid @toolbarShadowColor@ !important;
  background-clip: padding-box;
  background-image: linear-gradient(@toolbarHighlight@, transparent);
  box-shadow: 0 1px 0 @toolbarHighlight@ inset;
}

@media (-moz-os-version: windows-xp),
       (-moz-os-version: windows-vista),
       (-moz-os-version: windows-win7),
       (-moz-os-version: windows-win8) {
  #nav-bar {
    border-top: 1px solid @toolbarShadowColor@ !important;
  }
  @media not all and (-moz-windows-compositor) {
    #TabsToolbar[collapsed="true"] + #nav-bar {
      border-top-style: none !important;
    }
  }
}

>+          --tab-toolbar-navbar-overlap: 0px;

0 doesn't need a unit as noted earlier
Attachment #8635452 - Flags: review?(dao) → review+
Why do you have the border-top rule duplicated in the above restructure? It's duplicated for both the #nav-bar rule and the @media-queried #nav-bar rule.
Flags: needinfo?(dao)
(Reporter)

Comment 21

2 years ago
Sorry, copy-paste-failure ... the border should only appear in the media query, i.e.:

#nav-bar {
  background-clip: padding-box;
  background-image: linear-gradient(@toolbarHighlight@, transparent);
  box-shadow: 0 1px 0 @toolbarHighlight@ inset;
}

@media (-moz-os-version: windows-xp),
       (-moz-os-version: windows-vista),
       (-moz-os-version: windows-win7),
       (-moz-os-version: windows-win8) {
  #nav-bar {
    border-top: 1px solid @toolbarShadowColor@ !important;
  }
  @media not all and (-moz-windows-compositor) {
    #TabsToolbar[collapsed="true"] + #nav-bar {
      border-top-style: none !important;
    }
  }
}
Flags: needinfo?(dao)

Comment 22

2 years ago
That navbar box-shadow should also be removed, it looks really weird when a lw-theme is enabled.
(In reply to Tim Nguyen [:ntim] from comment #22)
> That navbar box-shadow should also be removed, it looks really weird when a
> lw-theme is enabled.

Can you please file a separate bug for this?
Flags: needinfo?(ntim.bugs)

Comment 24

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/67262fc52141
Comment on attachment 8635452 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Windows 10 UI theme adjustments
[User impact if declined]: Firefox on Windows 10 won't match native/desired look-and-feel
[Describe test coverage new/current, TreeHerder]: local testing, landed on fx-team
[Risks and why]: low risk, css only
[String/UUID change made/needed]: none
Attachment #8635452 - Flags: approval-mozilla-beta?
Attachment #8635452 - Flags: approval-mozilla-aurora?

Comment 26

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> (In reply to Tim Nguyen [:ntim] from comment #22)
> > That navbar box-shadow should also be removed, it looks really weird when a
> > lw-theme is enabled.
> 
> Can you please file a separate bug for this?

Filed bug 1186244
Flags: needinfo?(ntim.bugs)
https://hg.mozilla.org/mozilla-central/rev/67262fc52141
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
status-firefox39: --- → disabled
status-firefox40: --- → affected
Comment on attachment 8635452 [details] [diff] [review]
Patch

Small CSS change for Windows10. Beta+ Aurora+
Attachment #8635452 - Flags: approval-mozilla-beta?
Attachment #8635452 - Flags: approval-mozilla-beta+
Attachment #8635452 - Flags: approval-mozilla-aurora?
Attachment #8635452 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/cad2c87fdc36
status-firefox41: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/024bd9c532b8
status-firefox40: affected → fixed
QA Contact: cornel.ionce
The border between tab strip and the navigation toolbar is now properly removed on Windows 10 64-bit using:
- latest Nightly, build ID: 20150723030207;
- latest Aurora, build ID: 20150723004007;
- Firefox 40 beta 7, build ID: 20150723165742.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
status-firefox41: fixed → verified
status-firefox42: fixed → verified
Blocks: 1189216
(Reporter)

Updated

2 years ago
No longer blocks: 1189216
Depends on: 1189216
(Reporter)

Updated

2 years ago
Depends on: 1190462

Updated

4 months ago
Flags: needinfo?(shorlander)
You need to log in before you can comment on or make changes to this bug.