On all three platforms, the selected tab is drawn as follows: 1) Fill the tab-background-start::before, tab-background-middle, tab-background-end::before elements with -moz-dialog as a background-color 2) Apply a linear-gradient on top of those elements. That linear-gradient does not use any system colours. 3) Clip the tab-background-start::before and tab-background-end::before elements to the curve shape using an SVG clip-path 4) Put a tab stroke image on top of the tab-background-start::before, tab-background-middle, tab-background-end::before elements. There are potential cost-savings if we move that linear-gradient from step 2 into the tab stroke images from step 4. Apparently, animating linear-gradients (especially ones with stop positions) is expensive. Moving the gradients into the stroke image essentially lets us pre-compute the gradient, and this *should* take some heat off of graphics. This will also mean that the linear-gradients clip will be pre-computed, and that's more cost savings right there. And finally, it'll make any effort to switch to SVG curves (+ caching from bug 764299) easier to accomplish, since the SVG curves will only need to be filled with -moz-dialog. So, all in all, I think this is a valuable area of exploration for eliminating our TART regression.

So over to :shorlander for graphics.

Baked in the gradient to the PNGs.

(In reply to Mike Conley (:mconley) from comment #0) > This will also mean that the linear-gradients clip will be pre-computed, and > that's more cost savings right there. As discussed on IRC, this could harm hover.

(In reply to Avi Halachmi (:avih) from comment #3) > (In reply to Mike Conley (:mconley) from comment #0) > > This will also mean that the linear-gradients clip will be pre-computed, and > > that's more cost savings right there. > > As discussed on IRC, this could harm hover. Wait, remind me why that is again?

Ah, ok, avih and I cleared this up in IRC. Here's the problem he's referring to: We still need a clip-path in order to clip mouse events - that way, when hovering areas *outside* of the selected tab stroke, we should not think that we're hovering the selected tab (even though a portion of tab-background-start and tab-background-end is outside of the stroke). We do this by clipping the tab-background-start/-end::before elements to the curve shape, and only allowing those elements to respond to mouse-events. Note that bug 921051 is investigating switching from a clipped-path background to SVG for the tab curves. So how do we deal with mouse events? The solution, I believe, is to still clip the curves with a clip-path, and do the same mouse-event trickery - *except*, to only do this when the tabs are not animating. We can get away with this while doing the SVG curves because (if all goes well), the clip-path applied to the tab curves should not cause any visual effect. It'd just mean that the negative-space of the tab curves for the tabs will be clickable during a tab animation, but I think that's a compromise we can probably live with.

Stephen delivered, taking over.

Hm, so I replaced the stroke images, and removed the intermediate linear gradient, but there seems to be something wrong with how it all shakes out. There seems to be some incongruity between the colour at the bottom of the selected tab, and the colour at the top of the nav-bar. I double checked that I got the layering right by layering the stroke-middle image on top of the customToolbarColor for Windows 7 (hsl(210,75%,92%)) and comparing that with what I got when I applied the images / CSS changes, and they match. I'm going to call that my "test swatch". So I think the stroke images need some adjustments to make the blend into the nav-bar work. Or did I miss something? The image I've just attached shows a few things: 1) On the left is a selected tab with the patch / stroke applied. In the middle of that tab is a rectangular section blocking out some of the label - that's my test swatch pasted over the tab screenshot showing that they match, meaning that I think I got the layering right in the CSS, so I think something's up with the image. 2) On the right is the selected tab from the Windows 7 design spec[1], with the test swatch pasted over top of the middle of the label. Just thought I'd include it for comparison. So Stephen, is there something up with the stroke images, or did I mess something up?

This updates the stroke images and removes the linear-gradient from the CSS. This doesn't take into account the lightweight themes just yet. Local builds pushed to a talos slave support the theory that this'll give us a perf win. I'll push to try to confirm.

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=8468d0a9ed53 WIP Patch: https://tbpl.mozilla.org/?tree=Try&rev=51c49aa480e2

