1502080 - Having the sort order of about:performance change while attempting to click the close button is annoying

Status: VERIFIED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[Florian Quèze [:florian]]

This makes it too easy to close the wrong tab.

Comment 1

[(no longer active)]

Comparing to native task managers, here is a bag of random ideas for how this could be improved:

  * Support displaying what column the results are sorted by (perhaps using a chevron or something like that)
  * Support selecting all rows.  Right now we don't support selecting subframe rows which makes it difficult to highlight one and find it quickly as the order changes.
  * Support changing the sort order by different columns (e.g. by name).  That should help make the sort order much more stable.

Comment 2

[Florian Quèze [:florian]]

(In reply to :Ehsan Akhgari from comment #1)

>   * Support displaying what column the results are sorted by (perhaps using
> a chevron or something like that)
>   * Support changing the sort order by different columns (e.g. by name). 
> That should help make the sort order much more stable.

These 2 points are covered by bug 1498187.

>   * Support selecting all rows.  Right now we don't support selecting
> subframe rows which makes it difficult to highlight one and find it quickly
> as the order changes.

That's something that could be improved indeed. I didn't care about selecting these rows because they are not actionable, but the case you describe makes sense. It's an edge case though, as these lines are only visible if you expended another line using the ">" icon.

Comment 3

[(no longer active)]

(In reply to Florian Quèze [:florian] from comment #2)
> >   * Support selecting all rows.  Right now we don't support selecting
> > subframe rows which makes it difficult to highlight one and find it quickly
> > as the order changes.
> 
> That's something that could be improved indeed. I didn't care about
> selecting these rows because they are not actionable, but the case you
> describe makes sense. It's an edge case though, as these lines are only
> visible if you expended another line using the ">" icon.

How is it an edge case?  My impression was that one of the goals of the design was to make the cost of things like trackers or workers visible (which is why we're showing them in the first place).  Perhaps I'm misunderstanding the ultimate goal of the design.  But using the about:performance interface as a user it seemed to me that Firefox is actively preventing the selection of those rows and I couldn't think of a reason why the user should be stopped from doing that if that's what they want to do.

(FWIW I tend to select things for reading because I find it difficult to follow long lines otherwise, and every time software prevents me from selecting something I shake my fists at the sky because it hurts my ability to read things comfortably on the screen!)

Comment 4

[Florian Quèze [:florian]]

(In reply to :Ehsan Akhgari from comment #3)
> (In reply to Florian Quèze [:florian] from comment #2)
> > >   * Support selecting all rows.  Right now we don't support selecting
> > > subframe rows which makes it difficult to highlight one and find it quickly
> > > as the order changes.
> > 
> > That's something that could be improved indeed. I didn't care about
> > selecting these rows because they are not actionable, but the case you
> > describe makes sense. It's an edge case though, as these lines are only
> > visible if you expended another line using the ">" icon.
> 
> How is it an edge case?  My impression was that one of the goals of the
> design was to make the cost of things like trackers or workers visible
> (which is why we're showing them in the first place).  Perhaps I'm
> misunderstanding the ultimate goal of the design.

The main design goal was to help users find which tab is burning resources, so that they can blame the tab instead of Firefox.

Showing trackers was just something that was so easy that it I couldn't refrain from doing it (it wasn't part of the designs, it's just something I added because it seemed interesting).

Initially I had all the subitems shown by default. It's a UX request to collapse them by default. I don't expect most users to click the '>' icons to collapse each of them. But some users will do it, like you :-).

> But using the
> about:performance interface as a user it seemed to me that Firefox is
> actively preventing the selection of those rows and I couldn't think of a
> reason why the user should be stopped from doing that if that's what they
> want to do.

It's actually an implementation detail that ends up being unintentionally exposed. The selection is remembered across refreshes by remembering the id of the tab. Subframes and workers have no tab id, so they can't be selected. I could fix this by changing the implementation if we decide this is important. At the time I implemented this, tab ids were the best unique ids I had; now each perf counter as a unique id that I could use instead.

> (FWIW I tend to select things for reading because I find it difficult to
> follow long lines otherwise, and every time software prevents me from
> selecting something I shake my fists at the sky because it hurts my ability
> to read things comfortably on the screen!)

The hover effect is there to help reading the long lines.

Comment 5

[(no longer active)]

(In reply to Florian Quèze [:florian] from comment #4)
> (In reply to :Ehsan Akhgari from comment #3)
> > (In reply to Florian Quèze [:florian] from comment #2)
> > > >   * Support selecting all rows.  Right now we don't support selecting
> > > > subframe rows which makes it difficult to highlight one and find it quickly
> > > > as the order changes.
> > > 
> > > That's something that could be improved indeed. I didn't care about
> > > selecting these rows because they are not actionable, but the case you
> > > describe makes sense. It's an edge case though, as these lines are only
> > > visible if you expended another line using the ">" icon.
> > 
> > How is it an edge case?  My impression was that one of the goals of the
> > design was to make the cost of things like trackers or workers visible
> > (which is why we're showing them in the first place).  Perhaps I'm
> > misunderstanding the ultimate goal of the design.
> 
> The main design goal was to help users find which tab is burning resources,
> so that they can blame the tab instead of Firefox.
> 
> Showing trackers was just something that was so easy that it I couldn't
> refrain from doing it (it wasn't part of the designs, it's just something I
> added because it seemed interesting).
> 
> Initially I had all the subitems shown by default. It's a UX request to
> collapse them by default. I don't expect most users to click the '>' icons
> to collapse each of them. But some users will do it, like you :-).

I see, thanks for the background.

> > But using the
> > about:performance interface as a user it seemed to me that Firefox is
> > actively preventing the selection of those rows and I couldn't think of a
> > reason why the user should be stopped from doing that if that's what they
> > want to do.
> 
> It's actually an implementation detail that ends up being unintentionally
> exposed. The selection is remembered across refreshes by remembering the id
> of the tab. Subframes and workers have no tab id, so they can't be selected.
> I could fix this by changing the implementation if we decide this is
> important. At the time I implemented this, tab ids were the best unique ids
> I had; now each perf counter as a unique id that I could use instead.

OK.

In case you decided to fix this, one heuristic that you can use in order to compute a "unique ID" for iframes is to compute an XPath expression for how to access them in the DOM.  We have some code for this somewhere in devtools inspector view that may be possible to factor out (click on a node and select Copy -> XPath.)

For workers, a "unique ID" can be computed from their src URL and their nesting level (for nested dedicated workers).  For shared workers, of course, you should also look at the name of the worker in addition.

> > (FWIW I tend to select things for reading because I find it difficult to
> > follow long lines otherwise, and every time software prevents me from
> > selecting something I shake my fists at the sky because it hurts my ability
> > to read things comfortably on the screen!)
> 
> The hover effect is there to help reading the long lines.

Right, but see the summary.  :-)  With the sort order changing, the hover effect can only be used for this purpose for a few seconds in most circumstances.

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch

Bryan suggested that we freeze the sort order for 5s when the user moves the mouse. This is pretty simple and seems to solve the problem.

I was initially thinking we would also need to freeze the sort order when hovering action buttons, but that's not really needed as clicking a button without moving the mouse even slightly is pretty rare.

During the 5s after a mouse move event, the values in the Energy Impact column are still updated, but the subitem aren't updated. Not updating the list of children is to avoid causing motion, and not updating the Energy Impact values for children is because it would have complicated the implementation (I would need to add unique ids on child rows) for an edge case that doesn't really matter.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 9021643 [details] [diff] [review]
Patch

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

Looks sane to me. I thought this would be more complicated than it was.  Although it means that new tabs that show up (or close) automatically (e.g. a new popup window opened from an authorized tab) won't show up either, right?

I think it would be nicer to shorten the 5s delay, but also keep track (with mouseenter/mouseleave) if the mouse is on a close tab icon to hold the sort indefinitely. But that brings its own problems (is there a chance that mouseleave would not be fired?), so you don't need to do it here if you'd rather not.

Comment 8

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes (needinfo me!) from comment #7)

> Looks sane to me. I thought this would be more complicated than it was.

Thanks! I was also surprised that I managed to do it with so few lines of code. Of course ignoring subitems helped a lot.

> Although it means that new tabs that show up (or close) automatically (e.g.
> a new popup window opened from an authorized tab) won't show up either,
> right?

Correct. I think that's rare edge cases. I was initially thinking I would add new items at the bottom of the list of avoid messing with the order... but I suspect nobody will ever see them at the bottom.

> I think it would be nicer to shorten the 5s delay

The 5s delay seems pretty reasonable to me when trying to use it. The display is refreshed every 2s, so a 5s delay means we refresh twice without changing the sort order before we start sorting again.

The one case where the 5s delay might feel long is when scrolling I guess. If a user scrolls around we'll constantly have mouseevents and may end up not adjusting the order for a long while. I'll need to test it with my real profile for a couple days to see how I feel about it.

> also keep track (with
> mouseenter/mouseleave) if the mouse is on a close tab icon to hold the sort
> indefinitely. But that brings its own problems (is there a chance that
> mouseleave would not be fired?), so you don't need to do it here if you'd
> rather not.

I was initially thinking we would need to do this, but it seems to work well enough without it. And given I intend to request uplift on this patch, I would rather keep it strictly simple.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d34998a3d7d
Freeze the sort order of about:performance for 5s when the mouse is moved, r=felipe.

Comment 10

[Florian Quèze [:florian]]

I intend to request uplift to 64 after letting this bake on Nightly for a few days. This is the most commonly reported annoyance in the about:performance page as we have it right now on 64.

Comment 11

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/4d34998a3d7d

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 9021643 [details] [diff] [review]
Patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1498185

User impact if declined: Users may close the wrong tab

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: The patch is safe, I checked "Needs manual test" only because we don't have automated test coverage.

List of other uplifts needed: Bug 1504709

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Straight forward patch

String changes made/needed: none

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9021643 [details] [diff] [review]
Patch

ux improvement for new about:performance, approved for 64.0b8

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c7a40a87b9a

Comment 16

[Gyula Palko[:gpalko]]

Verified on Nightly 65(20181112100105) and Beta 64b8(20181108141956), that sort order is frozen for 5s after mouse move/click events.