1523864 - Put the filter buttons toolbar next to the filter input if there's enough space

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

Right now, the filter buttons toolbar is always displayed on its own row.
But there could be cases when there's plenty of space to have it directly after the filter input (before the "Persist log" checkbox).

This probably could be done with CSS only (grid would be a good candidate), and probably some markup changes,

Comment 1

[kelly.bell]

Hi there! I'm new to the community and looking to attempt my first bug fix. If no one is working on this, can I be assigned to give it a shot?

Thanks!

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: mockup

Hello Kelly,

Thanks for wanting to help us!
So here's a link you can follow to setup the work environment https://docs.firefox-dev.tools/getting-started/
When asked, make sure to use "Artifact builds" as it makes building Firefox much faster.

When everything is installed and ready to go, I suggest you to get familiar with the Browser Toolbox: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

This is basically a DevTools window to debug DevTools :) Here it will be helpful since we want to modify the layout of the filter bar.

I attached a mockup to the bug to better show what we want (we shouldn't care about the button colors for now, and omit the "cog" button as well).
Basically: we want the filter input to have a minimum width, and if there is enough space for the buttons, display them next to the input, before the persist log.

The console is built using React, and this is where the Filter bar is defined: devtools/client/webconsole/components/FilterBar.js.

It's then styled using devtools/client/themes/webconsole.css#510-631.

If you have any question, don't hesitate to ask here or in our Slack.

Comment 3

[kelly.bell]

Thanks for the information Nicolas! I'll get started on setting up my environment this weekend and read up on the Browser Toolbox before digging into the code.

Comment 4

[kelly.bell]

I think I do actually some follow up question about this ticket. I'm somewhat familiar with CSS, but my knowledge is limited and I'm struggling a bit to find the right approach. Essentially we have a simplified div structure as follows:

<div filterbar-wrapper
  <div filter-inputs>
    <div trash-can>
    <input filter-text-box>
    <label persist-checkbox>
  <div sometimes-visible-messages>
  <div filter-buttons>

I did some reading on CSS grid and it doesn't seem straightforward to stick the filter-button div in between the filter-text-box and persist-checkbox elements because of the nesting of the divs. It does seem pretty easy to change the grid template of the columns in the filterbar-wrapper grid to get filter-inputs and filter-buttons side by side, but this won't look like the mockup and I'm guessing that's not what we want.

I could try to play around with CSS a bit more, but another approach I've considered is to modify the FilterBar component. I could add a conditional check on the window size and either render the filter-buttons where they are now, or render them between filter-text-box and persist-checkbox when the window size exceeds some width. I'm worried this is a bit hacky though and I think it would require adding state to an otherwise "dumb" component in order to update it when the window size changes.

I'm wondering if it's better to continue trying to get this right with CSS or if modifying the React component is the better way to go?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Hello Kelly,

Thanks for writing down your work in progress, this is helpful.

I could try to play around with CSS a bit more, but another approach I've considered is to modify the FilterBar component. I could add a conditional check on the window size and either render the filter-buttons where they are now, or render them between filter-text-box and persist-checkbox when the window size exceeds some width. I'm worried this is a bit hacky though and I think it would require adding state to an otherwise "dumb" component in order to update it when the window size changes.

I have the same feeling towards doing layout using javascript: it will be a bit hacky, and might have edge case we're not thinking of. Furthermore, it would have a performance cost since we would need to add a resize event listener and re-render the component each time.
Ultimately, if we can't get something working in CSS, we could go that way, but I'd like us to experiment as much as we can to have this done in CSS.

I created a codepen with the filterbar layout so we can play with this easily (and then when we find a working solution, copying it to the console code).

I'm going to play with it for a bit this morning, and I'll report any progress (hopefully) I make there :)

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Okay, so after some tests, I wasn't able to have a grid layout that would wrap like we want using only CSS grid.
But I do have something working when using a media query.

The idea is to have a grid column template which would hold all the items in 1 row, with the filter input being 1fr (to take all the space), and the one for <div filter-buttons> set to auto (then, when we put the div in the next row, it will collapse to a 0 width).

Then, by manual testing, we can spot when things get bad, and add a media query at that point, in which we would change grid properties of <div filter-buttons>, to something like:

.webconsole-filterbar-secondary {
  grid-row: 1 / 2; // first row
  grid-column: 3 / 4; // the column after the filter input
}

@media (max-width: 820px) {
  .webconsole-filterbar-secondary {
    grid-row: -1 / -2; // last row
    grid-column: 1 / -1;
  }
}

Also, given this new design, I think we should have a different structure:

