Closed Bug 1834533 Opened 4 months ago Closed 4 months ago

Move login timeline CSS to a CSS file

Categories

(Firefox :: about:logins, task, P3)

Product:
Firefox
Component:
about:logins
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
116 Branch
Tracking Flags:
Tracking Status
firefox116 --- fixed

People

(Reporter: dao, Assigned: mtigley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1834533 - Move login timeline inline CSS to a separate CSS file. r=#credential-management-reviewers,dao
48 bytes, text/x-phabricator-request
Details | Review
about-logins-FOUC-nightly-115.0a1.gif
82.32 KB, image/gif
Details

The way the timeline component was implement in bug 1805233 is inconsistent with how components are generally implemented in our code base. Specifically, we want styling to be in CSS files, preferably in browser/themes/ or toolkit/themes/, respectively.

Inlining CSS in a script makes lives of people maintaining our stylesheets harder, see e.g. https://phabricator.services.mozilla.com/D178520#5895693.

Even if hypothetically, your CSS was 100% self-contained and wouldn't need to be part of regular theme maintenance, you should still put it in a CSS file so that linting works, as well as other tooling we may want to deploy in the future.

Micah, could you please look into fixing this?

Flags: needinfo?(mtigley)

Totally agree when it comes to themes, it should be defined in mentioned places.

When it comes to the CSS for the specific component only I'd prefer it to be as close to the specific component as possible.
If a separate file is so desired, keep it somewhere in aboutlogins folder.

Inlining CSS in a script makes lives of people maintaining our stylesheets harder, see e.g. https://phabricator.services.mozilla.com/D178520#5895693.

It has nothing to do with inlining. Our code has a ton of references to CSS styles in the event handlers (for the record I'm not a fan of some), so any rename should be a full text search.

Severity: -- → N/A
Type: defect → task
Priority: -- → P3
Assignee: nobody → mtigley
Status: NEW → ASSIGNED

I also totally agree with theming. These should be global and ideally we shouldn't care about those styles. But in our case and particularly for lit elements we want to make our components as encapsulated as possible.
Importing external style sheets is actually not recommended by the lit team. I think we should consider the impact here on the engineers writing the code. I find it annoying to switch between two files, when I can have everything in one place. Also a simple string search for all files would have picked this up.

Thanks Issam for the link! I too find switching between multiple files not appealing at all.

This part there is troubling:

External styles can cause a flash-of-unstyled-content (FOUC) while they load.

Micah, please check with our Lit folks and try to verify there is no funny visual effects because we moved it to the other file?

(In reply to Sergey Galich [:serg] from comment #5)

Thanks Issam for the link! I too find switching between multiple files not appealing at all.

This part there is troubling:

External styles can cause a flash-of-unstyled-content (FOUC) while they load.

Micah, please check with our Lit folks and try to verify there is no funny visual effects because we moved it to the other file?

Shouldn't be a problem for us, or a solved one anyway. moz-toggle is a good example to follow.

Flags: needinfo?(mtigley)

Honestly, I am still not convinced we should do this. aboutlogins components will most likely be used only internally by us. . And even if they are, I still don't know how this affects their reusability. We handle styling in js all over our codebase in all sorts of justifiable / not so justifiable ways. Our use case, however, is justifiable, as it aligns with the recommendations in the lit docs. While having an external stylesheet is fine, I don't see a need for it.

Inlining CSS in a script makes lives of people maintaining our stylesheets harder, see e.g. https://phabricator.services.mozilla.com/D178520#5895693.

I understand this concern, but I think a simple string search in searchfox with all files included would have done the trick. Additionally, if we were to document our design system and include CSS variable names in say markdown files, we would still need to perform a thorough search on all files. From a developer experience perspective, having the CSS inside the component is way better.

Even if hypothetically, your CSS was 100% self-contained and wouldn't need to be part of regular theme maintenance, you should still put it in a
CSS file so that linting works, as well as other tooling we may want to deploy in the future.

This also applies to html written inside lit components with the html template literal. We should use linting tools that are specific to lit ( same thing we do for instance for react ).

From a performance perspective, we have a guarantee that using those css template literals is optimal. I quote from the lit docs:

You define scoped styles in the static styles class field using the tagged template literal css function. Defining styles this way results in the most optimal performance.

The static styles class field is almost always the best way to add styles to your component, but there are some use cases you can't handle this way—for example, customizing styles per instance

Why are we embedding logic in our components to switch stylesheets for performance reasons when we have an optimal way to handle this ?

The main point I am trying to make. The components inside aboutlogins do too much and are tightly coupled. The work we are/will be doing aims to solve this and our core goal is to make UI components as "dumb" as possible and minimize external dependencies where possible. We notice all sorts of things in aboutlogins code that don't necessarily make sense to us now: like why are we using shadow dom everywhere when we don't need it, why do we have those templates in one big file instead of inside the components file and so on... And I find myself wondering, why do we need separate stylesheets ? And I honestly don't have a convincing answer for that !

(In reply to Issam Mani [:issammani] from comment #7)

Honestly, I am still not convinced we should do this. aboutlogins components will most likely be used only internally by us. [...]

Your use of CSS variables shows that your components are in fact not isolated, and the stylesheets will regularly be maintained outside of your team for years to come e.g. for large-scale redesigns or refactoring efforts.

We handle styling in js all over our codebase in all sorts of justifiable / not so justifiable ways.

Right, so some of these are justifiable, they're exceptions to the rule. Others that aren't justifiable shouldn't be used as an argument for further erosion.

Our use case, however, is justifiable, as it aligns with the recommendations in the lit docs. While having an external stylesheet is fine, I don't see a need for it.

Sorry, mozilla-central standards trump Lit recommendations here. We specifically discussed this topic with the Reusable Components team and reached the conclusion to keep drawing this line between code and styling.

Inlining CSS in a script makes lives of people maintaining our stylesheets harder, see e.g. https://phabricator.services.mozilla.com/D178520#5895693.

I understand this concern, but I think a simple string search in searchfox with all files included would have done the trick.

I did that but apparently overlooked that file. I then didn't edit files one by one but did a local search and replace across the code base, targeting only CSS files to save time, and this worked just fine if it weren't for this single script that doesn't align with our shared code base.

Additionally, if we were to document our design system and include CSS variable names in say markdown files, we would still need to perform a thorough search on all files. From a developer experience perspective, having the CSS inside the component is way better.

I too have a developer perspective here. It may be slightly easier for the original author, but at Mozilla your code could easily live on for a decade, and it's often centralized efforts rather than the original author keeping stylesheets up to date. When someone's not focused on one particular component but rather the theme as a whole e.g. for a redesign, having to wade through tons of scripts in order to find CSS to update or fix (which is often manual labor rather than just search and replace) becomes a major PITA.

I hear you when you say that switching between multiple files can be unappealing. Yet it's something we regularly have to do to organize our code, so that's what I'm asking for here.

(In reply to Sergey Galich [:serg] from comment #2)

When it comes to the CSS for the specific component only I'd prefer it to be as close to the specific component as possible.
If a separate file is so desired, keep it somewhere in aboutlogins folder.

Btw, I believe this means you'd still bypass the desktop-theme-reviewers group, which is not ideal because, again, those are the folks who'll most likely have to play a role in maintaining these stylesheets in the long run, and it's always better to be able to provide feedback early on rather than having to come in after the fact.

You could easily have an aboutlogins folder in browser/themes/, but I agree this current setup isn't ideal. Perhaps we can update the rules for desktop-theme-reviewers to catch all CSS files in browser/ and toolkit/.

We seems to be touching a lot here. Let's get together and figure out what everyone and everybody want or don't want.

I'd like to find a compromise that makes all of us most happy.

I gotta say, I'm troubled with how you're trying to steer this discussion. Here's where we are, the way I see it:

  1. mozilla-central has an established practice of separating stylesheets (not talking about style property access) into CSS files instead of having them in style tags or embedded in scripts. There's been a long-standing issue with files growing too large over time, requiring them to be split up. Generally separating stylesheets from scripts is an easy way to better organize stuff from the start before things get out of hand.
  2. There's necessarily going to be an overlap between your team and external folks working with / maintaining your code, stylesheets in particular.
  3. It's not going to be a blocking a review, but the desktop-theme-reviewers group would like to have the opportunity to glance over stylesheets as they land or get modified to prevent obvious mistakes from creeping in and growing into larger problems over time. However, reviewer group tagging is based on path and file name patterns rather than file contents.
  4. None of the reasons why Lit recommends inlining stylesheets seem to apply to Firefox. (From what I remember, inlining might actually be worse performance-wise as it prevents stylesheets from being shared in memory.)
  5. CSS linting integration with mozilla-central seems to be an unresolved issue.
  6. The reusable components team, who I believe is now the prime user of Lit, has accepted this reality.

The bottom line is, please be good mozilla-central citizens and work with the status quo. Let's avoid adding risks around making current and future theme work any harder. If you want to propose a new set of rules, if you have ideas on how to improve our tooling etc., there's always room for that in the right forum. However, this bug shouldn't be blocked on that.

We are not steering the conversation, rather, engaging in a discussion where we are seeking a resolution that meets both our expectations. Also thank you for your thoughts and insights. Here's the way I see it:

  1. mozilla-central has an established practice of separating stylesheets (not talking about style property access) into CSS files instead of having them in style tags or embedded in scripts. There's been a long-standing issue with files growing too large over time, requiring them to be split up. Generally separating stylesheets from scripts is an easy way to better organize stuff from the start before things get out of hand.

We have been doing it this way all these years, because we didn't have another choice. At one point, we bought into the promise of custom elements and they didn't deliver on that promise. Lit's reason for being is to minimize that friction we have when writing custom elements and one of those is styling. Plus, we are using lit. And lit is not the same as writing custom elements. Having big files is an issue with separation of concerns rather than having a long css string. Long-standing code is simply code written well not organized better.

  1. There's necessarily going to be an overlap between your team and external folks working with / maintaining your code, stylesheets in particular.

Indeed, we will undoubtedly have contributions from external contributors and people from other teams. If it's a simple search and replace, then I don't see an issue with that. If it's more involved than that, then that person probably needs to inspect the html. In that case, a css string is the least of their worries. In general, anyone doing more than a search and replace must have lit knowledge, because we are indeed writing lit elements.

  1. It's not going to be a blocking a review, but the desktop-theme-reviewers group would like to have the opportunity to glance over stylesheets as they land or get modified to prevent obvious mistakes from creeping in and growing into larger problems over time. However, reviewer group tagging is based on path and file name patterns rather than file contents.

This is a valid argument regarding reviews !

  1. None of the reasons why Lit recommends inlining stylesheets seem to apply to Firefox. (From what I remember, inlining might actually be worse performance-wise as it prevents stylesheets from being shared in memory.)

In fact they do. We are writing html inside Firefox. Just by the nature of how stylesheets are loaded, they are not blocking ( for good reason ). And I gave a link above that mentioned FAUC. This might not be noticeable to the user, but it is possible. We are not using any random inlining here, we are using lit's internal mechanism. Styles are static so they are shared by all instances of that component. Plus, we will never get FAUC because lit will always apply the styles upon rendeing. Also if we don't have any benchmarks done on central, I would take lit's word for it.

  1. CSS linting integration with mozilla-central seems to be an unresolved issue.

Again, we are using lit not just inlining css for the sake of it. Any decent linter/formatter for lit will have this baked in. I would use the same argument for html here. Should we write separate html just for linting ?

  1. The reusable components team, who I believe is now the prime user of Lit, has accepted this reality.

What reality exactly are you talking about? The reusable components team is writing components for the entire browser. They have a different set of requirements than ours and need to make different tradeoffs.

please be good mozilla-central citizens and work with the status quo.

I don't think there are bad and good "mozilla-central citizens" here, merely a bunch of people having a discussion on what's best for mozilla-central. The bad would be to not have these discussions at all !

Hopefully, this clarifies things a bit. We are still discussing things internally.

(In reply to Issam Mani [:issammani] from comment #12)

We are not steering the conversation, rather, engaging in a discussion where we are seeking a resolution that meets both our expectations.

We're not breaking new ground here, and rules are more often than not implied by how our shared code base works. We can't have this sort of week-long discussion every time someone feels like doing things differently. Again, if you want to propose a change of policy, that should happen in a different forum and format.

  1. There's necessarily going to be an overlap between your team and external folks working with / maintaining your code, stylesheets in particular.

Indeed, we will undoubtedly have contributions from external contributors and people from other teams. If it's a simple search and replace, then I don't see an issue with that. If it's more involved than that, then that person probably needs to inspect the html. In that case, a css string is the least of their worries.

They may or may not have to look at the DOM. Usually that can happen independently from looking at scripts or markup, thanks to tools like the Inspector.

In general, anyone doing more than a search and replace must have lit knowledge, because we are indeed writing lit elements.

We're familiar with Lit. Some more, some less. From what I can tell we're dealing with standard CSS as far as Lit is concerned, so people touching theming won't in fact need to be experts in Lit, but regardless, this seems orthogonal to the question of whether stylesheets should be inlined.

  1. It's not going to be a blocking a review, but the desktop-theme-reviewers group would like to have the opportunity to glance over stylesheets as they land or get modified to prevent obvious mistakes from creeping in and growing into larger problems over time. However, reviewer group tagging is based on path and file name patterns rather than file contents.

This is a valid argument regarding reviews !

Okay, so how many reasons that you think are valid do we need before we can move forward here?

  1. None of the reasons why Lit recommends inlining stylesheets seem to apply to Firefox. (From what I remember, inlining might actually be worse performance-wise as it prevents stylesheets from being shared in memory.)

In fact they do. We are writing html inside Firefox. Just by the nature of how stylesheets are loaded, they are blocking ( for good reason ).

We're not loading stylesheets remotely so tradeoffs won't exactly be the same.

And I gave a link above that mentioned FAUC.

That's for Storybook which isn't part of Firefox.

This might not be noticeable to the user, but it is possible.

Not for Firefox from what I can tell.

We are not using any random inlining here, we are using lit's internal mechanism. Styles are static so they are shared by all instances of that component.

Will they be shared in memory though?

  1. CSS linting integration with mozilla-central seems to be an unresolved issue.

Again, we are using lit not just inlining css for the sake of it. Any decent linter/formatter for lit will have this baked in. I would use the same argument for html here.

I actually don't know how well our current linter works with Lit. Have you checked?

Should we write separate html just for linting ?

Linting is / at this point needs to be part of the story. It's not the only reason why we'd currently want to organize stuff in separate files, but it's one of the questions you'll need to answer when proposing a new policy.

  1. The reusable components team, who I believe is now the prime user of Lit, has accepted this reality.

What reality exactly are you talking about?

The reality of how stylesheets are organized in mozilla-central.

The reusable components team is writing components for the entire browser. They have a different set of requirements than ours and need to make different tradeoffs.

Whether you're writing a domain-specific component or a shared one is largely a distinction without a difference e.g. when it comes to theme work potentially touching the entire browser, which your component is still part of.

Prettier appears to support lit out of the box. I tried reformatting the css block in the current login-timeline.mjs file, introducing the formatting issues Itiel pointed out in the patch on this bug and both of those issues were already auto-fixed by prettier. Likewise prettier will auto-fix the HTML formatting in html tagged template strings.

We don't have FOUC in other parts of Firefox when using external stylesheets, but this doesn't appear to be the case in about:logins. I think we load stylesheets in a blocking manner in main process/privileged windows, but about:logins is not one of those. With Micah's patch loaded I see big flashes of a dark circle SVG quite regularly when reloading.

I believe the static stylesheet is shared in memory, I seem to have lost the history of my discussions about this with Emilio, but I believe the bonus of using the stylesheet was that each instance would share changes to that stylesheet. Whereas with Constructable Stylesheets the main stylesheet is shared, but if an instance modifies it then it gets copied and only that instance is changed. I don't think we want to be modifying the stylesheets themselves in JS, so I think it's probably moot here.

For the Reusable Components team, that code is intended to be shared across all parts of Firefox, and is meant to integrate into our current systems. We didn't have the same perf impact of using external stylesheets, so using external stylesheets made sense to integrate with the existing theme code there.

Personally, I wouldn't be upset about the styles for the reusable components moving into the .mjs file. For example moz-button-group.css is only 9 lines of CSS. For something larger like moz-toggle.css (~200 lines of CSS) then maybe a stylesheet makes sense. Perhaps there's a balance, similar to moz-support-link being a vanilla custom element (extending HTMLAnchorElement) but other components being lit components. But until we get CSS Module Scripts (bug 1720570) then those components will continue to have FOUC in about:logins (or potentially if we get our blocking stylesheet loading added to about:logins somehow).

I'm not going to say that CSS in the JS file is better, but I'm nervous to tell someone they can't try new things. This isn't inline styles in JS objects attached to JSX elements. It's the exact same CSS, just in a different file.

(In reply to Mark Striemer [:mstriemer] from comment #14)

We don't have FOUC in other parts of Firefox when using external stylesheets, but this doesn't appear to be the case in about:logins. I think we load stylesheets in a blocking manner in main process/privileged windows, but about:logins is not one of those. With Micah's patch loaded I see big flashes of a dark circle SVG quite regularly when reloading.

For what it's worth, I'm seeing FOUC on Nightly for the sidebar when I hard reload/bypass the cache. Maybe there's something going on here with about:logins and external stylesheets?

With Micah's patch pulled down I am occasionally seeing the large dark circle SVG when I hard reload about:logins. Maybe 10% or 20% of the time if I had to put a number on it...

Already a lot of people weighing in here, but the changes in this patch might be of interest / relevant to this discussion. Mark already mentioned this in passing, but when we initially implemented moz-toggle with an external stylesheet we were seeing a FOUC. Emilio had to implement this platform level workaround to ensure the styles load synchronously, which fixed our issue.

I'm not familiar enough with about:logins code to know for sure if the workaround applies here, but based on the fact that we're seeing FOUC and the fix seems scoped to chrome document shadow trees I'm thinking it doesn't. If we do want to use external stylesheets in about:logins we might have to do something similar or expand the scope of that workaround.

Depends on: 1836818
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/469e5fce6145
Move login timeline inline CSS to a separate CSS file. r=dao,credential-management-reviewers,sgalich

https://hg.mozilla.org/mozilla-central/rev/469e5fce6145

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
status-firefox116: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch
You need to log in before you can comment on or make changes to this bug.