1016288 - CSS coverage needs buttons in the Style Editor UI

Status: NEW

(DevTools :: Style Editor, enhancement, P3)

Description

[Joe Walker [:jwalker]]

Probably a button that says "Start Coverage", which changes to "Stop Coverage" when coverage is running, and when results are available changes to 2 buttons that say "Show summary" and "Clear results".

Pressing "Clear results" changes the button back to one that says "Start Coverage".

Comment 1

[Joe Walker [:jwalker]]

Here's how I think it could work:

Initial state: No results, coverage not running. 1 button [Start Coverage], which when pressed, does:

    const csscoverage = require("devtools/server/actors/csscoverage");

    let usage = yield csscoverage.getUsage(target);

    yield usage.start(chromeWindow, target);

Running state: 1 button [Stop Coverage], which does:

    yield usage.stop();

Results state: 2 buttons [Clear Results] takes us back to the initial state, and [Show Summary] which does:

    yield usage.createPageReport()
    // Call the csscoveragePageReport->dom converter
    // from toolkit/devtools/gcli/commands/csscoverage:125
    // will need factoring out into a separate function

To change from initial state to running state, and running state to results state we do:

    usage.on("state-change", ev => {
      if (ev.isRunning) state = running;
      else state = results;
    });

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch

Still need to add tests, but I'd like to get your first thoughts on the UI and the code as well.

Comment 3

[Tim Nguyen :ntim]

Attachment: Patch

Fixed the csscoverage report command not working.

Apart from the tests, this is pretty much ready for review.

Comment 4

[Tim Nguyen :ntim]

Attachment: screencast

Comment 5

[Tim Nguyen :ntim]

Attachment: Part 1 : Add a coverage button to the style editor

A couple of fixes

Comment 6

[Tim Nguyen :ntim]

Attachment: Part 2 : Test

The window is leaking, but the checks are passing.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8603706 [details] [diff] [review]
Part 1 : Add a coverage button to the style editor

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

I don't know a ton about the css coverage implementation so it's not possible for me to say if this is a good separation of logic between the two tools - but the changes look pretty mechanical and simple.  I also would want to get UX review for where / when the button shows up in the UI and for what the interaction is like while the command is running (I'm guessing we'd want things to behave similar to other kinds of long running commands like the profiler).

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +922,5 @@
> +   * Toggles CSS coverage
> +   */
> +  _toggleCoverage: Task.async(function *() {
> +    if (!this._coverageButton.checked) {
> +      this._coverageButton.checked = true;    

Nit: trailing whitespace

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 8603706 [details] [diff] [review]
Part 1 : Add a coverage button to the style editor

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

Thanks!

I'm happy with the code, but wanted to stop and think about the interaction: We click 'start' tests run. Click 'stop', report automatically shows. All good so far. I think start does an implicit clear anyway, so I can't think why the original plan was for 2 buttons, but this seems ok to me.

The screencast doesn't show an infobar while the test is running, which it should. actors/csscoverage.js:777 does this, maybe it's just cut off the bottom of the video?

I think we should fix bug 1034062 and bug 1013356 before we land this (in other words the things in the meta bug 1013886). And we should remove the 'hidden: true' from the commands as part of making it visible.

Comment 9

[Tim Nguyen :ntim]

Attachment: Part 1 : Add coverage button (v2)

2 changes :
- Folded the patch from bug 1045619
- Now disables the button when clicking on it to avoid an edge case

Comment 10

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 11

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 12

[Tim Nguyen :ntim]

Attachment: Part 1: Add coverage button (v3) (r=jwalker, bgrins)

Comment 13

[Tim Nguyen :ntim]

Attachment: Part 2: Test

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8770727 [details] [diff] [review]
Part 2: Test

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

I'd like to put this on hold pending a product / UX review of the feature.  Specifically:

1) if it hit a certain quality bar, would we want this surfaced in the product
2) do we want to prioritize this over other work to get it to hit said quality bar

Comment 15

[Brian Grinstead [:bgrins]]

Helen, Bryan, see Comment 14.  Happy to have a call or chat on IRC if that's easier for discussing

Comment 16

[Tim Nguyen :ntim]

Note that we can always land this button hidden, so we can focus on fixing the quality bugs before deploying the feature to the public.

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

I’ve said before that I think the gcli has a lot of cool features that would be nice to service in the UI better, so, philosophically, I’d like to see this in the UI. I do have some questions and UX-review:

Looking over the CSS Coverage MDN page, I’m told that I should press start, visit a representative number of pages in my website, and then hit stop for a full analysis. Should we consider having the csscoverage oneshot command be the default, with this “recording” paradigm being the second, less-used one? I ask because while I think we’re used to recording for this like performance, it’s perhaps not as common a pattern in the UI-Engineering part of devtools. I think this would also handle the issue that pressing the performance button doesn’t make it obvious you’re supposed to roam around and traverse pages—but, assuming we do use the csscoverage start/stop commands somewhere, somehow, I think it’d be a good idea to have that message displayed somewhere.

A small thing, but I wonder if we can make the connection between “CSS Coverage” and “Critical CSS” for our users, since I believe that one informs the others. Googling “CSS Coverage” gets me the MDN page for this tool, while googling “Critical CSS” gets me lots of tutorials from Chris Coyier, Smashing Mag, etc. Like Brian I thought I didn’t understand anything about CSS Coverage until I’d read through the docs, but I’d definitely heard and implemented critical CSS before.

At this point we get into things I don’t have the bandwidth to solve right now, but the kind of modal we’re using to show this information audit was one of the first things dev rel pointed out to me as being both undiscoverable and “weird” in the Network panel.

I think this was also because people reacted badly to the layout of information in the panels, even when the information itself was pretty good. I don’t think solving either of these issues is hard, it’s just not on my radar of things to accomplish this quarter.

(In reply to Brian Grinstead [:bgrins] from comment #14)
> I'd like to put this on hold pending a product / UX review of the feature.
> Specifically:
>
> 1) if it hit a certain quality bar, would we want this surfaced in the
> product
> 2) do we want to prioritize this over other work to get it to hit said
> quality bar

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> Note that we can always land this button hidden, so we can focus on fixing
> the quality bugs before deploying the feature to the public.

