ui: remove unused style rules

[cdamus]

A number of style rules that actually are no longer needed were leaking
into applications that embed the Perfetto UI. So these are deleted.
Add a 'perfetto' class to the document body to ensure proper confinement
of the UI with internal scrolling instead of scrolling all content.

Fixes #2166

[cdamus]

@stevegolton I see the same test failures in the integration tests as on current main branch, mostly having to do with the indentation of grouped tracks. I don't see obvious regressions otherwise.

[stevegolton]

I suspect we might be able to get rid of this button style entirely actually!

[stevegolton]

I think in general most of these styles in common.scss can go and nothing uses them any more. Let me confirm this...

Also, in terms of using the pf-overlay-container as a general perfetto container - the overlay container was really for placing overlays like popups and tooltips rather than attaching them to the body, so that they don't leak out into the containing application. In the spirit of the SRP I'm not convinced I'd like to attach another responsibility to this class. In fact I'd prefer a general .perfetto container like you did in your other PR.

I am however a little surprised about these leaks and the work I have done was in an attempt to avoid needing a .perfetto container at all.

Anyway, please see the comments.

[stevegolton]

And this one

[stevegolton]

We could probably get rid of these too, I think. Let me check...

[cdamus]

In testing I found that the heading styles are actually needed: see commit dc4e3f5

[stevegolton]

OK I think most of the styles in this file are redundant to be honest.

[stevegolton]

@stevegolton I see the same test failures in the integration tests as on current main branch, mostly having to do with the indentation of grouped tracks. I don't see obvious regressions otherwise.

Eh, I'm not sure I follow you here? The integration tests look good on this PR + on main. Am I missing something?

[cdamus]

I think in general most of these styles in common.scss can go and nothing uses them any more. Let me confirm this...

That would be great. I never know to what extent other bits of the Perfetto app that I'm not familiar with might be relying on things that seem obsolete.

Also, in terms of using the pf-overlay-container as a general perfetto container - the overlay container was really for placing overlays like popups and tooltips rather than attaching them to the body, so that they don't leak out into the containing application. In the spirit of the SRP I'm not convinced I'd like to attach another responsibility to this class. In fact I'd prefer a general .perfetto container like you did in your other PR.

Yes, that makes sense. Here I was trying to minimize the impact of my changes by not introducing new elements, but if you have other reasons to introduce another container, then that's even better for my use case 😀

I am however a little surprised about these leaks and the work I have done was in an attempt to avoid needing a .perfetto container at all.

Yeah, there weren't many leaks remaining and it sounds like they are all from rules that can be deleted ... ? wbn

[cdamus]

@stevegolton I see the same test failures in the integration tests as on current main branch, mostly having to do with the indentation of grouped tracks. I don't see obvious regressions otherwise.

Eh, I'm not sure I follow you here? The integration tests look good on this PR + on main. Am I missing something?

When I run the integration tests on my machine, there are 11 or 12 failures on both main branch and this. This is the first of my PRs that I see the checks actually complete and report results in the PR. And, of course, it's nice to see that they are green 😉

[stevegolton]

Aha, gotcha. Yes unfortunately the golden screenshot tests are not particularly portable!

[cdamus]

OK, thanks a lot for your feedback @stevegolton . I've updated the PR to implement what I think is a cleaner solution more closely aligned to your goals.

• remove a bunch of style rules that seem to be unused instead of further qualifying them
• move some button styling that is needed by pf-button into the rule for that class. Otherwise, all of the little buttons everywhere get light grey backgrounds and heavy borders
• set a perfetto class on the document body to fit the content to the viewport with internal scrolling. It isn't sufficient to do this on some container div because the body element has special semantics as relates to the viewport. This is OK for my embedding use case because my application embeds the entire Perfetto UI in a div that is statically sized

[stevegolton]

OK, thanks a lot for your feedback @stevegolton . I've updated the PR to implement what I think is a cleaner solution more closely aligned to your goals.

