Closed Bug 1173728 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Theme, defect, P1)

Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox39 --- disabled
firefox40 --- verified
firefox41 --- verified
firefox42 --- verified

People

(Reporter: dao, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We'll need updated tab images for this.
Flags: needinfo?(shorlander)
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.
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
Priority: -- → P1
Depends on: 1173725
Leaving as P1 as one of the few remaining important theme update bits.
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)
(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)
(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. :-\
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)
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)
(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)
Attached patch Patch (obsolete) — Splinter Review
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)
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 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 ?
(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.
Attached patch PatchSplinter Review
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 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)
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)
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)
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 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?
(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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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+
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.
No longer blocks: 1189216
Depends on: 1189216
Depends on: 1190462
Flags: needinfo?(shorlander)
You need to log in before you can comment on or make changes to this bug.