Improve EventListener UI

[harshlele]

#7879 - Improve UI implementation of EventListeners list

Summary of Changes

• Moved the arrow icon to outside the label, so when it's clicked, it doesn't trigger the checkbox
• Fixed the vertical alignment of the checkbox and text inside the label
• Fixed height of rows according to UI spec(at 20px)
• Wrapped a button element around the arrow icon so it can be used with keyboard

[jaril]

@fvsch Correct me if I'm wrong, but the arrow referenced in the mockup seems to be based on an older ~8x9px arrow, but it was changed to 10x10px recently.

Is the intent to keep things as similar to the mockup as possible? If so you'd probably want something like this below for the checkbox:

margin-inline-start: 6px /* instead of the mockup's 8px */
margin-inline-end: 4px

Apart from that I just played with it and it looks good - great job @harshlele!

[fvsch]

Correct me if I'm wrong, but the arrow referenced in the mockup seems to be based on an older ~8x9px arrow, but it was changed to 10x10px recently.

So it's the same arrow, in the images folder (and similarly in mozilla-central's devtools/client/themes/images) it's drawn on a 10px viewBox, with some blank pixels on the sides. It's drawn like that for ease of use and visual balance when rotating it; it could theoretically be a 8x8 canvas but that doesn't change much.

So the red/blue lines drawn by Matt have to be taken with a grain of salf because they encore visual intent, which may not be the same as the actual distance between two DOM nodes.

[fvsch]

I'm looking at the PR, and it's a good improvement overall.

This point was is not addressed in the PR yet:

• the arrow icon should be a button so that it can be focusable; we probably need to come up with an accessible label for those buttons too

(By button I mean a element wrapping the <span class="img arrow"></span>. In testing, we should be able to use the keyboard to access and use the whole panel; especially Tab and Shift+Tab, and the Space and Enter key to activate a button or input.)

Regarding the spacing, it's a nice improvement but I think we can still improve it further:

• It would be nice if the label would take the full width, starting from the checkbox and up to the right edge of the panel. This can be done using flex-grow.
• We can perhaps add a tiny bit of padding on the left (padding-inline-start) of the label too, maybe just 2px, so that if a user clicks just beside the checkbox it will still be activated (fault tolerance).
• The arrow button should be bigger than 10x10. The icon itself should be 10x10 (which renders as a 8x5 triangle I believe), but clicking a 10x10 target is too hard. That button could be 16x16 or even 20x20.

On the code quality side:

• I don't think we need the
  around the (in headers)
• The
  that wraps the header (arrow button + label) should probably have a className, and we should avoid imprecise selectors like .event-listener-group div {...} which may apply to unwanted elements. A class name like event-listener-header might work.

[harshlele]

So i just put the arrow inside a button, then made that button element 20x20px, and moved the arrow's click listener to the button. I think this should solve both of these problems:

• the arrow icon should be a button so that it can be focusable; we probably need to come up with an accessible label for those buttons too

• The arrow button should be bigger than 10x10. The icon itself should be 10x10 (which renders as a 8x5 triangle I believe), but clicking a 10x10 target is too hard. That button could be 16x16 or even 20x20.

[fvsch]

Since all the classes in this file use a .event-listener-* pattern, can we use that here too?

• .event-listener-expand could work;
• .event-listener-header > button works too, but I'm partial to having a specific class, in case we want to add other buttons in the same context in the future.

Currently, selectors in Debugger stylesheets are global, so this style is publishing "btn-expand-category" as a global name that can be consumed anywhere in Debugger (see #7905). That can be a worthwhile thing to do, but if we want to do it we should create a reusable ExpandCategoryButton or something (we may do it if we end up repeating the same UI pattern and design in a few places).

[fvsch]

I think we can remove this extra <div> and just use the <label> for styling.

[fvsch]

This selector could impact too many elements, especially if we add some in the future.
Maybe .event-listener-event label span, or a new class like .event-listener-name.

[fvsch]

For spacing, I find that with a 20px arrow button we can adjust spacing like this to match the mockup:

1. Left space: 14px padding-inline-start on the container, instead of 20px
2. Space between arrow and checkbox: 2px padding-inline-start on the label, and margin: 0; margin-inline-end: 4px; on the checkbox.

Rationale:

• 20px of left space = 14px container padding + 5px button padding + 1px of extra visual space in the 10px icon (Matt's measurements are based on a 8px square icon).
• 8px of arrow-to-checkbox space = 1px of extra visual space in the arrow icon + 5px button padding + 2px of label padding

[harshlele]

• It would be nice if the label would take the full width, starting from the checkbox and up to the right edge of the panel. This can be done using flex-grow.

I'm not sure if i understood this correctly, but wouldn't this look/act weird? If the label extended all the way to the edge,it would feel like you're just randomly clicking in an empty area to right of the label text. (because the label text would always just be besides the checkbox)

[fvsch]

Sorry I thought I had replied to this earlier, but I must have forgotten to send the comment.

If the label extended all the way to the edge, it would feel like you're just randomly clicking in an empty area to right of the label text.

I guess I'm just used to this kind of UI, but if you feel that would be too strange let's not do it.
Instead, it might be good to have a bit of padding on the label itself, something like:

.event-listener-group label {
  display: block;
  padding-block: 3px;
  padding-inline-start: 2px;
  padding-inline-end: 10px;
}

So that if the user targets the end of the label text and clicks with some level of imprecision, we still register that click. (Rule of thumb for desktop UI: users will routinely click roughly 10px away from their target. And for touch interfaces, the imprecision can be much bigger, which is why Apple and Material guidelines ask for buttons that are at least 44px or 48px tall.)

[harshlele]

Hmm I added the padding to the label. Although, I got "unknown property" warnings in my editor, chrome and firefox for the padding-block property, so I haven't added that.
I've also removed the redundant div around the label. I think this is mostly complete now.

[fvsch]

@harshlele Thanks a lot. I tried the result locally and it looks great and works with keyboard navigation too. 👍

@darkwing This should be good to go.

[darkwing]

Wow, this is amazing work @harshlele ! Thank you so much!