Closed Bug 851009 Opened 11 years ago Closed 11 years ago

Australis tab separators are missing the light highlight/glow

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: Gijs)

References

()

Details

(Whiteboard: [Australis:M3])

Attachments

(1 file, 8 obsolete files)

The current separators came from a previous WIP patch and I didn't realize they didn't match the spec.

We should probably just switch to using images from the specs instead of the existing linear gradients to avoid potential performance issues.

If we want to continue with gradients, Frank Yan provided the styles he threw together for this:
background: linear-gradient(hsla(0,0%,100%,0), hsla(0,0%,100%,.2)) 3px,
            linear-gradient(transparent, hsla(0,0%,0%,.4)) 4px,
            linear-gradient(hsla(0,0%,100%,0), hsla(0,0%,100%,.2)) 5px !important;
background-repeat: no-repeat !important;
background-size: 1px 100% !important;
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Attachment #736608 - Flags: review?(mnoorenberghe+bmo)
Attached image Screenshot on OSX (obsolete) —
Steven, does this look OK? I have the impression the lighter (highlight) pixels on the side of the actual line are lighter than they were supposed to be, but I'm using the images from the mockup so I'm not sure what's going on there. bug 858089 will handle the light inside shadow on the horizontal stroke between the tabs and navigation bar if that's the problem, but still I'm not sure.
Attachment #736609 - Flags: feedback?(shorlander)
(In reply to :Gijs Kruitbosch from comment #2)
> Created attachment 736609 [details]
> Screenshot on OSX
> 
> Steven, does this look OK? I have the impression the lighter (highlight)
> pixels on the side of the actual line are lighter than they were supposed to
> be, but I'm using the images from the mockup so I'm not sure what's going on
> there. bug 858089 will handle the light inside shadow on the horizontal
> stroke between the tabs and navigation bar if that's the problem, but still
> I'm not sure.

(this still needs testing on Windows and Linux)

(also, sorry Stephen... I'll try and remember how to spell your first name)

(and this is the end of my bugspam... for now)
Comment on attachment 736609 [details]
Screenshot on OSX

Thanks for working on this!

I'll provide code feedback and ui-review on this by Tuesday.
I'll defer to MattN for final code review.
Attachment #736609 - Flags: feedback?(shorlander) → ui-review?(fyan)
Comment on attachment 736609 [details]
Screenshot on OSX

The tab separator should stop at the pixel above the border and not overlap the border. If it's a problem of incorrect CSS, let's fix that. If the tab separator images are 1px too tall, we need to make some new PNGs by cropping out the bottom pixel of the existing ones.
Attachment #736609 - Flags: ui-review?(fyan) → ui-review-
Attached image Screenshot of misaligned tab separator (obsolete) —
The tab separators are misaligned. They should be at the intersection of the borders of the adjacent tabs. If this results in incorrect padding between the separator/border and the tab favicon or close button, we'll need to adjust those too.
Comment on attachment 736608 [details] [diff] [review]
Patch

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

Thanks for working on this. :)

I've only tested the patch on OS X so far, and the screenshot I attached is of OS X.
There are a few details that need to be fixed before we land this:

::: browser/themes/shared/tabs.inc.css
@@ +200,3 @@
>    background-position: left bottom;
>    background-repeat: no-repeat;
> +  background-size: 3px 100%;

Perhaps, this should be:
background-size: 3px calc(100%-1px);
but we should check the computed size of the background-size to see that the PNG is not being stretched or shrunk. The PNG's intrinsic height, with default settings, should exactly match the size at which we use it.

::: browser/themes/windows/browser.css
@@ +1967,5 @@
> +
> +  #tabbrowser-tabs[movingtab] > .tabbrowser-tab[beforeselected]:not([last-visible-tab])::after,
> +  .tabbrowser-tab:not([selected]):not([afterselected-visible]):not([afterhovered]):not([first-visible-tab]):not(:hover)::before,
> +  #tabbrowser-tabs:not([overflow]) > .tabbrowser-tab[last-visible-tab]:not([selected]):not([beforehovered]):not(:hover)::after {
> +    background-image: url("chrome://browser/skin/tabbrowser/tabSeparator-luna-blue.png");

In jar.mn, you named this tabSeparator-luna.png, so the the `-blue` should probably be removed here.
Attachment #736608 - Flags: review?(mnoorenberghe+bmo) → review-
So we talked this over with Stephen, and we need the separators to go underneath the border/shadow below the tabs. Because that's Hard (TM), Stephen will get us some new images which have the translucency on the bottom pixels set in such a way that it'll work if these overlap the shadow rather than going underneath it.

In the meantime, this patch should fix the horizontal alignment issue Frank pointed out, and the XP luna blue image location (which was updated in jar.mn, as the actual image is included with the right filename, and should correspond to the selectors we're using for consistency's sake).
Attachment #736608 - Attachment is obsolete: true
Attachment #738213 - Flags: ui-review?(fyan)
Attachment #738213 - Flags: review?(mnoorenberghe+bmo)
Flags: needinfo?(shorlander)
Looking at Stephen's interactive mockup's code, I realized that we can just use the kind of CSS he used to implement the mockup. I don't think we will need Stephen to make new images.

Interactive mockup URL for reference:
https://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-osx-mainWindow.html

In the mockup, he applies a box-shadow to the navigation toolbar's parent, which in his code is a sibling of the tab bar.
In our code, the tab bar is a sibling of the navigation, but as I understand, we'll removing the ability to turn off the navigation toolbar and set tabs on bottom, so implementing the navigation toolbar's shadow by applying a box-shadow directly to the navigation toolbar should be okay:

#nav-bar {
  box-shadow: 0 0 0 1px hsla(0,0%,0%,.3),
              0 0 2px hsla(0,0%,0%,.1);
}

