Closed
Bug 1463820
Opened 7 years ago
Closed 6 years ago
Create a test build that ports all XBL <resource> sheets to UA styles to measure UI breakage
Categories
(Firefox :: Theme, task, P5)
Firefox
Theme
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: bgrins, Unassigned)
References
Details
Attachments
(3 files, 2 obsolete files)
As we've been discussing in Bug 1457907, there are various things in the UI that can break that happen when porting XBL styles to UA styles.
I'd like to measure how much work it would be to deal with that so we can balance cost vs making a platform change to support a new cascade (see https://bugzilla.mozilla.org/show_bug.cgi?id=1457907#c23).
So the idea is to make a patch that migrates every `<resources><stylesheet /></resources>` in toolkit/content/widgets into components.css. We can then check talos, mochitests, and visual issues.
We could also make a version that moves them into global.css (as a document style), although I expect there will be much more breakage in that case.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
Would you agree that porting the stylesheets to Shadow DOM would be a better solution for encapsulation and therefore maintainability in the long term? I think reduced scoping is a major feature of `<resources><stylesheet /></resources>` that isn't worth losing for a short-term gain.
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)
> Would you agree that porting the stylesheets to Shadow DOM would be a better
> solution for encapsulation and therefore maintainability in the long term? I
> think reduced scoping is a major feature of `<resources><stylesheet
> /></resources>` that isn't worth losing for a short-term gain.
Shadow DOM doesn't allow you to 'reach in' to the anon content with CSS, does it? We use that all over the place for XBL (breaking encapsulation), and moving away from it would require some pretty major rewriting AFAICT, since consumers could only customize the shadow styles through attributes or CSS variables defined on the host element.
What would satisfy both cases (keeping encapsulation while not requiring rewrites) would be scoped stylesheets using normal DOM under the CE. But AIUI that's been removed from the platform, and there's not really interest to re-add it (https://bugzilla.mozilla.org/show_bug.cgi?id=1457907#c25).
I'll be curious to see how much breakage there is with this patch. We've already been moving some individual scoped sheets to global sheets without many issues. For the most part these sheets don't seem to use overly generic selectors - although there are some cases where they do.
Reporter | ||
Comment 5•7 years ago
|
||
Also, I expect there will be some cases where the document-level styling of anon content is low enough and/or we want to take advantage of other SD features such that switching directly to SD will make sense.
Comment 6•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #3)
> > Would you agree that porting the stylesheets to Shadow DOM would be a better
> > solution for encapsulation and therefore maintainability in the long term? I
> > think reduced scoping is a major feature of `<resources><stylesheet
> > /></resources>` that isn't worth losing for a short-term gain.
>
> Shadow DOM doesn't allow you to 'reach in' to the anon content with CSS,
> does it? We use that all over the place for XBL (breaking encapsulation),
> and moving away from it would require some pretty major rewriting AFAICT,
> since consumers could only customize the shadow styles through attributes or
> CSS variables defined on the host element.
There have been multiple proposals (and even implementations in other browsers) to support such a thing like we allow with child selectors and XBL anonymous content. From some Google searches it seemed like Mozilla wasn't against supporting such a thing. Since we're already talking about new/non-standard APIs in bug 1457907, I don't see why we shouldn't includes a way to reach in as a potential option.
IIRC some of the cases where we reach into anonymous content now is simply to override the "toolkit" style because someone didn't want to change/break other toolkit apps… that's no longer something we're worrying about so IMO the right thing to do is to merge the browser/ overrides into the toolkit widget.
I guess I'm not really convinced that major rewriting would be necessary, at least not so much so to go with this blunt approach. Has someone individually evaluated many of the components affected by this to reach this conclusion? I'm be interested in seeing notes on that.
> What would satisfy both cases (keeping encapsulation while not requiring
> rewrites) would be scoped stylesheets using normal DOM under the CE. But
> AIUI that's been removed from the platform, and there's not really interest
> to re-add it (https://bugzilla.mozilla.org/show_bug.cgi?id=1457907#c25).
That proposal doesn't address the issue of rules not intending to modify anonymous/shadow content leaking into it… the encapsulation helps in both directions: preventing styles from coming in and leaking out.
> I'll be curious to see how much breakage there is with this patch. We've
> already been moving some individual scoped sheets to global sheets without
> many issues. For the most part these sheets don't seem to use overly generic
> selectors - although there are some cases where they do.
The ones I've seen you move so far have been trivial which is why I didn't complain about them, even though I think the move was a step backwards in maintainability.
Updated•7 years ago
|
Priority: -- → P5
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•7 years ago
|
||
- Try push for original version (UA styles): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8562fad4eaf8a5aa92f211d1c106f7865fcf2dc
- Talos comparison for original version (nothing noticeable): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0c0d8c15d5472da496a242df865b140be0251049&newProject=try&newRevision=c8562fad4eaf8a5aa92f211d1c106f7865fcf2dc&framework=1&showOnlyImportant=1
- Try push for alternate version (document styles): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d9b13ee6c9e9107c4b4d0048dc30cbf2adc45a3
- Talos comparison for alternate version (pretty bad): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0c0d8c15d5472da496a242df865b140be0251049&newProject=try&newRevision=0d9b13ee6c9e9107c4b4d0048dc30cbf2adc45a3&framework=1&showOnlyImportant=1
Comment 10•7 years ago
|
||
That's really interesting! It may be worth trying to migrate only half of the bindings to see if the regression is cut in half, or if there is one specific stylesheet that introduces a problem.
There is also a way to create profiles for the builds, not sure if it can help here:
https://wiki.mozilla.org/Buildbot/Talos/Profiling
As far as styling goes, both builds look pretty good, there are only minor issues as far as I can see.
Reporter | ||
Comment 11•7 years ago
|
||
(In reply to :Paolo Amadini from comment #10)
> That's really interesting! It may be worth trying to migrate only half of
> the bindings to see if the regression is cut in half, or if there is one
> specific stylesheet that introduces a problem.
The way global.css and components.css get loaded are quite different (with the latter being handled in nsLayoutStylesheetCache). It's been on my todo list to measure any impact we'd get by moving global.css into the cache - I guess this would be an excuse to do that.
Comment 12•7 years ago
|
||
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)
> Would you agree that porting the stylesheets to Shadow DOM would be a better
> solution for encapsulation and therefore maintainability in the long term?
Actually, I don't think porting the XUL widgets stylesheets to Shadow DOM would improve maintainability in the long term. It may even have the opposite effect.
To frame the discussion, our main milestone is to create a set of Custom Elements that serve the purpose of replacing the obsolete XBL bindings. If we were designing a library of widgets for web content to be used in an uncontrolled environment, then scoping may bring some benefits. However, this is definitely out of scope for us, since the scope of stylesheets would be the least of our problems, as we would have to spend much more time in redesigning the API of each widget to make it sane, and this would multiply our already year-long project by a factor of two or more.
Given this, for our Custom Elements we can safely assume a controlled environment, with legacy add-ons gone and even Mozilla-authored bootstrapped add-ons being replaced by WebExtensions experiments.
I took a look at the XBL stylesheets affected by this change, and the vast majority of rules are already scoped:
- by specifying the tag name, or
- by class name, where the class is generally prefixed with the tag name, or in general uniquely named.
The same type of rules are also found in "global.css", which is not loaded at the level of the individual widget. This works just as well as the XBL stylesheets, so the approach is already proven. This file also contains generic rules like "plain" that are designed to apply to multiple elements, and are unscoped by intent, so are less relevant to this case.
In our main browser window, most of the styles are also conrolled by "browser.css". The styles defined here are also not scoped. At the moment, if I understand correctly, they are also at a different CSS cascade level than both "global.css" and XBL stylesheets.
It is this variety of combinations, together with the inherent complexity of CSS specificity, that makes it difficult for people to predict the effects of any change made on the browser CSS. Of course, these effects can always be tested with developer tools, and regressions can be prevented with tooling like MozScreenshots, but this doesn't take away the time lost with making a "bad prediction" to begin with.
For example, if I look up a CSS rule with a global text search, and the file a rule is located in makes no difference as to the cascade level, I can use much more ordinary web programming knowledge to make the predictions.
I think this is a key element of "maintainability", in other words it reduces the time it takes for someone to successfully make changes to the code. I posit that by reducing the number of cascade levels and reducing forms of encapsulation that are not apparent from the contents of the file, we improve maintainability.
I'm saying this from personal experience working on the XBL removal project - even for someone like me who has been around for years, it still takes a while to ramp up among all these special cases. Those who deal every day with browser and toolkit style modifications have all of this complex information already in their heads, so the difference may not be as apparent, but by reducing the amount of information needed we can lower the barrier to contribution and go faster, both for volunteers and for staff.
Finally, reducing the combinations by moving everything to be scoped seems quite far away from the current state. Given the controlled environment, I think moving everything to be unscoped and instead controlled at the rule level is a better move.
Unsurprisingly, the try build that does this looks strikingly good, with a few minor glitches, even when using document styles.
There is a longer Nightly cycle upcoming. If we can stabilize on this browser architecture, we can figure out the most obvious regressions and then land the patch at the beginning of the cycle, having quite some time to catch any regressions that we didn't notice initially. I'd prefer figuring out the performance regressions with the document styles and going for that approach, rather than UA styles, for the reasons stated earlier. Fixing those regression will also be much simpler with the simplified cascade rules, not having to deal with bugs like the "!important" in the UA stylesheets overriding everything else.
Comment 13•7 years ago
|
||
Also, Emilio had some reservations about preserving the current XBL cascade level in bug 1457907 comment 28, again driven by the need for maintainability of the platform code. Without this cascade level, we'll have to deal with unpredictable regressions anyways during the XBL removal project. I think that by doing a bulk migration to document styles we'll save time overall to our team and others, and finish the XBL removal sooner.
Reporter | ||
Comment 14•6 years ago
|
||
There's a lot of relation between this, bug 1457907, and bug 1458426 (with the latter two bugs being related to fallout of converting sheets to UA styles).
I still think the lowest cost for migration on the frontend side would be adding a new cascade level after UA and before document as Emilio outlines in https://bugzilla.mozilla.org/show_bug.cgi?id=1457907#c23. But that has two main drawbacks:
1) It's a lot of churn on the platform side
2) It's a new non-standard thing, albeit more simple than the current non-standard thing we do with XBL sheets.
Long term, at least for unscoped rules, I agree document styles would be ideal since we could avoid those two bugs (1457907 and 1458426), and others like it. If we were to do that, it's likely we'd be addressing CSS regressions for the entire next cycle, so we'd want to commit to having bandwidth to do so, both on the team doing XBL replacement and from likely reviewers on the frontend team. Doing so would smooth over the rest of the work in the XBL project (frontload the cost of the CSS migration so that the cost of migrating individual bindings is lower). We'd have to spend time now looking into what's going on with the performance issues now though if we want that to even be a possibility.
See Also: → 1458426
Reporter | ||
Comment 15•6 years ago
|
||
(In reply to :Paolo Amadini from comment #12)
> The same type of rules are also found in "global.css", which is not loaded
> at the level of the individual widget. This works just as well as the XBL
> stylesheets, so the approach is already proven. This file also contains
> generic rules like "plain" that are designed to apply to multiple elements,
> and are unscoped by intent, so are less relevant to this case.
>
> In our main browser window, most of the styles are also conrolled by
> "browser.css". The styles defined here are also not scoped. At the moment,
> if I understand correctly, they are also at a different CSS cascade level
> than both "global.css" and XBL stylesheets.
Here's the current situation:
global.css and browser.css are at the same cascade level: they are both loaded as author sheets and not scoped:
- browser.css is loaded in browser.xul as `<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>`: https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/browser/base/content/browser.xul#19
- which in turn loads global.css as `@import url("chrome://global/skin/");` https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/browser/themes/windows/browser.css#5
XBL sheets have their own cascade (after UA but before author) and are scoped.
xul.css and components.css (where we have been porting some XBL sheets) are both loaded at the UA cascade and not scoped.
Comment 16•6 years ago
|
||
I think we're talking past each other as you're talking about UA vs. document in some places which isn't relevant for switching from <resource> stylesheets to a shadow DOM stylesheet as I'm suggesting. The more relevant part is how often we need to override the encapsulated styles from the outside (e.g. using a child selector to go into the anonymous content) and that is what I was hoping for an analysis of.
(In reply to :Paolo Amadini from comment #12)
> To frame the discussion, our main milestone is to create a set of Custom
> Elements that serve the purpose of replacing the obsolete XBL bindings. If
> we were designing a library of widgets for web content to be used in an
> uncontrolled environment, then scoping may bring some benefits. However,
> this is definitely out of scope for us, since the scope of stylesheets would
> be the least of our problems, as we would have to spend much more time in
> redesigning the API of each widget to make it sane, and this would multiply
> our already year-long project by a factor of two or more.
I really don't see how this makes sense… if the XBL binding you're replacing uses <content> then continuing to us anonymous/shadow content for the same contents should be easier for porting. I don't know why you're talking about changing APIs as I never suggested that.
> Given this, for our Custom Elements we can safely assume a controlled
> environment, with legacy add-ons gone and even Mozilla-authored bootstrapped
> add-ons being replaced by WebExtensions experiments.
Right, but we still have layering and separation of responsibilities: toolkit => browser => my browser UI feature, and yes I was suggesting we try to flatten some of toolkit with browser while we're porting.
> I took a look at the XBL stylesheets affected by this change, and the vast
> majority of rules are already scoped:
> - by specifying the tag name, or
> - by class name, where the class is generally prefixed with the tag name,
> or in general uniquely named.
OK, but how will we handle the exceptions? And what burden do they cause?
> The same type of rules are also found in "global.css", which is not loaded
> at the level of the individual widget. This works just as well as the XBL
> stylesheets, so the approach is already proven. This file also contains
> generic rules like "plain" that are designed to apply to multiple elements,
> and are unscoped by intent, so are less relevant to this case.
btw. the usefulness of the separation between global.css vs. xul.css is not clear to me.
> In our main browser window, most of the styles are also conrolled by
> "browser.css". The styles defined here are also not scoped. At the moment,
> if I understand correctly, they are also at a different CSS cascade level
> than both "global.css" and XBL stylesheets.
I'm not sure that "most of the styles" is telling me much, more useful would be knowing how many of the stylesheets need to be override by browser.css (ideally understanding whether the override was *actually* needed) i.e. what problems would occur if the resource stylesheets were inside a shadow DOM. I suspect some of the resource stylesheets are not getting overridden by browser.css and so using shadow DOM means that developers working on that widget/component won't need to worry about looking at stylesheets outside of their own while styling the shadow children
> It is this variety of combinations, together with the inherent complexity of
> CSS specificity, that makes it difficult for people to predict the effects
> of any change made on the browser CSS. Of course, these effects can always
> be tested with developer tools, and regressions can be prevented with
> tooling like MozScreenshots, but this doesn't take away the time lost with
> making a "bad prediction" to begin with.
I don't understand this argument… contents of the anonymous content ideally (and often) aren't styled by browser.css from what I can recall. For some cases where it's needed, CSS variables could be used.
> For example, if I look up a CSS rule with a global text search, and the file
> a rule is located in makes no difference as to the cascade level, I can use
> much more ordinary web programming knowledge to make the predictions.
I wouldn't even have to search if I can just look at the stylesheet(s) that are part of my shadow DOM. That's the ideal state IMO.
> I think this is a key element of "maintainability", in other words it
> reduces the time it takes for someone to successfully make changes to the
> code. I posit that by reducing the number of cascade levels
Right.
> and reducing
> forms of encapsulation that are not apparent from the contents of the file,
> we improve maintainability.
I'm not sure why you're talking about this since I'm talking about encapsulation that is apparent from the contents of the file as the stylesheet reference would be in the shadow DOM.
> …
> Finally, reducing the combinations by moving everything to be scoped seems
> quite far away from the current state.
I'm not suggesting changing everything to be scoped, this bug is about maintaining existing scoping where it makes sense.
> Given the controlled environment, I
> think moving everything to be unscoped and instead controlled at the rule
> level is a better move.
This would be going against the trend in web development, and I think it's clear that there is value in encapsulation. Making all rules unscoped is going to be a maintenance nightmare and a significant regression. We'd need a significant investment in tooling like mozscreenshots to detect accidental changes if every CSS change has the potential to change totally unrelated UI. Sure we can enforce encapsulation my class naming prefixes but why place that burden on everyone when proper tools (shadow DOM) exist for the job.
I still would like to see more concrete notes or diffs of the issues with converting the anonymous content stylesheets to shadow DOM stylesheets so we can have a more concrete discussion about the issues.
Reporter | ||
Comment 17•6 years ago
|
||
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #16)
> > It is this variety of combinations, together with the inherent complexity of
> > CSS specificity, that makes it difficult for people to predict the effects
> > of any change made on the browser CSS. Of course, these effects can always
> > be tested with developer tools, and regressions can be prevented with
> > tooling like MozScreenshots, but this doesn't take away the time lost with
> > making a "bad prediction" to begin with.
>
> I don't understand this argument… contents of the anonymous content ideally
> (and often) aren't styled by browser.css from what I can recall. For some
> cases where it's needed, CSS variables could be used.
>
> > Given the controlled environment, I
> > think moving everything to be unscoped and instead controlled at the rule
> > level is a better move.
>
> This would be going against the trend in web development, and I think it's
> clear that there is value in encapsulation. Making all rules unscoped is
> going to be a maintenance nightmare and a significant regression. We'd need
> a significant investment in tooling like mozscreenshots to detect accidental
> changes if every CSS change has the potential to change totally unrelated
> UI. Sure we can enforce encapsulation my class naming prefixes but why place
> that burden on everyone when proper tools (shadow DOM) exist for the job.
>
> I still would like to see more concrete notes or diffs of the issues with
> converting the anonymous content stylesheets to shadow DOM stylesheets so we
> can have a more concrete discussion about the issues.
So this won't be a very exhaustive list since it's a super simple example that doesn't even have it's own <resources> sheet (so no possible browser<->toolkit collapsing) but FWIW I did attempt to switch the <dropmarker> to use Shadow DOM in a patch.
So rather than constructing DOM beneath the CE in the connectedCallback at https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/content/widgets/general.js#39, I would create a Shadow DOM in the constructor. I do prefer using SD in this case as it takes away the weird check we have to do to see if it's already been connected.
For styling, we would only have to worry about rules that touch `.dropmarker-icon` (the only child node): https://searchfox.org/mozilla-central/search?q=dropmarker-icon&path=. All the CSS references are document-level sheets that dig into XBL anon content with descendant selector. The styles that get set from outside the anon content are: `list-style-image`, `width`, `height`, `-moz-context-properties`, `fill`. `pointer-events` gets set in xul.css, but that's applied to all dropmarkers so could be migrated into a scoped SD sheet.
My feeling was that hoisting up everything we override to a CSS variable is clumsy - even with this small list of properties it felt wrong to add variables for `-moz-context-properties` and `fill`. Originally I had only seen list-style-image, width, and height and it seemed pretty reasonable to have something like `--dropmarker-image`, `--dropmarker-width`, and `--dropmarker-height`. I guess we would use need to use fallback properties like `none` / `auto` to handle the case where those variables were undefined on the host, but that's doable. And, maybe `-moz-context-properties: fill; fill: currentColor;` from https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/themes/shared/popupnotification.inc.css#123 would be safe to just go ahead unconditionally set in a dropmarker sheet loaded by the Shadow DOM so we wouldn't need variables? I didn't spend more time on it because we aren't quite ready to use SD in chrome anyway (bug 1465592).
Comment 18•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #17)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #16)
> > > It is this variety of combinations, together with the inherent complexity of
> > > CSS specificity, that makes it difficult for people to predict the effects
> > > of any change made on the browser CSS. Of course, these effects can always
> > > be tested with developer tools, and regressions can be prevented with
> > > tooling like MozScreenshots, but this doesn't take away the time lost with
> > > making a "bad prediction" to begin with.
> >
> > I don't understand this argument… contents of the anonymous content ideally
> > (and often) aren't styled by browser.css from what I can recall. For some
> > cases where it's needed, CSS variables could be used.
> >
> > > Given the controlled environment, I
> > > think moving everything to be unscoped and instead controlled at the rule
> > > level is a better move.
> >
> > This would be going against the trend in web development, and I think it's
> > clear that there is value in encapsulation. Making all rules unscoped is
> > going to be a maintenance nightmare and a significant regression. We'd need
> > a significant investment in tooling like mozscreenshots to detect accidental
> > changes if every CSS change has the potential to change totally unrelated
> > UI. Sure we can enforce encapsulation my class naming prefixes but why place
> > that burden on everyone when proper tools (shadow DOM) exist for the job.
> >
> > I still would like to see more concrete notes or diffs of the issues with
> > converting the anonymous content stylesheets to shadow DOM stylesheets so we
> > can have a more concrete discussion about the issues.
>
> So this won't be a very exhaustive list since it's a super simple example
> that doesn't even have it's own <resources> sheet (so no possible
> browser<->toolkit collapsing) but FWIW I did attempt to switch the
> <dropmarker> to use Shadow DOM in a patch.
>
> So rather than constructing DOM beneath the CE in the connectedCallback at
> https://searchfox.org/mozilla-central/rev/
> c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/content/widgets/general.
> js#39, I would create a Shadow DOM in the constructor. I do prefer using SD
> in this case as it takes away the weird check we have to do to see if it's
> already been connected.
>
> For styling, we would only have to worry about rules that touch
> `.dropmarker-icon` (the only child node):
> https://searchfox.org/mozilla-central/search?q=dropmarker-icon&path=. All
> the CSS references are document-level sheets that dig into XBL anon content
> with descendant selector. The styles that get set from outside the anon
> content are: `list-style-image`, `width`, `height`,
> `-moz-context-properties`, `fill`. `pointer-events` gets set in xul.css, but
> that's applied to all dropmarkers so could be migrated into a scoped SD
> sheet.
>
> My feeling was that hoisting up everything we override to a CSS variable is
> clumsy - even with this small list of properties it felt wrong to add
> variables for `-moz-context-properties` and `fill`. Originally I had only
> seen list-style-image, width, and height and it seemed pretty reasonable to
> have something like `--dropmarker-image`, `--dropmarker-width`, and
> `--dropmarker-height`. I guess we would use need to use fallback properties
> like `none` / `auto` to handle the case where those variables were undefined
> on the host, but that's doable.
We wouldn't need CSS variables for width/height because we can simply use: the "inherit" property value for them. The width and height properties would be set on the dropmarker itself then instead of reaching across the shadow DOM boundary. I tested that it currently works in Chrome and Firefox from ShadowDOM in HTML documents: https://jsbin.com/wefejususe/1/edit?html,js,output
> And, maybe `-moz-context-properties: fill;
> fill: currentColor;` from
> https://searchfox.org/mozilla-central/rev/
> c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/themes/shared/
> popupnotification.inc.css#123 would be safe to just go ahead unconditionally
> set in a dropmarker sheet loaded by the Shadow DOM so we wouldn't need
> variables?
Yes, definitely.
So you'd only need one variable for the image and that seems really nice IMO.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 23•6 years ago
|
||
The talos regression has almost disappeared with the alternate way of loading components.css as a document sheet. It looks to be down to a 3% regression on `tps opt e10s stylo windows10-64-qr`: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b53109be5f040db1af8830178634e30d13769fc9&newProject=try&newRevision=1ba7e9636a75eed0c0b8e2e5b91015ec1ef8d36a&framework=1&showOnlyImportant=1
Comment hidden (mozreview-request) |
Comment 25•6 years ago
|
||
Attachment 8987027 [details] shows examples of CSS fixes I made after looking at some of the Mac screenshots. While we don't have extensive regression tests, most cases would likely involve increasing the specificity of "browser.css" selectors.
In other cases, like the tab bar, we may even be able to remove styles that we don't require anymore or used to not be applied anyways. This works for components that used to be general-purpose, but are now only used in the browser window. Note that a tab bar is still used in the print preview dialog, but we may be able to remove it, or just port the styles we still need.
Most issues will be very easy to debug. I had some difficulties with the disabled color of the toolbar button, especially since on Mac they change when the window is deselected and developer tools seems to be unreliable in this case, but I expect this to be the exception rather than the norm.
Comment 26•6 years ago
|
||
I do recommend landing a patch that renames "components.css" to "components-ua.css", and then adds the C++ code to load "components.css" as an author sheet, if there is no significant Talos regression in doing that.
This allows us to use artifact builds when we port styles from "components-ua.css" to "components.css". We can also land each stylesheet move separately as an individual push. This allows reporters to use MozRegression later to find the exact stylesheet responsible for a change, even if we miss the regression now.
We can also easily backout a cascade level change locally, without having to rebuild C++ code. This will be immediately useful when debugging the known issues.
After a few weeks, when we're confident we've handled most regressions, we will remove support for loading "components-ua.css".
Brian, how does this sound?
Flags: needinfo?(bgrinstead)
Comment 27•6 years ago
|
||
By the way, since we are keeping the Custom Elements implementations in the "widgets" folder, we can migrate the styles to a file named "widgets.css" and avoid renaming "components.css". I'll post a patch to that effect.
Flags: needinfo?(bgrinstead)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8980024 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8980444 -
Attachment is obsolete: true
Comment 31•6 years ago
|
||
Talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d46fc54e64299d769b0c4a2cc0f66aefe62d953a
Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a61fa6c368ca4f28a24464e97767f79518631dc0
Screenshots: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b08537261d2877a0b2ece0467d8aaa248358772
Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe61dc4b0fdc7081f57567d94b8b3c57e4fb1dd9
Comment 32•6 years ago
|
||
Comment on attachment 8987338 [details]
Bug 1463820 - Add "widgets.css" as an author stylesheet for migrating "components.css".
Talos and screenshots look good, so I moved the changeset to its own bug 1470842.
Attachment #8987338 -
Flags: review?(bgrinstead)
Comment 33•6 years ago
|
||
Also, I filed bug 1470830 as a tracking bug for the first part of the CSS migration, with more details about the steps.
Comment 34•6 years ago
|
||
Closing this bug since the build served its purpose, and the work in bug 1470830 is underway.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Updated•6 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•