• remove a bunch of style rules that seem to be unused instead of further qualifying them
• move some button styling that is needed by pf-button into the rule for that class. Otherwise, all of the little buttons everywhere get light grey backgrounds and heavy borders

I think it's the right idea to put the styles into the widgets/components that actually need them, but I think you've missed a few.

• menu.scss - the menu items are <button>s
• perf_monitor.scss - has some custom styled buttons (these should really just be replaced with the Button widget but one step at a time).

Also I don't think this list of properties you've added to pf-button properly resets the browser styling (at least on chrome on macos), you need:

  font: inherit;
  color: inherit;
  // and maybe...
  appearance: none;

Maybe a @mixin would help if we need to repeat those styles everywhere.

I think the table user-select styles are not required on modern browsers so they can go.

The button.close style I don't think is used anywhere so that's fine.

I'm not sure about the h1, h2, etc - I haven't checked those yet.

• set a perfetto class on the document body to fit the content to the viewport with internal scrolling. It isn't sufficient to do this on some container div because the body element has special semantics as relates to the viewport. This is OK for my embedding use case because my application embeds the entire Perfetto UI in a div that is statically sized

I was going to ask about that - I'm guessing you change the initialization in typescript for your build because if it ran verbatim it would add the perfetto class to your main application's body which would cause issues. I mean it currently also clears the body and mounts Mithril in there, so I guess you must change it!

Assuming you have a conditional build anyway, I was thinking maybe we could conditionally just exclude common.scss (or rather some other smaller sass file with just the problematic styles) for embedded builds?

[cdamus]

I think it's the right idea to put the styles into the widgets/components that actually need them, but I think you've missed a few.

• menu.scss - the menu items are <button>s
• perf_monitor.scss - has some custom styled buttons (these should really just be replaced with the Button widget but one step at a time).

Thanks for the pointers, and for letting the UI integration test action run. I'll track these down. Yes, I expect a mixin will help.

The button.close style I don't think is used anywhere so that's fine.

Yes, I found no trace of it.

I'm not sure about the h1, h2, etc - I haven't checked those yet.

We could qualify these with the .perfetto ancestor class otherwise, perhaps.

• set a perfetto class on the document body to fit the content to the viewport with internal scrolling. It isn't sufficient to do this on some container div because the body element has special semantics as relates to the viewport. This is OK for my embedding use case because my application embeds the entire Perfetto UI in a div that is statically sized

I was going to ask about that - I'm guessing you change the initialization in typescript for your build because if it ran verbatim it would add the perfetto class to your main application's body which would cause issues. I mean it currently also clears the body and mounts Mithril in there, so I guess you must change it!

In my application I'm working on the basis of #2267, which skips the instantiation of the UI in the main() start-up when there is an EmbedderContext present from an embedding application. So my application subsequently takes care of instantiating UiMainPerTraces in the new ThemeProvider structure in multiple subtrees of the DOM.

Assuming you have a conditional build anyway, I was thinking maybe we could conditionally just exclude common.scss (or rather some other smaller sass file with just the problematic styles) for embedded builds?

Thanks to your work on the theme provider, there is now no conditional or other compile-time variation point. My application just takes the Perfetto UI build as is 😀

[cdamus]

Commit 74f8c69 uses a new button-base mixin to pull base styles into button rules that need it. This includes, as it turns out, also the cookie-consent UI that like the perf_monitor is not yet using the button widget.

[cdamus]

And the heading styles were needed for selection tab and more, so those are restored as I suggested with the perfetto ancestor class in commit dc4e3f5.

[stevegolton]

Sorry for the late reply, I've been OOO for a couple days.

I've been ruminating on what the best approach is for sorting out these styles. I'm not convinced the .perfetto class is the right thing necessarily for some of the styles, though it probably will be needed for the final html/body overriding styles.

I've hacked around a bit and tidied up a few messy parts in #2864. The general approach I've taken is to move everything into the UIMain component, so that if used in isolation without the ThemeProvider then everything will work except the theme scss variables obviously.