We'll need to remove the code in browser.css that currently implements the "shadow". It comes after the comment:

/*
 * Draw the bottom border of the tabstrip above ::before
 */

Also remove the relevant tabsontop=false block below it.
Flags: needinfo?(shorlander)
Comment on attachment 738213 [details] [diff] [review]
Aligned patch, fixed Windows problem

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

I realize that fixing the shadow widens the scope of this bug, but I'm okay with that, since I don't need much benefit in splitting off a separate bug just to fix that issue.

If you think my previous comment makes sense, let's do that, and I'll UI review it once there's a patch for that.
If you have other thoughts/questions, do let me know. :)
Attachment #738213 - Flags: ui-review?(fyan)
Attachment #738166 - Attachment is obsolete: true
Attachment #736609 - Attachment is obsolete: true
Comment on attachment 738213 [details] [diff] [review]
Aligned patch, fixed Windows problem

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

Aside from the aforementioned shadow problem, this looks good. :)

::: browser/themes/shared/tabs.inc.css
@@ +190,4 @@
>    background-size: 85% 100%;
>  }
>  
>  /* Background tab separators (1px wide).

Nit: could you update this to `(3px wide)` or just remove the parenthetical phrase?
Attached patch Patch with box-shadow (obsolete) — Splinter Review
Fixed the nit, changed the border stuff. I don't have windows/linux at hand to build this with, so I stuck this on a very minimal try run: https://tbpl.mozilla.org/?tree=Try&rev=59031908811e

If anyone can confirm/deny that this looks OK on other platforms before then, feel free. :-)

(holding off on requesting review until I've at least checked it looks good on Windows/Linux)
Attachment #738213 - Attachment is obsolete: true
Attachment #738213 - Flags: review?(mnoorenberghe+bmo)
Attached patch Patch that has been tested (obsolete) — Splinter Review
So it turns out there was unnecessary aero stuff in the old patch, which also broke the separators on aero (because we're jar.mn'ing those files into the same filename, and then native theme magic puts them in the right place, so we can just use the existing CSS and it should Just Work... more or less).

I checked this on all of the XP styles, Windows 7 aero, linux and mac, and it looks good as far as I can tell.
Attachment #738266 - Attachment is obsolete: true
Attachment #738312 - Flags: ui-review?(fyan)
Attachment #738312 - Flags: review?(mnoorenberghe+bmo)
Attachment #738312 - Attachment description: Patch with tests → Patch that has been tested
Comment on attachment 738312 [details] [diff] [review]
Patch that has been tested

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

r- because no border is shown on the add-ons manager tab because there is no nav-bar shown. I think we should remove this inconsistency of sometimes hiding the nav-bar (bug 752434) because it hinders implementation of bugs such as this one and bug 858089 but that requires more input from UX as that patch go ui-review- in the past.
IMO we should be able to remove it since we're removing other UI features which are more useful and which users care more about.

::: browser/themes/osx/browser.css
@@ +2298,2 @@
>   */
> +#nav-bar {

You should have the :not(:-moz-lwtheme) here until there is LWT support for Australis on OS X.

