Closed Bug 1446168 Opened 2 years ago Closed Last year

Load "tabbox.css" as a document stylesheet

Categories

(Toolkit :: XUL Widgets, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Looking at tabbox.css I don't see any obvious issues with loading this as a non-scoped sheet - selectors like `tabs`, `tabpanels`, `.tab-text`, `.tab-bottom` appear to be only used in the context of bindings that already load that sheet.

My suspicion is that loading this in components.css is going to be easier and require less changes than loading it in global.css (due to the priority change it would get from going from a xbl sheet to a document sheet). But worth taking a closer look before making a decision.
Priority: -- → P5
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
Blocks: 1469902
Screenshots look good: https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=47e3cd1ce0e9cb5efcbfd0a8931a9ca378581cc6&newProject=try&newRev=1d41d015ceeb670112fc4ca60d4122ed1da939c6.

Paolo, what do you think about us landing this in the current UA sheet versus waiting to convert existing components to document sheets?
Comment on attachment 8986556 [details]
Bug 1446168 - Remove tab-base binding and import tabbox.css in widgets.css;

https://reviewboard.mozilla.org/r/251858/#review258600

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Paolo, what do you think about us landing this in the current UA sheet
> versus waiting to convert existing components to document sheets?

No strong preference, but definitely this should land after the merge, as well as bug 1461793.
Attachment #8986556 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #5)
> but definitely this should land after the merge, as
> well as bug 1461793.

Agreed
May as well update this patch to go straight to an author sheet after Bug 1470842.
Blocks: 1470830
Depends on: 1470842
Summary: Remove tab-base binding and import tabbox.css in components.css (or in global.css) → Remove tab-base binding and import tabbox.css in widgets.css
Note that "tabbox.css" is still referenced two more times in the file, I'll update the patch to remove those.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: P5 → P1
Attachment #8986556 - Flags: review+
Attachment #8986556 - Attachment is obsolete: true
Attachment #8990704 - Flags: review?(bgrinstead) → review?(dao+bmo)
Comment on attachment 8990704 [details]
Bug 1446168 - Remove the "tab-base" binding and import the "tabbox.css" file as a document stylesheet.

https://reviewboard.mozilla.org/r/255790/#review262582

I'd like Dão to weigh in on how to handle the specificity changes that are required due to making tabbox.css an author sheet.
Attachment #8990704 - Flags: review?(bgrinstead)
Comment on attachment 8990704 [details]
Bug 1446168 - Remove the "tab-base" binding and import the "tabbox.css" file as a document stylesheet.

The overall approach here is in line with our plan and looks good to me.
Attachment #8990704 - Flags: review?(bgrinstead) → review+
Assignee: bgrinstead → paolo.mozmail
Component: XUL → XUL Widgets
Product: Core → Toolkit
Comment on attachment 8990704 [details]
Bug 1446168 - Remove the "tab-base" binding and import the "tabbox.css" file as a document stylesheet.

https://reviewboard.mozilla.org/r/255790/#review263088

::: browser/base/content/browser.css:161
(Diff revision 2)
>  #tabbrowser-tabs:not([overflow="true"])[using-closing-tabs-spacer] ~ #alltabs-button {
>    visibility: hidden; /* temporary space to keep a tab's close button under the cursor */
>  }
>  
> -.tabbrowser-tab {
> +#tabbrowser-tabs > .tabbrowser-tab {
>    -moz-binding: url("chrome://browser/content/tabbrowser.xml#tabbrowser-tab");

What's this change about? The default tab binding is set in xul.css which this patch doesn't change.

::: browser/base/content/browser.css:164
(Diff revision 2)
>  
> -.tabbrowser-tab {
> +#tabbrowser-tabs > .tabbrowser-tab {
>    -moz-binding: url("chrome://browser/content/tabbrowser.xml#tabbrowser-tab");
>  }
>  
> -.tabbrowser-tab:not([pinned]) {
> +#tabbrowser-tabs > .tabbrowser-tab:not([pinned]) {

(In reply to Brian Grinstead [:bgrins] from comment #16)
> I'd like Dão to weigh in on how to handle the specificity changes that are
> required due to making tabbox.css an author sheet.

I don't understand why these are needed. tabbox.css should be loaded before browser stylesheets, so the latter should win on rules with matching specificity... right?
Attachment #8990704 - Flags: review?(dao+bmo) → review-
In the current state with a different cascade level for XBL, any property defined in a rule like ".tabbrowser-tab" would override the same property defined for the same matching element in a XBL stylesheet, even if the rule in the XBL stylesheet has higher specificity.

It is common for rules relating to different control states to have a relatively high specificity, for example in this case we have 2 class-like selectors and one element-like selector:

https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/toolkit/themes/windows/global/tabbox.css#42

When the cascade level becomes the same and the item is in a special state, these Toolkit properties always take priority over the alternate styling defined in the ".tabbrowser-tab" rule, which is not the original intention.

In cases where this is possible, the safest way to keep the current relative priority is to include ID selectors in browser stylesheets, like we do for example for the autocomplete popups. The first rule to need the ID selector is the base ".tabbrowser-tab" rule, and other rules that also apply to ".tabbrowser-tab" follow so they keep the same relative specificity.

This is easy to maintain because it operates on the simple rule that an ID selector is always present for a rule that applies to ".tabbrowser-tab". We could technically look at each individual rule for each platform, see which properties are defined in higher specificity rules, and only increase the specificity of rules where these properties are directly involved, but this would be much more error-prone, take much more time, and lead to confusion when future properties are added to existing rules, because it wouldn't be obvious whether there is a "toolkit" rule on some platform that would take priority.

Does this clarify the intent here?

(For special cases like the "-moz-binding" property defined in "xul.css", I think it's reasonable to not use the ID selector, even though I've added it for consistency.)
Flags: needinfo?(dao+bmo)
Also, something I didn't mention is that we could chase "toolkit" rules with high specificity that we don't need to define in the first place because they are now overridden and only used in the browser window, but this would be a much bigger style unification project that I don't think would be in scope for XBL removal, and would be as error-prone as looking at individual properties and their specificity.
(In reply to :Paolo Amadini from comment #19)
> In the current state with a different cascade level for XBL, any property
> defined in a rule like ".tabbrowser-tab" would override the same property
> defined for the same matching element in a XBL stylesheet, even if the rule
> in the XBL stylesheet has higher specificity.
> 
> It is common for rules relating to different control states to have a
> relatively high specificity, for example in this case we have 2 class-like
> selectors and one element-like selector:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 085cdfb90903d4985f0de1dc7786522d9fb45596/toolkit/themes/windows/global/
> tabbox.css#42

Let's get rid of this rule.

> When the cascade level becomes the same and the item is in a special state,
> these Toolkit properties always take priority over the alternate styling
> defined in the ".tabbrowser-tab" rule, which is not the original intention.
> 
> In cases where this is possible, the safest way to keep the current relative
> priority is to include ID selectors in browser stylesheets, like we do for
> example for the autocomplete popups. The first rule to need the ID selector
> is the base ".tabbrowser-tab" rule, and other rules that also apply to
> ".tabbrowser-tab" follow so they keep the same relative specificity.
> 
> This is easy to maintain because it operates on the simple rule that an ID
> selector is always present for a rule that applies to ".tabbrowser-tab".

I don't think this is sound. Specificity games with selectors easily escalate and in the end nobody understands why our selectors are overly complex.

> We
> could technically look at each individual rule for each platform, see which
> properties are defined in higher specificity rules, and only increase the
> specificity of rules where these properties are directly involved, but this
> would be much more error-prone, take much more time,

tabbox.css is pretty small. I don't think I buy it that there's a lot of stuff to review.

> and lead to confusion
> when future properties are added to existing rules, because it wouldn't be
> obvious whether there is a "toolkit" rule on some platform that would take
> priority.

Instead of changing selectors, you can use !important on the specific property you want to take priority, which I think is preferable anyway.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #21)
> tabbox.css is pretty small. I don't think I buy it that there's a lot of
> stuff to review.

This is a specific case where this issue came up, but there are at least twelve more stylesheets to convert. Following a general approach that requires us to look at individual properties, rather than individual rules, does add a significant amount of time.

> Instead of changing selectors, you can use !important on the specific
> property you want to take priority, which I think is preferable anyway.

If I understand correctly, you are not suggesting to put "!important" on every property in the "browser" window, but only on those properties that have a counterpart in the "toolkit" folder, or even only those where it's really required because of specificity issues. This is, in fact, the usual approach we have taken when dealing with "!important" as we made style changes in the browser, either for individual features or for projects like Australis and Photon.

This still requires us to look at each individual property. Properties vary the most between platforms, while selectors tend to be similar, so this also requires us to reason mostly separately on each platform. This also generally requires to decide on a case-by-case basis whether we can use "!important" on a shared stylesheet, or we should redefine the property in a platform stylesheet.

The difference in the alternative approach of using ID selectors is that they would be used in all the rules that apply to elements that are meant to be overridden, so deciding to include it would mostly be automatic. I believe that while these selector would appear longer, they would be less complex because the rule to build them is unambiguous. To be clear, this isn't totally isolated from the base stylesheet because there needs to be knowledge of which properties to override, which is unavoidable anyways.

All of that said, I would be fine with working at the property level, as we've always done, for the remaining twelve stylesheet conversions, and even take the opportunity to remove potentially unneeded rules from "toolkit", like you suggested for the example I made. However, it's more work and we can't front-load this as easily.

On the upside, all of the stylesheets from "components.css" have been already migrated to "widgets.css" and for the moment it seems regressions are in check, so we could call this first part of the project done and watch out for new regressions here.

We may work more gradually and precisely on XBL to document stylesheet conversions, maybe making them at the same time or right before the conversion of each component to a Custom Element. We can plan to work on these near the beginning of each Nightly cycle to reduce the need to uplift regressions. By the end of the project, this would certainly result in more compact browser stylesheets overall.
If we remove the one rule at https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/toolkit/themes/windows/global/tabbox.css#42 do the screenshot errors go away? I guess we are lucky for this case that this is a piece of UI that's well-covered by mozscreenshots - other sheet changes will be harder to confirm with automation.
Blocks: 1476769
Depends on: 1477954
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Summary: Remove tab-base binding and import tabbox.css in widgets.css → Load "tabbox.css" as a document stylesheet
Attachment #8990704 - Flags: review?(bgrinstead)
Depends on: 1483813
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8990704 - Attachment is obsolete: true
Sorry, somehow this review flag fell off my radar since I switched the review dashboard I was using in an attempt to also see phab reviews. Will look at this today.
Comment on attachment 9001948 [details] [diff] [review]
patch

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

Thanks - much simpler!
Attachment #9001948 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9c90bd0f2f
Load tabbox.css as a document stylesheet. r=bgrins
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f9c90bd0f2f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Backed out changeset 2f9c90bd0f2f (Bug 1446168) for R3 failures in /tests/reftest/tests/layout/reftests/xul/mac-tab-toolbar-ref.xul

Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=195397880
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195397880&repo=mozilla-inbound&lineNumber=109010

16:58:14     INFO - REFTEST TEST-UNEXPECTED-PASS | file:///Users/cltbld/tasks/task_1534980002/build/tests/reftest/tests/layout/reftests/xul/mac-tab-toolbar.xul == file:///Users/cltbld/tasks/task_1534980002/build/tests/reftest/tests/layout/reftests/xul/mac-tab-toolbar-ref.xul | image comparison, max difference: 0, number of differing pixels: 0
Flags: needinfo?(dao+bmo)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout by shindli@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/a54366d12608
Backed out changeset 2f9c90bd0f2f for R3 failures in /tests/reftest/tests/layout/reftests/xul/mac-tab-toolbar-ref.xul
Target Milestone: mozilla63 → ---
Keywords: checkin-needed
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91dc894ac303
Load tabbox.css as a document stylesheet. r=bgrins
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/91dc894ac303
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1491042
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.