I’d like Bryan to weigh in on prioritization.

To address Tim’s point, though, if it's behind a flag, will it get worked on?

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

I agree with Helen on the one-shot approach, it feels awkward otherwise.

> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > I'd like to put this on hold pending a product / UX review of the feature.
> > Specifically:
> >
> > 1) if it hit a certain quality bar, would we want this surfaced in the
> > product

Do you mean the UX in general or is the data its returning is poor?  If its the former I'd be willing to try surfacing it assuming we could fix up some of the immediate concerns Helen has.  If the information it provides is incorrect then there's little we'll learn from shipping it.

> > 2) do we want to prioritize this over other work to get it to hit said
> > quality bar

It is unique and interesting.  But I think this falls under the visual tools work that we have planned for later in the year.  I don't think we have the bandwidth to take this on while we're doing the devtools.html.

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> > Note that we can always land this button hidden, so we can focus on fixing
> > the quality bugs before deploying the feature to the public.
> 
> I’d like Bryan to weigh in on prioritization.
> 
> To address Tim’s point, though, if it's behind a flag, will it get worked on?

I don't like preffing things off because we just spend lots of marketing time trying to educate people on how to turn it on so we can then get feedback.  If this provides real value to our developers, and I believe it does though I haven't played with it yet, we should consider shipping it and fixing up the issues down the road.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #18)
> I agree with Helen on the one-shot approach, it feels awkward otherwise.
> 
> > (In reply to Brian Grinstead [:bgrins] from comment #14)
> > > I'd like to put this on hold pending a product / UX review of the feature.
> > > Specifically:
> > >
> > > 1) if it hit a certain quality bar, would we want this surfaced in the
> > > product
> 
> Do you mean the UX in general or is the data its returning is poor?  If its
> the former I'd be willing to try surfacing it assuming we could fix up some
> of the immediate concerns Helen has.  If the information it provides is
> incorrect then there's little we'll learn from shipping it.

I think that it's UX in general, especially if we are doing a recording. Like, what happens if you navigate to a different site, or a different page on your site that includes different stylesheets altogether.  And how do we show results from files that aren't included on the page that you ended up on (iirc this uses the style editor UI to show files).

We'd need to take a deeper look into it to be sure.  It would be especially good to try this out for some sample use-cases on discovering unused CSS and see how accurate it is / how the UI works / if "recordings" are necessary.

Comment 20

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> I think that it's UX in general, especially if we are doing a recording.
> Like, what happens if you navigate to a different site, or a different page
> on your site that includes different stylesheets altogether.  And how do we
> show results from files that aren't included on the page that you ended up
> on (iirc this uses the style editor UI to show files).
Tim, since you did the original work, have you given any thought to these scenarios? At this point you probably would have a better idea of what should or should not be done.

> We'd need to take a deeper look into it to be sure.  It would be especially
> good to try this out for some sample use-cases on discovering unused CSS and
> see how accurate it is / how the UI works / if "recordings" are necessary.
Unused CSS is generally an issue that's pretty difficult to solve—build tools often have packages for this sort of thing but they only work well on static sites. It's definitely a problem that lots of people are trying to solve.

Comment 21

[Tim Nguyen :ntim]

Unassigning to reflect real status.