@@ -2300,5 @@
> -  background: linear-gradient(to top, hsla(0,0%,0%,.3), hsla(0,0%,0%,.3) 1px, transparent 1px);
> -}
> -
> -/* In tabs-on-bottom mode, fill the whole toolbar with the chrome
> - * background color.

Please leave this deletion to bug 755593.

::: browser/themes/windows/jar.mn
@@ +142,5 @@
>          skin/classic/browser/tabbrowser/tab-stroke-end.png           (tabbrowser/tab-stroke-end.png)
>          skin/classic/browser/tabbrowser/tab-stroke-start.png         (tabbrowser/tab-stroke-start.png)
>          skin/classic/browser/tabbrowser/tabDragIndicator.png         (tabbrowser/tabDragIndicator.png)
> +        skin/classic/browser/tabbrowser/tabSeparator-luna-blue.png   (tabbrowser/tabSeparator-luna-blue.png)
> +        skin/classic/browser/tabbrowser/tabSeparator.png             (tabbrowser/tabSeparator.png)

Nit: all of the other new australis tab images use hyphens rather than camelCasing in the filename so tab-separator.png is preferered for consistency.
Attachment #738312 - Flags: review?(mnoorenberghe+bmo) → review-
Whiteboard: [Australis:M3]
Attached patch Patch v2Splinter Review
Attachment #738312 - Attachment is obsolete: true
Attachment #738312 - Flags: ui-review?(fyan)
Attachment #739873 - Flags: ui-review?(fyan)
Attachment #739873 - Flags: review?(mnoorenberghe+bmo)
Attached image Screenshot without Patch v2 (obsolete) —
Attachment #740451 - Attachment description: Screenshot without this patch → Screenshot without Patch v2
Attached image Screenshot with Patch v2 (obsolete) —
Attachment #740451 - Attachment is obsolete: true
Attachment #740453 - Attachment is obsolete: true
Comment on attachment 739873 [details] [diff] [review]
Patch v2

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

I used the Photoshop eyedropper tool on the toolbar "shadow" in Stephen's mockup, and the color of that row of pixels is #7c7c7c.
The color of the row in Firefox with this patch is #828282, which becomes #808080 if we change the alpha from .22 to .3.
I think we should change the alpha to .3.
It seems to me that the reason the "shadow" mismatches with the tabs is that we're currently getting the background colors/gradients of the toolbar and tabs wrong, which is something else we need to fix.

::: browser/themes/osx/browser.css
@@ +2309,5 @@
>  
>  /* In tabs-on-bottom mode, fill the whole toolbar with the chrome
>   * background color.
>   */
>  #TabsToolbar[tabsontop="false"]:not(:-moz-lwtheme) {

It's a bit odd not to remove this rule, because after this patch lands, the tabs-on-bottom appearance will already be broken.
Nonetheless, I defer to Matt on code decisions for Australis.
Attachment #739873 - Flags: ui-review?(fyan) → ui-review+
Comment on attachment 739873 [details] [diff] [review]
Patch v2

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

I used the Photoshop eyedropper tool on the toolbar "shadow" in Stephen's mockup, and the color of that row of pixels is #7c7c7c.
The color of the row in Firefox with this patch is #828282, which becomes #808080 if we change the alpha from .22 to .3.
I think we should change the alpha to .3.
It seems to me that the reason the "shadow" mismatches with the tabs is that we're currently getting the background colors/gradients of the toolbar and tabs wrong, which is something else we need to fix.

::: browser/themes/osx/browser.css
@@ +2309,5 @@
>  
>  /* In tabs-on-bottom mode, fill the whole toolbar with the chrome
>   * background color.
>   */
>  #TabsToolbar[tabsontop="false"]:not(:-moz-lwtheme) {

It's a bit odd not to remove this rule, because after this patch lands, the tabs-on-bottom appearance will already be broken.
Nonetheless, I defer to Matt on code decisions for Australis.
Attachment #739873 - Flags: feedback+
Comment on attachment 739873 [details] [diff] [review]
Patch v2

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

I'll land this on UX today.

(In reply to Frank Yan (:fryn) from comment #19)
> I used the Photoshop eyedropper tool on the toolbar "shadow" in Stephen's
> mockup, and the color of that row of pixels is #7c7c7c.
> The color of the row in Firefox with this patch is #828282, which becomes
> #808080 if we change the alpha from .22 to .3.
> I think we should change the alpha to .3.
> It seems to me that the reason the "shadow" mismatches with the tabs is that
> we're currently getting the background colors/gradients of the toolbar and
> tabs wrong, which is something else we need to fix.

IIRC, when the tabs are moved higher in the titlebar on OS X the color will change a bit because of the gradient there. We can handle this in a new bug that depends on that change. Bug 858089 may also help.

> It's a bit odd not to remove this rule, because after this patch lands, the
> tabs-on-bottom appearance will already be broken.
> Nonetheless, I defer to Matt on code decisions for Australis.

AFAICT, this patch doesn't break tabs on bottom.  I think the problem you are mentioning is related to a customization patch which wouldn't need to be fixed here.
Attachment #739873 - Flags: review?(mnoorenberghe+bmo) → review+
Blocks: 864562
No longer blocks: 864562
Whiteboard: [Australis:M3] → [Australis:M3][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/58409a99aa1e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M3][fixed-in-ux] → [Australis:M3]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.