Compare talos suggests that this might be a pretty nice win for most OS's, including Win XP: http://compare-talos.mattn.ca/?oldRevs=8468d0a9ed53&newRev=51c49aa480e2&server=graphs.mozilla.org&submit=true

Windows gradients should meld with the NavBar now. Changed the OS X gradients as well, but OS X active tab will need a background-color. Based on #d3d3d3 / rgb(211,211,211) / hsla(0,0%,83%)

Using the new stroke images, and fixing the lwtheme issues on Windows. Will do OS X and Linux next and then request review.

OS X is done, moving to Linux...

Ok, this seems to do it. Gonna do a quick check on OS X and Windows again, since I did some shuffling about, and then I'll request review.

Comment on attachment 815586 [details] [diff] [review] Patch v1 How does this look Matt? Note that I think the gradients in the image don't work for OSX and Linux with lw-themes, but we might be able to get away with fixing that in a follow-up.

UX Baseline: https://tbpl.mozilla.org/?tree=Try&rev=b48c0f6649ef UX + Patch: https://tbpl.mozilla.org/?tree=Try&rev=b3a485c352f6

Hm, so, it looks like while this is a win, it might not be enough to completely neutralize the XP regression. We might need to do more, like switching to the SVG background with SVG caching, and only clipping for mouse events when not animating.