I've moved the catch-all button, h[1,2,3,4,5,6], pre, code, and box-sizing into .pf-ui-main, and moved the OverlayContext inside UIMain too so that popups still get the right styling. I've used :where(...) to specify the elements which has the same specificity as the widget level selectors, but they appear after so they take precedence.

I think this replaces the need for the .perfetto class for a lot of these styles, as .pf-ui-main is functionally identical now as it's the main perfetto container element.

Next I've moved the various variables attached to :root into pf-ui-main too - things like the font, sidebar-width, etc. I'm not sure if they were causing issues when embedded but it makes sense to contain them too.

Next, I've moved the various Sass mixins into theme.scss (which is now more of a badly named 'utils' file).

Next I've moved the filedrag styles into UIMain too, which probably weren't causing issues but it makes sense to contain them anyway.

Finally I've cleaned up the html, body styles to what I think is the bare minimum.

Now, this isn't perfect as the html an body styles still exist, but I think everything is a lot neater now.

Again, apologies for just duplicating a lot of what's in PR, but it's the sort of job you just need to poke around and get a feel for, which is a lot easier than me trying to guide you through it, so I hope you don't mind!

I think it would be best if, once I've landed #2864, if you rebase on top of that, and that'll give us a much more stable ground on which to finalize the last pieces of the puzzle.

[cdamus]

Sorry for the late reply, I've been OOO for a couple days.

Absolutely no need to apologize! Thanks for the update.

I've hacked around a bit and tidied up a few messy parts in #2864. The general approach I've taken is to move everything into the UIMain component, so that if used in isolation without the ThemeProvider then everything will work except the theme scss variables obviously.

OK, cool. I was using the ThemeProvider to inject my application's fonts and colours. I don't imagine that it won't still work 😀

I've moved the catch-all button, h[1,2,3,4,5,6], pre, code, and box-sizing into .pf-ui-main, and moved the OverlayContext inside UIMain too so that popups still get the right styling. I've used :where(...) to specify the elements which has the same specificity as the widget level selectors, but they appear after so they take precedence.

Yes, that sounds good. :where is a nice escape hatch for the specificity inversion that I ran into.

Next I've moved the various variables attached to :root into pf-ui-main too - things like the font, sidebar-width, etc. I'm not sure if they were causing issues when embedded but it makes sense to contain them too.

They weren't a problem as I obviously don't use them in my app, but neither will moving them be a problem.

Next I've moved the filedrag styles into UIMain too, which probably weren't causing issues but it makes sense to contain them anyway.

Yeah, that wasn't a problem, but agreed that this is cleaner.

Finally I've cleaned up the html, body styles to what I think is the bare minimum.

Now, this isn't perfect as the html an body styles still exist, but I think everything is a lot neater now.

I think they were mostly compatible with my application anyways, which has similar space-filling requirements.

Again, apologies for just duplicating a lot of what's in PR, but it's the sort of job you just need to poke around and get a feel for, which is a lot easier than me trying to guide you through it, so I hope you don't mind!

Again, no apology needed. I appreciate the effort that you're putting into this. I've been trying too hard to fit my proposed changes into the current structure and I certainly don't mind you taking the helm when you're willing to blow it all up, as it were.

I think it would be best if, once I've landed #2864, if you rebase on top of that, and that'll give us a much more stable ground on which to finalize the last pieces of the puzzle.

Absolutely. I'll be happy to validate those changes for my use case and follow up for any remaining gaps, here! Thanks so much.

[stevegolton]

OK, cool. I was using the ThemeProvider to inject my application's fonts and colours. I don't imagine that it won't still work 😀

Ahh I hadn't imagined you'd be interested in changing the fonts - in that case I'll put them in the theme provider. The idea is that you only need to remove the ThemeProvider in order to get a raw UI that you can embed and theme.

Good, sound like everything else should be pretty much there aside from the bare html/body styles, but we're getting closer! Thanks for bearing with me.