<div class="webconsole-filteringbar-wrapper" aria-live="off">
  
  <button class="devtools-button devtools-clear-icon" title="Clear the Web Console output"></button>
  <div class="devtools-separator"></div>
  <input class="devtools-plaininput text-filter" type="search" placeholder="Filter output">
  <label class="filter-checkbox">
    <input type="checkbox">Persist Logs
  </label>
  
  <div class="devtools-toolbar webconsole-filterbar-secondary">
    <button aria-pressed="true" class="devtools-button error checked">Errors</button>
    <button aria-pressed="true" class="devtools-button warn checked">Warnings</button>
    <button aria-pressed="true" class="devtools-button log checked">Logs</button>
    <button aria-pressed="true" class="devtools-button info checked">Info</button>
    <button aria-pressed="true" class="devtools-button debug checked">Debug</button>
    <div class="devtools-separator"></div>
    <button aria-pressed="false" class="devtools-button css">CSS</button>
    <button aria-pressed="false" class="devtools-button netxhr">XHR</button>
    <button aria-pressed="false" class="devtools-button net">Requests</button>
    <div class="devtools-separator"></div> <!-- NEW -->
  </div>

</div>

The <div class="devtools-separator"> in .webconsole-filterbar-secondary would then be hidden in the same media query.

How does that sound to you kelly?

Comment 7

[kelly.bell]

The codepen and your followup comments were really helpful! Thank you! I wasn't aware of display: contents; before now, but it looks like that's where a lot of the magic is happening. With a few tweaks I was able to get this working in Firefox.

One issue that came up when using display: contents; is that it removes the background and border styling of the webconsole-filterbar-primary div. I modified the .webconsole-filterbar-secondary and .webconsole-filterbar-filtered-messages divs a bit to accommodate this. I basically just decided to set the backgrounds to be transparent and set the border to 0px when those divs are in line with the webconsole-filterbar-primary div. Otherwise, they have the same styles they did before these changes. I'll add a gif of the filter bar in action as an attachment.

I liked the approach of using the media query to hide/show the separator divs, so I added that as well.

If things look ok, I think I just need to update the comment section in the CSS file that describes the Console toolbar to reflect these changes and then I'll be ready to submit the code for feedback and review.

Thanks for your support with this bug. This has been a great learning experience for me and I'm glad I learned some new things about CSS.

Comment 8

[kelly.bell]

Attachment: wip_filter_bar.gif

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Hello kelly, I'm glad I was able to help you :)

The GIF looks awesome I must say! :)
One thing that we could improve is the "X messages hidden" thing, but we'll take care of it in a follow up bug.

For the display:contents, I added it in the codepen only because I didn't want to change the markup.
But in order to fix this bug properly, I think we should get rid of the .webconsole-filterbar-primary element. We should be able to do that by removing those lines: FilterBar.js#244-247,269

Comment 10

[kelly.bell]

Ok, that makes sense. I deleted the .webconsole-filterbar-primary element and got things working again without it.

I agree about the filtered messages div, but it seemed out of scope to start styling that more than necessary.

Should I be writing tests for this? I'm currently running the devtools test suite to make sure everything still passes.

Otherwise, I'm ready to submit for code review (I guess through Phabricator?)

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Hello Kelly, thanks again for working in this.

If you're ready for code review, yes you can push to phabricator and I will have a look as soon as I can :)

Comment 12

[kelly.bell]

Attachment: Bug 1523864 Display filter buttons next to filter input if there is enough space. r=nchevobbe!

Set a minimum width on the filter bar of the Devtools Console window. When there is enough empty space
to the right of the filterbar, allow the filter buttons to be desplaying inline, rather than on their own
separate row.

Comment 13

[kelly.bell]

Attachment: Bug 1523864 Display filter buttons next to filter input if there is enough space. r=nchevobbe!

Address revision issues: Attempt a React solution and revert CSS changes

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Hello Kelly, is there anything blocking you to move forward? (it's totally fine if you simply don't have the time :) )
There are only a few comments on the last patch you posted, and maybe we could ignore some and land this initial patch and file follow-up, smaller bugs to iron it out.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1523864 - Move filter bar layout change to the App level. r=Honza.

This makes the console code more consistent, and adds the
nice benefit of being able to check if the layout should
be modified when performing non-window-resize events that
might still impact the layout (sidebar toggle, sidebar
resizing, ...).

Depends on D20188

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c8aa3c1d123
Display filter buttons next to filter input if there is enough space. r=Honza
https://hg.mozilla.org/integration/autoland/rev/89b7f73b2701
Move filter bar layout change to the App level. r=Honza.

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4c8aa3c1d123
https://hg.mozilla.org/mozilla-central/rev/89b7f73b2701