(In reply to Mike Conley (:mconley) from comment #15) > Comment on attachment 815586 [details] [diff] [review] > > Note that I think the gradients in the image don't work for OSX and Linux > with lw-themes, but we might be able to get away with fixing that in a > follow-up. I'd rather just get this right so we know that the approach works and because there has already been quite a bit of churn with the tab colors/images which makes it hard to follow the changes.

Comment on attachment 815586 [details] [diff] [review] Patch v1 Review of attachment 815586 [details] [diff] [review]: ----------------------------------------------------------------- r- due to comment 18. Since there is a chance a lot will change to make that work, I haven't reviewed the whole patch closely. ::: browser/themes/shared/tabs.inc.css @@ +173,5 @@ > > .tab-background-start[selected=true]::before, > .tab-background-end[selected=true]::before { > background-color: @fgTabBackgroundColor@; > + background-image: none; I think this can be deleted instead, right? @@ +183,2 @@ > background-color: @fgTabBackgroundColor@; > + background-image: url(chrome://browser/skin/tabbrowser/tab-stroke-middle.png), Why the rename of the image? It's not just the stroke in the file. I would rather the "-stroke-" images get renamed to "-active-" now that you're including the gradient.

Ok, I'll see if I can get some updated LWT strokes from shorlander. I've renamed the summary because I plan on doing the SVG curve patch here as well.

(In reply to Mike Conley (:mconley) from comment #20) > I've renamed the summary because I plan on doing the SVG curve patch here as > well. My understanding is that just doing the gradient as an image wasn't enough to fix the XP regression (comment 17). Do we know if the SVG change alone is enough? If so, I don't think we should change the gradient this late into Australis.

(In reply to Matthew N. [:MattN] from comment #22) > (In reply to Mike Conley (:mconley) from comment #20) > > I've renamed the summary because I plan on doing the SVG curve patch here as > > well. > > My understanding is that just doing the gradient as an image wasn't enough > to fix the XP regression (comment 17). Do we know if the SVG change alone is > enough? If so, I don't think we should change the gradient this late into > Australis. The expensive part was clipping the gradient and the background. Not clipping the gradient helped. Not clipping either (and using SVG) definitely puts us back in the black on Windows XP. Here's a compare-talos comparing m-c with a UX that uses SVG curves + caching, with the linear gradient moved into the stroke, for reference: http://compare-talos.mattn.ca/?oldRevs=1aeb31ef4ec8&newRev=f5c67778eccc&server=graphs.mozilla.org&submit=true We *could* try putting the gradient in the SVG curve instead. That might improve maintainability...

Here's the POC patch I was working with. Likely bitrotted, but the SVG files might be useful.

This patch is mostly done but I want to double-check some things, check the performance, and run the screenshot tool on it. It depends on a fix for bug 928790 (there is a PoC there which is unshippable by itself). UX Base: https://tbpl.mozilla.org/?tree=Try&rev=7cd2eb5814df Patch only (with -end without clip-path): https://tbpl.mozilla.org/?tree=Try&rev=61a405c945f6 Same as above but with SVG caching: https://tbpl.mozilla.org/?tree=Try&rev=b9a499bfe386 BTW, fgTabBackgroundColor isn't -moz-dialog on OS X (although there are only a few pixels where it matters).

I pushed the associated m-c that the UX baseline is based off of to try, to compare against: https://tbpl.mozilla.org/?tree=Try&rev=4c1378eb07b0 Here's a compare-talos between UX+patch+SVG caching, and the m-c baseline: http://compare-talos.mattn.ca/?oldRevs=4c1378eb07b0&newRev=b9a499bfe386&server=graphs.mozilla.org&submit=true What's striking is that the performance boost I saw in comment 23 is somewhat lessened here - in particular, on the tab open sub tests. I'd say, however, that UX+patch+SVG caching isn't terribly worse than m-c on those subtests. We're talking about a fraction of a millisecond here, and that might be an acceptable margin of error. Avi, what do you think?

(In reply to Mike Conley (:mconley) from comment #26) > Here's a compare-talos between UX+patch+SVG caching, and the m-c baseline: > > http://compare-talos.mattn.ca/ > ?oldRevs=4c1378eb07b0&newRev=b9a499bfe386&server=graphs.mozilla. > org&submit=true I think the regressions are minor and the wins are solid. For me this is landable, assuming svg caching actually lands and you're happy with the svg tabs implementation.

Comment on attachment 819738 [details] [diff] [review] v.0.1 WIP patch to switch to SVG for the clipping/gradient with non-LWT Review of attachment 819738 [details] [diff] [review]: ----------------------------------------------------------------- Tentative feedback+ - there's clearly a lot of work here still needed, but I more or less follow what's going on, and it seems to make sense. I'm just concerned (as are you, it seems) of what appears to be some duplication going on, and how we can go about reducing it. ::: browser/themes/osx/tabbrowser/tab-selected-svg.css @@ +6,5 @@ > + * CSS for tab SVG files. > + */ > + > +%include ../shared.inc > +%include ../../shared/tabs.inc.css I don't think I understand why we need to bring all of tabs.inc.css into these SVG files... unless it's because of that tab-selected-end::before rule that tab-background-fill is paired with in tabs.inc.css... but I'd imagine that this tab-selected-end::before rule would be removed once we stop this a/b testing, and this could be moved entirely into the tab-selected-svg.css files. ::: browser/themes/shared/tabbrowser/tab-selected-end.svg @@ +28,5 @@ > + <path d="m 0,0.0625 -0.05,0 0,0.938 1,0 0,-0.028 C 0.67917542,0.95840561 0.56569036,0.81970962 0.51599998,0.5625 0.48279998,0.3905 0.465,0.0659 0,0.0625 z"/> > + </clipPath> > + </defs> > + > + <foreignObject class="node" x="0" y="0" width="30.5" height="31" clip-path="url(#tab-curve-clip-path-end)"> Interesting - OK, so I'm guessing we're clipping the background instead of drawing the curve because otherwise the curve won't scale correctly? ::: browser/themes/shared/tabs.inc.css @@ +178,1 @@ > clip-path: url(chrome://browser/content/browser.xul#tab-curve-clip-path-end); I'd be curious to know if (and with what magnitude) this clip-path affects TART performance. I'm guessing it's just for pointer events. If it does happen to affect TART performance, we might want to only apply this rule if we're not animating the selected tab.

Comment on attachment 819738 [details] [diff] [review] v.0.1 WIP patch to switch to SVG for the clipping/gradient with non-LWT Review of attachment 819738 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. FWIW, I managed to make only 2 svg files which are shared so I already reduce the duplication a lot. I think I'll end up sharing the clip-path with a %include and then things won't be bad IMO. ::: browser/themes/shared/tabbrowser/tab-selected-end.svg @@ +28,5 @@ > + <path d="m 0,0.0625 -0.05,0 0,0.938 1,0 0,-0.028 C 0.67917542,0.95840561 0.56569036,0.81970962 0.51599998,0.5625 0.48279998,0.3905 0.465,0.0659 0,0.0625 z"/> > + </clipPath> > + </defs> > + > + <foreignObject class="node" x="0" y="0" width="30.5" height="31" clip-path="url(#tab-curve-clip-path-end)"> Not really, it's because the other way didn't work due to limitations in our SVG implementation (mostly bug 732930) and I was trying to reuse the same CSS gradients to reduce the amount of churn and hopefully reduce regressions as a result. ::: browser/themes/shared/tabs.inc.css @@ +178,1 @@ > clip-path: url(chrome://browser/content/browser.xul#tab-curve-clip-path-end); It's only for LWT (the comments is for A/B testing) so this is needed to clip the header image from browser-lightweightTheme.css

There is a lot of duplication in the Makefiles but I'm not sure how/if I should reduce it as there are multiple ways so I'll wait for gps' opinion. I haven't run the screenshot tool on this yet but I spot-checked it quickly on 4 platforms (including Aero). The remaining TODO is something to just check with shorlander but doesn't really need to block this bug, I just noticed the inconsistency while working on it and while I'm moving the defines. The graphical difference at the bottom seems to be gone with preserveAspectRatio so I'm feeling good about this.

UX Base: https://tbpl.mozilla.org/?tree=Try&rev=e5a808e887ff Try push of patch: https://tbpl.mozilla.org/?tree=Try&rev=3f852f0e3414 Mike, I'd appreciate it if you did an m-c base push :)

m-c Base: https://tbpl.mozilla.org/?tree=Try&rev=f0a6937120e7

Comment on attachment 821667 [details] [diff] [review] v.1 Avoid bug 928790 using pre-processing Review of attachment 821667 [details] [diff] [review]: ----------------------------------------------------------------- r+ covers just the build bits. ::: browser/themes/linux/Makefile.in @@ +10,5 @@ > +# otherwise. This falls apart when a file using one marker needs to include a file with the other > +# marker since the pre-processor instructions in the included file will not be processed. The > +# following SVG files need to include a file which uses "%" as the marker so we invoke the pre- > +# processor ourselves here with the marker specified. The resulting SVG files will get packaged by > +# the processing of the jar file in this directory. This is rather unfortunate and it makes me sad. We'll tackle this later. For now, it must do. Thank you for the detailed source comment! @@ +21,5 @@ > + --marker "%" -D TAB_SIDE=end \ > + $(ACDEFINES) \ > + $(srcdir)/../shared/tab-selected.svg > tab-selected-end.svg > + > +export:: tab-selected-svg Consider adding the following rule: .PHONY: tab-selected-svg This will ensure the rule is always executed. As it is, if a 'tab-selected-svg' file is ever created, the rule may not run.

While running mozscreenshots on Windows 7, I discovered that I broke Classic and High Contrast themes in version 1. These are fixed now but there is an existing invalidation issue that we are now hitting if -moz-windows-default-theme changes at runtime. I think that's rare enough that it doesn't need to block landing this bug. I'll investigate more tomorrow. While looking at attachment 821667 [details] [diff] [review] I realized that I didn't fold the patches as I expected so you were still seeing some A/B testing stuff which you shouldn't have. (In reply to Gregory Szorc [:gps] from comment #33) > Comment on attachment 821667 [details] [diff] [review] > v.1 Avoid bug 928790 using pre-processing > > Consider adding the following rule: > > .PHONY: tab-selected-svg Done.

Comment on attachment 822254 [details] [diff] [review] v.1.1 Fix classic and high contrast on Windows 7 and add .PHONY (r=gps) Review of attachment 822254 [details] [diff] [review]: ----------------------------------------------------------------- Kudos on this Matt - I'm really pleased with this approach, especially since it seems it'll give XP the boost it needs. A couple of bugs though: 1) On my OS X machine with a Retina display, I'm getting a 1px seam after the left curve: http://i.imgur.com/3m1iIOL.png 2) On my Windows 8 VM, I'm seeing what appears to be a clipping discrepancy on the right curve: http://i.imgur.com/eZOJOxt.png. I see the same discrepancy on Windows XP Luna: http://i.imgur.com/L2RYqcV.png I'm OK fixing that up in follow-ups though, if you are. So this has my r+ with the provision that we file follow-ups for those polish issues. ::: browser/themes/shared/tab-selected.svg @@ +30,5 @@ > + #tab-background-fill { > + background-color: @fgTabBackgroundColor@; > + background-image: @fgTabTexture@; > + background-repeat: no-repeat; > + height:100%; Nit - spaces after the :'s in height and width. @@ +46,5 @@ > + > +%include ../../base/content/tab-shape.inc.svg > + </defs> > + > + <foreignObject class="node" x="0" y="0" width="30.5" height="31" clip-path="url(#tab-curve-clip-path-@TAB_SIDE@)"> This is really quite brilliant. :) ::: browser/themes/windows/places/organizer-aero.css @@ +9,4 @@ > > %filter substitution > %define toolbarHighlight rgba(255,255,255,.5) > +/* TODO: can the above be deleted to use the color from windowsShared.inc instead? */ My vote is yes - I think we can / should unify these. It doesn't make a whole lot of sense for there to be a difference (and so small a difference, too).

(In reply to Mike Conley (:mconley) from comment #35) > Comment on attachment 822254 [details] [diff] [review] > v.1.1 Fix classic and high contrast on Windows 7 and add .PHONY (r=gps) > A couple of bugs though: > … > I'm OK fixing that up in follow-ups though, if you are. So this has my r+ > with the provision that we file follow-ups for those polish issues. I'm pretty sure I fixed the OS X HiDPI issue and very possibly the other issue too. I removed the viewBox since it seems like it was no longer needed to workaround clipping issues and according to jwatt, it is somewhat expensive. I also made the width of the foreignObject correct since the viewBox was gone. Those two seemed to make it about the same as before. We already have bug 879679 for a leak on the left middle of the tab. > ::: browser/themes/shared/tab-selected.svg > Nit - spaces after the :'s in height and width. Fixed. > @@ +46,5 @@ > > + > > +%include ../../base/content/tab-shape.inc.svg > > + </defs> > > + > > + <foreignObject class="node" x="0" y="0" width="30.5" height="31" clip-path="url(#tab-curve-clip-path-@TAB_SIDE@)"> > > This is really quite brilliant. :) Thanks :) > ::: browser/themes/windows/places/organizer-aero.css > > %define toolbarHighlight rgba(255,255,255,.5) > > +/* TODO: can the above be deleted to use the color from windowsShared.inc instead? */ > > My vote is yes - I think we can / should unify these. It doesn't make a > whole lot of sense for there to be a difference (and so small a difference, > too). I agree, done.

Thanks for the reviews! https://hg.mozilla.org/projects/ux/rev/e771168640a7

Bug 935305 Move preprocessor to mozbuild.action . the patch shall be updated.

(In reply to zhoubcfan from comment #38) > Bug 935305 Move preprocessor to mozbuild.action . > > the patch shall be updated. Thanks for noticing. This is being handled in bug 937024.