Closed Bug 1354199 Opened 4 years ago Closed 4 years ago

Remove border and background fallback styling from tabbox.css

Categories

(Toolkit :: Themes, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: srivatsav1998, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [good first bug][lang=css])

Attachments

(1 file, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #1343196 +++

In toolkit/themes/windows/global/tabbox.css and toolkit/themes/linux/global/tabbox.css, we can remove border and background related properties from elements that also have -moz-appearance set, since -moz-appearance causes these borders and backgrounds not to be used.

I think this affects all rules in both files except for these two rules:

http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/toolkit/themes/windows/global/tabbox.css#13-17

http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/toolkit/themes/windows/global/tabbox.css#128-132
I would lik to work on this bug. Should I remoe the remove border and background related properties under toolkit/themes/linux/global/tabbox.css ?
Attached patch bug-1354199-fix.patch (obsolete) — Splinter Review
I uploaded the patch removing the border and background related properties under toolkit/themes/linux/global/tabbox.css too. Please review it and if the properties under Linux should not be removed please let me know I will resubmit the patch without removing those.
Attachment #8855616 - Flags: review?(dao+bmo)
Comment on attachment 8855616 [details] [diff] [review]
bug-1354199-fix.patch

>--- a/toolkit/themes/linux/global/tabbox.css
>+++ b/toolkit/themes/linux/global/tabbox.css
>@@ -70,18 +70,16 @@ tab + tab {
>    :: Tabs that are attached to the bottom of a panel, but not necessarily
>    :: a tabpanels.
>    ::::: */
> 
> .tab-bottom {
>   margin-top: 0;
>   margin-bottom: 2px;
>   border-top: none;
>-  border-bottom: 2px solid;
>-  -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
>   border-top-left-radius: 0;
>   border-top-right-radius: 0;
>   border-bottom-right-radius: 2px;
>   border-bottom-left-radius: 2px;
> }

Thanks for working on this. In this file, there are many more rules where you need to remove border and background related properties. Even in the above rule, there's still border-top and the border-radius properties.

>--- a/toolkit/themes/windows/global/tabbox.css
>+++ b/toolkit/themes/windows/global/tabbox.css
>@@ -7,38 +7,36 @@
>   ======================================================================= */
> 
> @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> 
> /* ::::: tabs ::::: */
> 
> .tabs-left,
> .tabs-right {
>-  border-bottom: 2px solid;
>-  -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
> }

You seem to have misread comment 0. This rule is one of the two that need to stay as they were.
Attachment #8855616 - Flags: review?(dao+bmo) → review-
I want to work on this bug.Can someone assign it to me please.
(In reply to Hemant Singh Patwal from comment #4)
> I want to work on this bug.Can someone assign it to me please.

I'd like to first give Srivatsav a chance to submit a new patch. Srivatsav, are you going to do that?
Flags: needinfo?(srivatsav1998)
Sorry my bad, I have misread the information in the comment 0. I have University exams going on so I am unable to devote much time for this. I am still interested in working on this issue and will submit the new patch in a week.
Flags: needinfo?(srivatsav1998)
Attached patch bug-1354199-fix.patch (obsolete) — Splinter Review
This is a proposed patch for the bug. Please review the patch and guide me if I am wrong.
Attachment #8855616 - Attachment is obsolete: true
Attachment #8857831 - Flags: review?(dao+bmo)
Comment on attachment 8857831 [details] [diff] [review]
bug-1354199-fix.patch

>--- a/toolkit/themes/linux/global/tabbox.css
>+++ b/toolkit/themes/linux/global/tabbox.css
>@@ -31,17 +31,16 @@ tabpanels {
> }
> 
> /* ::::: tab ::::: */
> 
> tab {
>   position: relative;
>   -moz-appearance: tab;
>   margin-top: 2px;
>-  border: 2px solid;
>   border-bottom: none;
>   -moz-border-top-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>   border-top-left-radius: 2px;
>   border-top-right-radius: 2px;
>   padding: 3px 4px;
>   background-color: -moz-Dialog;
>@@ -70,17 +69,16 @@ tab + tab {
>    :: Tabs that are attached to the bottom of a panel, but not necessarily
>    :: a tabpanels.
>    ::::: */
> 
> .tab-bottom {
>   margin-top: 0;
>   margin-bottom: 2px;
>   border-top: none;
>-  border-bottom: 2px solid;
>   -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
>   border-top-left-radius: 0;
>   border-top-right-radius: 0;
>   border-bottom-right-radius: 2px;
>   border-bottom-left-radius: 2px;
> }
> 
> .tab-bottom[visuallyselected="true"] {

There's a lot more stuff to remove in this file. For instance, all properties involving anything border or background related in the rules above. (As well as in the other rules in this file.)

>--- a/toolkit/themes/windows/global/tabbox.css
>+++ b/toolkit/themes/windows/global/tabbox.css
>@@ -14,37 +14,31 @@
> .tabs-right {
>   border-bottom: 2px solid;
>   -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
> }
> 
> /* ::::: tabpanels ::::: */
> 
> tabpanels {
>-  -moz-appearance: tabpanels; 
>-  border-right: 2px solid;
>-  border-bottom: 2px solid;
>-  border-left: 2px solid;
>+  -moz-appearance: tabpanels;
>   -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>   padding: 8px;
>   background-color: -moz-Dialog;
>   color: -moz-DialogText;
> }
> 
> /* ::::: tab ::::: */
> 
>-tab 
>+tab
> {
>   -moz-appearance: tab;
>   margin-top: 2px;
>-  border-top: 2px solid;
>-  border-right: 2px solid;
>-  border-left: 2px solid;
>   border-bottom: 1px solid ThreeDHighlight;
>   -moz-border-top-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>   border-top-left-radius: 2px;
>   border-top-right-radius: 2px;
>   padding: 1px 4px 2px 4px;
>   background-color: -moz-Dialog;
>@@ -91,18 +85,16 @@ tab:first-of-type[visuallyselected="true
> /* ::::: tab-bottom ::::::::::
>    :: Tabs that are attached to the bottom of a panel, but not necessarily
>    :: a tabpanels.
>    ::::: */
> 
> .tab-bottom {
>   margin-top: 0;
>   margin-bottom: 2px;
>-  border-top: 1px solid;
>-  border-bottom: 2px solid;
>   -moz-border-top-colors: ThreeDShadow;
>   -moz-border-bottom-colors: ThreeDShadow ThreeDLightShadow;
>   border-top-left-radius: 0;
>   border-top-right-radius: 0;
>   border-bottom-right-radius: 2px;
>   border-bottom-left-radius: 2px;
>   padding: 2px 4px 1px 4px;
> }

Same here.

Please upload an updated patch and request review again. Thanks!
Attachment #8857831 - Flags: review?(dao+bmo) → review-
Attached patch bug-1354199-fix-v2.patch (obsolete) — Splinter Review
I think I have removed the required things. Please review it and guide me if I am wrong again. Sorry for taking too long to solve this issue, I am new to solve a bug in this category.
Attachment #8857831 - Attachment is obsolete: true
Attachment #8857869 - Flags: review?(dao+bmo)
Comment on attachment 8857869 [details] [diff] [review]
bug-1354199-fix-v2.patch

># HG changeset patch
># User srivatsav <srivatsav1998@gmail.com>
># Date 1492075974 -19800
>#      Thu Apr 13 15:02:54 2017 +0530
># Node ID 1b3066696a2479a802f4cbb76862d469e2d50e2b
># Parent  aca6b2a5a2ab3338436c9e819dc2244a022b6425
>Bug 1354199: Removed border and background fallback styling from tabbox.css.
>
>diff --git a/toolkit/themes/linux/global/tabbox.css b/toolkit/themes/linux/global/tabbox.css
>--- a/toolkit/themes/linux/global/tabbox.css
>+++ b/toolkit/themes/linux/global/tabbox.css
>@@ -15,50 +15,42 @@ tabs {
>   position: relative;
>   z-index: 0;
> }
> 
> /* ::::: tabpanels ::::: */
> 
> tabpanels {
>   -moz-appearance: tabpanels;
>-  border: 2px solid;
>   -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-top-colors: ThreeDHighlight ThreeDLightShadow;
>   padding: 8px;
>   background-color: -moz-Dialog;
>   color: -moz-DialogText;
> }
> 
> /* ::::: tab ::::: */
> 
> tab {
>   position: relative;
>   -moz-appearance: tab;
>   margin-top: 2px;
>-  border: 2px solid;
>-  border-bottom: none;
>   -moz-border-top-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>-  border-top-left-radius: 2px;
>-  border-top-right-radius: 2px;
>   padding: 3px 4px;
>-  background-color: -moz-Dialog;
>   color: -moz-DialogText;
> }
> 
> tab[visuallyselected="true"] {
>   z-index: 1;
>   margin-top: 0;
>   margin-bottom: -2px;
>-  border-bottom-left-radius: 3px;
>-  border-bottom-right-radius: 3px;
>   padding-top: 4px;
>   padding-bottom: 6px;
> }
> 
> tab + tab {
>   margin-inline-start: -2px;
> }
> 
>@@ -69,23 +61,17 @@ tab + tab {
> /* ::::: tab-bottom ::::::::::
>    :: Tabs that are attached to the bottom of a panel, but not necessarily
>    :: a tabpanels.
>    ::::: */
> 
> .tab-bottom {
>   margin-top: 0;
>   margin-bottom: 2px;
>-  border-top: none;
>-  border-bottom: 2px solid;
>   -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
>-  border-top-left-radius: 0;
>-  border-top-right-radius: 0;
>-  border-bottom-right-radius: 2px;
>-  border-bottom-left-radius: 2px;
> }

Looks better, but you need to remove stuff even more aggressively. This includes -moz-border-*-colors and background-color.

>--- a/toolkit/themes/windows/global/tabbox.css
>+++ b/toolkit/themes/windows/global/tabbox.css
>@@ -14,120 +14,94 @@
> .tabs-right {
>   border-bottom: 2px solid;
>   -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
> }
> 
> /* ::::: tabpanels ::::: */
> 
> tabpanels {
>-  -moz-appearance: tabpanels; 
>-  border-right: 2px solid;
>-  border-bottom: 2px solid;
>-  border-left: 2px solid;
>+  -moz-appearance: tabpanels;
>   -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-bottom-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>   padding: 8px;
>   background-color: -moz-Dialog;
>   color: -moz-DialogText;
> }
> 
> /* ::::: tab ::::: */
> 
>-tab 
>+tab
> {
>   -moz-appearance: tab;
>   margin-top: 2px;
>-  border-top: 2px solid;
>-  border-right: 2px solid;
>-  border-left: 2px solid;
>-  border-bottom: 1px solid ThreeDHighlight;
>   -moz-border-top-colors: ThreeDHighlight ThreeDLightShadow;
>   -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>   -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>-  border-top-left-radius: 2px;
>-  border-top-right-radius: 2px;
>   padding: 1px 4px 2px 4px;
>-  background-color: -moz-Dialog;
>   color: -moz-DialogText;
> }

Same.

> tab:-moz-locale-dir(rtl) {
>-  border-bottom-left-radius: 1px;
>-  border-bottom-right-radius: 0px;
> }

You can also remove this rule since it's now empty.

> tab[beforeselected="true"]:-moz-locale-dir(ltr),
> tab[visuallyselected="true"]:-moz-locale-dir(rtl) + tab {
>-  border-right: none;
>-  border-top-right-radius: 0;
> }
> 
> tab[visuallyselected="true"]:-moz-locale-dir(ltr) + tab,
> tab[beforeselected="true"]:-moz-locale-dir(rtl) {
>-  border-left: none;
>-  border-top-left-radius: 0;
> }

ditto

(In reply to Srivatsav Gunisetty from comment #9)
> Sorry for taking too long to solve this issue, I am new to
> solve a bug in this category.

No worries!
Attachment #8857869 - Flags: review?(dao+bmo) → review-
Attached patch bug-1354199-fix-v3.patch (obsolete) — Splinter Review
I think I have removed all the lines which are useless. Please review the patch.
Attachment #8857869 - Attachment is obsolete: true
Attachment #8858471 - Flags: review?(dao+bmo)
I'm on my phone, will need to do a full review when I'm back on my computer. Looks good at a first glance. The only thing I noticed is that you should remove the empty rule for tab:-moz-locale-dir(rtl).
Attached patch bug-1354199-fix-v4.patch (obsolete) — Splinter Review
I have removed the empty rule which you have mentioned too. Please review it.
Attachment #8858471 - Attachment is obsolete: true
Attachment #8858471 - Flags: review?(dao+bmo)
Attachment #8858479 - Flags: review?(dao+bmo)
Comment on attachment 8858479 [details] [diff] [review]
bug-1354199-fix-v4.patch

>--- a/toolkit/themes/windows/global/tabbox.css
>+++ b/toolkit/themes/windows/global/tabbox.css

> .tab-bottom[visuallyselected="true"] {
>   margin-bottom: 0;
>   -moz-border-top-colors: -moz-Dialog;
>   padding: 4px 6px 1px 6px;
> }

Oops, you forgot to remove -moz-border-top-colors here. Looks good otherwise!
Attachment #8858479 - Flags: review?(dao+bmo) → review+
Assignee: nobody → srivatsav1998
Sorry, forgot about that. I have removed the line mentioned.
Attachment #8858479 - Attachment is obsolete: true
Attachment #8858574 - Flags: review?(dao+bmo)
Attachment #8858574 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/364c11f78435
Remove border and background fallback styling from tabbox.css. r=dao
https://hg.mozilla.org/mozilla-central/rev/364c11f78435
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
thanks for landing this, I see a performance improvement:
== Change summary for alert #6071 (as of April 16 2017 07:37 UTC) ==

Improvements:

 20%  tps summary windows7-32 opt      61.09 -> 49.1
 17%  tps summary windows8-64 opt      56.54 -> 47
 15%  tps summary windows7-32 pgo      45.47 -> 38.48

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6071
Hi, is it expected that only non-e10s builds are benefiting from the tps improvements from this patch? In bug 1362920 it came up, that it were nice to be able to extend this improvement for e10s builds as well... since I don't quite understand what is the cause of the improvement exactly I thought I'll just ask you if it's completely non-e10s specific or something similar could be done for e10s as well in theory.
Flags: needinfo?(dao+bmo)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> Hi, is it expected that only non-e10s builds are benefiting from the tps
> improvements from this patch? In bug 1362920 it came up, that it were nice
> to be able to extend this improvement for e10s builds as well... since I
> don't quite understand what is the cause of the improvement exactly I
> thought I'll just ask you if it's completely non-e10s specific or something
> similar could be done for e10s as well in theory.

I didn't expect that this would affect tps at all. You'd have to ask a Style System peer how it did that and why e10s didn't benefit.
Flags: needinfo?(dao+bmo) → needinfo?(cam)
My guess would be that it's the rules with the later siblings combinator, such as:

  tab[visuallyselected="true"]:-moz-locale-dir(rtl) + tab {
    border-right: none;
    border-top-right-radius: 0;
  }

With this rule, when a tab has its visuallyselected attribute changed to or from "true", we'll restyle all later sibling <tab> elements and their descendants.  If there are many tabs open, this could take some time.  (Gecko unfortunately doesn't know it only needs to restyle the later sibling <tab> elements but not their descendants.  We might add this optimization in stylo.)

You could test this hypothesis by restoring those rules but without the property declarations in them, and seeing if the time gets worse again.

I don't know why this only affects non-e10s configurations.
Flags: needinfo?(cam)
Duplicate of this bug: 1354901
Duplicate of this bug: 1198005
You need to log in before you can comment on or make changes to this bug.