1444302 - Provide an easier way to close a split console

Status: VERIFIED FIXED

(DevTools :: Console, enhancement)

Description

[Brian Birtles (:birtles)]

Attachment: Close button suggestion

This is related to bug 1444301 but I often accidentally open the split console because I mistake the toolbar buttons at the right. Some time later I decide I want to close it but I can't find out how to close it.

Could we, perhaps, add a close button to the top right of the console itself, just next to the filter part?

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

I could be adding the close button to webconsole's FilterBar. This webconsole module is used several tools, like browser console, split console, web console(tool panel), scratch pad.
This close button will appear on these tools which are using this webconsole if we added this close button to FilerBar without thinking. We need to distinguish current web console is a split console or not.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> I could be adding the close button to webconsole's FilterBar. This
> webconsole module is used several tools, like browser console, split
> console, web console(tool panel), scratch pad.
> This close button will appear on these tools which are using this webconsole
> if we added this close button to FilerBar without thinking. We need to
> distinguish current web console is a split console or not.

This comment is a mistake. split console and web console(tool panel) is same object(frame). i.e. Toolbox changes web console height size when choosing split console or choosing web console.[1] So we can distinguish a current web console is a split console or not.

[1] https://searchfox.org/mozilla-central/rev/b80994a43e5d92c2f79160ece176127eed85dcc9/devtools/client/framework/toolbox.js#970-983

WIP: https://hg.mozilla.org/try/rev/3e462ab8dbacf33fbabdf92eb53ac98df3a5dcab

Comment 3

[Victoria Wang [:victoria]]

As we discussed in the meeting: yes, I agree that adding a close button to the right side of the split Console filter bar would be a good idea. Would be great to have it visually align with the main DevTools close button.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Thanks, Victoria,

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> > I could be adding the close button to webconsole's FilterBar. This
> > webconsole module is used several tools, like browser console, split
> > console, web console(tool panel), scratch pad.
> > This close button will appear on these tools which are using this webconsole
> > if we added this close button to FilerBar without thinking. We need to
> > distinguish current web console is a split console or not.
> 
> This comment is a mistake. split console and web console(tool panel) is same
> object(frame). i.e. Toolbox changes web console height size when choosing
> split console or choosing web console.[1] So we can distinguish a current
> web console is a split console or not.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> b80994a43e5d92c2f79160ece176127eed85dcc9/devtools/client/framework/toolbox.
> js#970-983
> 
> WIP: https://hg.mozilla.org/try/rev/3e462ab8dbacf33fbabdf92eb53ac98df3a5dcab

I updated wip patch. This patch used the "split-console" and "select" event of toolbox in order to detect opening webconsole panel and split webconsole panel:
https://hg.mozilla.org/try/rev/e96332776add3581643e5fd715badb33e63eb36a

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Added the test:
https://hg.mozilla.org/try/rev/b24e205baad38cfb62dd26a49d428ceb48c14538

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444302 - Add the close button into the split console.

This patch will display the button of closing split console. The FilterBar
should display this close button if target is split console.

Review commit: https://reviewboard.mozilla.org/r/235532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235532/

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444302 - Add the close button into the split console.

This patch will display the button of closing split console. The FilterBar
should display this close button if target is split console.

Review commit: https://reviewboard.mozilla.org/r/235540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235540/

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #7)
> Created attachment 8966862 [details]
> Bug 1444302 - Add the close button into the split console.
> 
> This patch will display the button of closing split console. The FilterBar
> should display this close button if target is split console.
> 
> Review commit: https://reviewboard.mozilla.org/r/235540/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/235540/

Nicolas.
Sorry, I modified the patch to add the undefined check of the toolbox. In the several cases(like the browser console), some tests will be failed.[1]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c24d7faa39987cbab441dae668b65dc0ecfa0a4

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8966862 [details]
Bug 1444302 - Add the close button into the split console.

https://reviewboard.mozilla.org/r/235540/#review241392

Thanks a lot of Mantaroh for the patch.
r- since I think this needs another approach to be consistent with the rest of the code.

::: devtools/client/themes/webconsole.css:697
(Diff revision 1)
>  /*
>    This element contains the different toolbars in the console
>      - primary, containing the clear messages button and the text search input.
>        It can expand as much as it need.
>      - filtered messages, containing the "X items hidden by filters" and the reset filters button.
>        It should be on the same row than the primary bar if it fits there, or on its own 100% row if it is wrapped.
>      - secondary, containing the filter buttons (Error, Warning, …).
>        It should be on its own 100% row.
>  
>    Basically here's what we can have :
>  
>    -----------------------------------------------------------------------------------------------------------
>    | Clear button - Open filter bar button - Filter Input | X items hidden by filters - Reset Filters button |
>    -----------------------------------------------------------------------------------------------------------
>    | Error - Warning - Log - Info - Debug - CSS - Network - XHR                                              |
>    -----------------------------------------------------------------------------------------------------------
>  
>    or
>  
>    ------------------------------------------------------------------------------------
>    | Clear button - Open filter bar button - Filter Input                             |
>    ------------------------------------------------------------------------------------
>    |                                X items hidden by filters  - Reset Filters button |
>    ------------------------------------------------------------------------------------
>    | Error - Warning - Log - Info - Debug - CSS - Network - XHR                       |
>    ------------------------------------------------------------------------------------
>  */

Could this be updated to take the close button into account ?

::: devtools/client/webconsole/components/FilterBar.js:56
(Diff revision 1)
> +    this.needCloseButton = this.needCloseButton.bind(this);
> +
> +    this.state = {
> +      needDisplayCloseButton: false,
> +    };
> +
> +    // We need to two event in order to detect opening the webconsole panel and
> +    // split webconsole panel.
> +    if (this.props.toolbox) {
> +      this.props.toolbox.on("split-console", this.needCloseButton);
> +      this.props.toolbox.on("select", this.needCloseButton);
> +    }

Since we use redux, we try not to have any state inside our components, only props.
This "displayCloseButton" could be put into the ui state [1] (and renamed `closeButtonVisible` for consistency).

Then, instead of passing the whole toolbox, we would only pass an `onCloseButtonClick` prop which we would add as the click handler on the close button.

Now, we already have an event listener on "select" [2], and we should also add the "split-console" one there. From the listeners callback, we would call a `dispatchCloseButtonToggle` like we do in [3], with `toolbox && toolbox.currentToolId !== "webconsole"` as an argument.

Then `dispatchCloseButtonToggle` should dispatch a redux action (like in [4]), that we would react to in order to set the `closeButtonVisible` property to true/false (like in [5]).


I hope all of this makes sense. Don't hesitate to ping me if it does not.

---

[1] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/reducers/ui.js#23-31
[2] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/new-webconsole.js#233-235
[3] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/new-webconsole.js#276
[4] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/new-console-output-wrapper.js#348
[5] https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/devtools/client/webconsole/reducers/ui.js#35-36

::: devtools/client/webconsole/test/mochitest/browser_webconsole_split_close_button.js:1
(Diff revision 1)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */

nit: I tend to omit those lines now

::: devtools/client/webconsole/test/mochitest/browser_webconsole_split_close_button.js:22
(Diff revision 1)
> +  ok(closeButton, "The split console has close button.");
> +
> +  info("Close the split console by using close button");

between those 2 calls, would it make sense to close the toolbox, re-open it, and assert that: 
- the split console id displayed
- it has a close button

Then, switch to the console panel and assert the close button isn't displayed anymore.

Then, select the inspector again and make sure the close button is displayed.

::: devtools/client/webconsole/test/mochitest/browser_webconsole_split_close_button.js:43
(Diff revision 1)
> +  ok(!closeButton, "The web console panel doesn't have a close button.");
> +});
> +
> +function getCloseButton(toolbox) {
> +  let hud = toolbox.getPanel("webconsole").hud;
> +  return hud.ui.outputNode.querySelector("#split-console-close-button");

we could also use `hud.ui.outputNode.getElementById("split-console-close-button")`

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: close button with hidden items

So this is with the patch applied, when errors are filtered off but some are displayed. I think the close button should always be at the top-right corner, so we might want to check if other elements are visible as well to place it where we want.

STR to have something similar: 
1. Go to `data:text/html,<meta charset=utf8>error<script>a.dfsdf.sdfsdf</script>`
2. Open the inspector
3. Toggle the split console
4. Toggle the "Errors" filter

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Thanks, Nicolas.

I updated this patch by using redux:
https://hg.mozilla.org/try/rev/e5671c97d728c4c3aa247b327a1a6a55ab9b0c85

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
> Created attachment 8966986 [details]
> close button with hidden items
> 
> So this is with the patch applied, when errors are filtered off but some are
> displayed. I think the close button should always be at the top-right
> corner, so we might want to check if other elements are visible as well to
> place it where we want.
> 
> STR to have something similar: 
> 1. Go to `data:text/html,<meta
> charset=utf8>error<script>a.dfsdf.sdfsdf</script>`
> 2. Open the inspector
> 3. Toggle the split console
> 4. Toggle the "Errors" filter

I have a question about this before requesting the review. I think that I need to consider this problem. This wrapper style is flexbox as follow:

-------------------------------------------------------------------------
| Primary(e.g. filtering text input) | filtered-message  | close button |
-------------------------------------------------------------------------
| secondary (e.g. filtering button.)                                    |
-------------------------------------------------------------------------

The problem is that this flexbox width is narrow. If the width is narrow, filtered-message box and the close button will be in the second row as follow:

-------------------------------------------------------------------------
| Primary(e.g. filtering text input)[0]                                 | 
-------------------------------------------------------------------------
|                                filtered-message[1]  | close button[2] | 
-------------------------------------------------------------------------
| secondary (e.g. filtering button.) [3]                                |
-------------------------------------------------------------------------
(Note: [] number is the order of flexbox)

I thought that resolution is to add the media query like max-width:800px, then change these element order. i.e. The filtered message element will be on the second line. And the width of the filtered-message element should be 100% in this case.

-------------------------------------------------------------------------
| Primary(e.g. filtering text input)[0]              | close button [1] | 
-------------------------------------------------------------------------
|                                                   filtered-message[2] | 
-------------------------------------------------------------------------
| secondary (e.g. filtering button.) [3]                                |
-------------------------------------------------------------------------
(Note: [] number is order of flexbox)

However, the close button might be in second line if the filtered-message is too long message (e.g. "1000000000 item hidden by filters" or arabic language). So I need to adjust this media query size.

What do you think about this implementation?

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Having a media query for this feels a bit arbitrary.
I poked around at a CSS grid layout to see if that would be possible. but I am not familiar enough with the wrapping mechanism and couldn't find a way to make the filter-message element wrap to its own row when needed.
If we can't find something better, the media query solution might work, but I'd rather have something less brittle.

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: move-filtered-message-to-second-line.mp4

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> Having a media query for this feels a bit arbitrary.
> I poked around at a CSS grid layout to see if that would be possible. but I
> am not familiar enough with the wrapping mechanism and couldn't find a way
> to make the filter-message element wrap to its own row when needed.
> If we can't find something better, the media query solution might work, but
> I'd rather have something less brittle.

Sorry for my late reply. I tried several implement ways(reordering flex container/media query..), but to put the close button on the right of filtered message element which is resizable width might difficult without the hack. (e.g. detect the resizing the window height, then change the order of flex containers.) This might bring performance issue I think.

So I'd like to move the filtered message element to the second line. This attachment is WIP behavior for it.

What do you think about this change?

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

 > So I'd like to move the filtered message element to the second line. This
> attachment is WIP behavior for it.
> 
> What do you think about this change?

Hello,
So that looks clean but it bring me 2 questions a user might ask: 
- why this message doesn't show up in the upper bar since I have plenty of space for it (it devtools docked to bottom on a large monitor)
- if we show this message on the same line as the filters, shouldn't we directly show what the filters are ?

These are genuine question, I'm not sure what the answer should be.

That being said, I'd still prefer a way to keep current behavior where we wrap when it's needed.

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
>  > So I'd like to move the filtered message element to the second line. This
> > attachment is WIP behavior for it.
> > 
> > What do you think about this change?
> 
> Hello,
> So that looks clean but it bring me 2 questions a user might ask: 
> - why this message doesn't show up in the upper bar since I have plenty of
> space for it (it devtools docked to bottom on a large monitor)
> - if we show this message on the same line as the filters, shouldn't we
> directly show what the filters are ?
> 
> These are genuine question, I'm not sure what the answer should be.
> 
> That being said, I'd still prefer a way to keep current behavior where we
> wrap when it's needed.

Thanks, Nicolas!

I guess so. We might need to put the filtered message to the same line of the primary element, especially the case of bottom dock.
I tried the Style+JavaScript. i.e. using by resize handler + requestIdleCallback:
https://hg.mozilla.org/try/rev/8b973a3c1d5e9ace339f45eaf0b51f7d18b4421a

Before working on this, I want to ask about a second option to Jen.

Jen,
I want to add the close button to the split console, however, this the position of this close button will be changed as follows: (like comment 11)

 * If the devtool width is bigger than fitting into one line, the close button will be right of the filtered message element.
 * If the devtool width is narrow, the close button will be right on the primary element.

I tried this behavior by using the only stylesheet, however, I'm not sure the implementation method by using stylesheet only, Do you any idea of implementing this by using stylesheet only?

I created a minimum case:
https://codepen.io/mantaroh/pen/gzppOj

Comment 16

[Jen Simmons [:jensimmons]]

To do this in all CSS, you do need a media query of some sort to trigger a change in code. You could use Flexbox, as you are, and switch the order of the content using `order` (instead of using Javascript to change the order in the HTML). Do think about the implications for someone using a keyboard, however. Will the order tabbing through still make sense?

Reading your code made we want to switch it all over to CSS Grid. Like this: https://codepen.io/jensimmons/pen/GdqmLM?editors=1100

Comment 17

[Jens]

I come here to report this :) FWIW, one aspect worth mentioning: Now with bug 1444301 being merged (`console split` button being hidden in an `...` menu) it is even harder to know how to close a split console. Imagine someone hitting `ESC` accidentically. 

Also this bug is `parity-chrome` since the feature is very similar to Chrome's (or Firebug's) except for the console-split's close button missing in Firefox.

Due to bug 1444301 making it very hard to find the way the split-console can be closed, I think this bug (once fixed) should be uplifted.

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Thanks jens, Jen,

I will use media query to fix this bug, and modify the style by using grid layout as well.
If we use media query, we will need to know the width size of fitting all elements into one line, however, the size of each element is based on locale string length. So I will define the 'min-width' each element, this attribute value is calculated based on greek locale string since greek locale string is longer than other locale string.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Following is WIP patches by using media query:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ac17fec6069245d744a25b37cd303fc967573a5

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #18)
> Thanks jens, Jen,
> 
> I will use media query to fix this bug, and modify the style by using grid
> layout as well.
> If we use media query, we will need to know the width size of fitting all
> elements into one line, however, the size of each element is based on locale
> string length. So I will define the 'min-width' each element, this attribute
> value is calculated based on greek locale string since greek locale string
> is longer than other locale string.

I tried that applying the grid layout. It might make this split console implementation to complicate a bit. Because "filtered message" element will be displayed when actually toolbar has filtered message, in that case, we should change "grid-area" and "grid-template-column" attribute. The following patch is an experimental implementation:

https://hg.mozilla.org/try/rev/d2c80caa608582db22db874b16e85220e07b0276

I decided that this bug will use flex layout since I'd like to uplift, and I will change toolbar by using grid layout as another bug.

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444302 - Part 1. Add the close button into the split console.

This patch will display the button of closing split console. The FilterBar
should display this close button if target is split console.

Review commit: https://reviewboard.mozilla.org/r/243664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243664/

Comment 22

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

Previous part 1 patch will display close button on the right of a primary
element of filter bar. However, a filtered message wrapped element will display
on the right of a close button if there are filtered message actually.

So this patch will reorder the filtered message's order if devtools width is
narrow. And each wrapped element (i.e. primary / filtered message / close)
has 'min-width' property in order to trigger reorder by using a media query.

This 'min-width' is calculated based on greek locale(i.e. 'el') since locale
string of a filtered message of greek is longer than other locale string.

Review commit: https://reviewboard.mozilla.org/r/243666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243666/

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

https://reviewboard.mozilla.org/r/243664/#review249504

Thanks mantaroh ! This looks good to me, I only have a few nits about naming some things :)

::: devtools/client/webconsole/actions/ui.js:21
(Diff revision 1)
>    PREFS,
>    SELECT_NETWORK_MESSAGE_TAB,
>    SIDEBAR_CLOSE,
>    SHOW_OBJECT_IN_SIDEBAR,
>    TIMESTAMPS_TOGGLE,
> +  UPDATE_SPLIT_CONSOLE_BUTTON,

Do you think we could be more consistent with the other constants names and rename this to SPLIT_CONSOLE_CLOSE_BUTTON_TOGGLE ? or something similar ?

::: devtools/client/webconsole/actions/ui.js:70
(Diff revision 1)
>    return {
>      type: SIDEBAR_CLOSE,
>    };
>  }
>  
> +function updateSplitConsoleCloseButton(shouldDisplayButton) {

if we rename the constant, I think we should rename the function name as well

::: devtools/client/webconsole/components/FilterBar.js:37
(Diff revision 1)
>        filterBarVisible: PropTypes.bool.isRequired,
>        persistLogs: PropTypes.bool.isRequired,
>        hidePersistLogsCheckbox: PropTypes.bool.isRequired,
>        filteredMessagesCount: PropTypes.object.isRequired,
> +      closeButtonVisible: PropTypes.bool,
> +      closeSplitConsoleHandler: PropTypes.func,

could we rename this to `toggleCloseButtonVisibility` so we are more consistent with the name of the other functions ?

::: devtools/client/webconsole/components/FilterBar.js:279
(Diff revision 1)
>  
>      if (filteredMessagesCount.global > 0) {
>        children.push(this.renderFilteredMessagesBar());
>      }
>  
> +    if (this.props.closeButtonVisible) {

nit: could we extract `closeButtonVisible` like the other props at the beginning of the render function ?

::: devtools/client/webconsole/new-console-output-wrapper.js:365
(Diff revision 1)
>  
>    dispatchSidebarClose: function() {
>      store.dispatch(actions.sidebarClose());
>    },
>  
> +  dispatchUpdateSplitConsoleCloseButton: function() {

we may update the name of the function if we update the name of the action and of the constant

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

https://reviewboard.mozilla.org/r/243666/#review249506

Thanks mantaroh.
I have a few question on why we need some of these numbers, and also, we should document why they are needed in the file.

This patch does not seem to work well when the console is docked to the right: http://prntscr.com/jhklpy The close button is on the second line, and the "Persist logs" checkbox get cut-off if I narrow down the devtools (I guess because of the min-widths).

This is why the grid solution felt more appropriate to me. I a CSS-only solution is too complex/impossible to do in order to deal with the "hidden messages label", we can have some help from the javascript. We could add a css class on the filterbar element when we do show the label (see https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/devtools/client/webconsole/components/FilterBar.js#268), so we can target it in the CSS.

What do you think of this ?

::: devtools/client/themes/webconsole.css:546
(Diff revision 1)
>    /* We want the toolbar (which contain the text search input) to fit
>    the content, we don't allow to shrink/overlap it */
>    flex: 100 0 -moz-fit-content;
>    align-items: center;
>    min-height: var(--primary-toolbar-height);
> +  min-width: 520px;

can we add a comment here to explain this width ?

::: devtools/client/themes/webconsole.css:560
(Diff revision 1)
> +  order: 5;
>  }
>  
>  .split-console-close-button-wrapper {
> -    min-height: var(--primary-toolbar-height);
> +  min-height: var(--primary-toolbar-height);
> +  min-width: 34px;

this seems a bit wide for a button ? If this is needed, add a comment to tell why it is so wide

::: devtools/client/themes/webconsole.css:599
(Diff revision 1)
>    color: var(--theme-comment);
>    text-align: end;
>    align-items: center;
>    min-height: var(--primary-toolbar-height);
>    line-height: var(--primary-toolbar-height);
> +  min-width:412px;

why do we need this ?

::: devtools/client/themes/webconsole.css:602
(Diff revision 1)
> +@media (min-width: 966px) {
> +  .webconsole-filterbar-filtered-messages {
> +    order: 2;
> +  }
> +}
> +
> +@media (max-width: 965px) {
> +  .webconsole-filterbar-filtered-messages {
> +    order: 4;
> +  }
>  }

add a comment to explain why we need this media query

Comment 25

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243664/diff/1-2/

Comment 26

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243666/diff/1-2/

Comment 27

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

https://reviewboard.mozilla.org/r/243664/#review249504

Thank you for review!

> could we rename this to `toggleCloseButtonVisibility` so we are more consistent with the name of the other functions ?

OK. I renamed to dispatchCloseSplitConsole based on another function name.

Comment 28

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

https://reviewboard.mozilla.org/r/243666/#review249506

Umm, as you said, If I try the flex solution, the close button will be displayed in new next line.
OK. I'll use the grid layout.

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

https://reviewboard.mozilla.org/r/243666/#review249886

Hello Mantaroh,
I have some concerns with this patch that uses fixed width, which looks a bit brittle.
I tried to recreate the desired behaviour in codepen and here it is: https://codepen.io/nchevobbe/pen/xjJMad?editors=0100 . You can resize it to see the label move accordingly to the screen width.
We still rely on a media-query to break the "hidden messages label" into its own row (i would have liked to be able to automatically wrap but that doesn't seem possible).

```css
.webconsole-filteringbar-wrapper {
  display: grid;
  grid-template-columns: 1fr auto auto;
}

#close {
  /* Always on first row, last column */
  grid-column: -1 / -2;
  grid-row: 1 / 2;
}

.webconsole-filterbar-secondary {
  grid-column: 1 / -1;
}

@media (max-width: 965px) {
  /* Move the label to a new row, make it take the whole width and align it to the right */
  .hidden-label-visible .webconsole-filterbar-filtered-messages {
    grid-row: 2 / 3;
    grid-column: 1 / -1;
    justify-self: end;
  }
}
```

What do you think of this ?

::: devtools/client/themes/webconsole.css
(Diff revisions 1 - 2)
>    | Error - Warning - Log - Info - Debug - CSS - Network - XHR                                      |
>    ---------------------------------------------------------------------------------------------------
>  */
>  .webconsole-filteringbar-wrapper {
> -  display: flex;
> +  display: grid;
> -  grid-row: 1 / 2;

we still need this for the parent grid layout I think (it still work because it's implicit, but I'd like us to be very explicit)

::: devtools/client/themes/webconsole.css:545
(Diff revisions 1 - 2)
> +  /*  Primary element width is fixed since we need to display the close button
> +   in right-top always. */
>    min-width: 520px;

We shouldn't need this. The grid layout should be flexible enough to take care of this

::: devtools/client/themes/webconsole.css:601
(Diff revisions 1 - 2)
> +  /* A filtered message element width is fixed since we need to display the
> +   close button in right-top always. */
>    min-width:412px;

Let's try to remove this width

::: devtools/client/themes/webconsole.css:612
(Diff revisions 1 - 2)
> +  This close button position will be changed by the existence of filtered message
> +  element and the size of toolbox width. */
> +
>  @media (min-width: 966px) {
> +  .webconsole-filteringbar-wrapper-without-filtered-message {
> +    grid-template-columns: minmax(520px, auto) 34px;

i think `1fr auto` should work.
If we are not in the split console, we don't want to have a space for the close button. `auto` will then collapse if we don't show it

::: devtools/client/themes/webconsole.css:615
(Diff revisions 1 - 2)
>  @media (min-width: 966px) {
> +  .webconsole-filteringbar-wrapper-without-filtered-message {
> +    grid-template-columns: minmax(520px, auto) 34px;
> +  }
> +  .webconsole-filteringbar-wrapper-with-filtered-message {
> +    grid-template-columns: minmax(520px, auto) minmax(412px, auto) 34px;

same here, `1fr auto auto` should work
We don't want to have fixed width at all

::: devtools/client/themes/webconsole.css:617
(Diff revisions 1 - 2)
>    .webconsole-filterbar-filtered-messages {
> -    order: 2;
> +    grid-area: 1 / 2 / 3 / 3;
> +  }
> +  .webconsole-filterbar-secondary-with-filtered-message {
> +    grid-area: 2 / 1 / 3 / 4;
> +  }
> +  .webconsole-filterbar-secondary-without-filtered-message {
> +    grid-area: 2 / 1 / 3 / 3;
>    }

grid-area is a bit hard to read imo, could we use grid-column and grid-row ?

::: devtools/client/webconsole/components/FilterBar.js:140
(Diff revision 2)
> -      className: "devtools-toolbar webconsole-filterbar-secondary",
> +      className: ["devtools-toolbar webconsole-filterbar-secondary",
> +                  filteredMessagesCount.global > 0
> +                  ? "webconsole-filterbar-secondary-with-filtered-message"
> +                  : "webconsole-filterbar-secondary-without-filtered-message"].join(" "),

i don't think we need this, the class on webconsole-filteringbar-wrapper should be enough

::: devtools/client/webconsole/components/FilterBar.js:310
(Diff revision 2)
> -        className: "webconsole-filteringbar-wrapper",
> +        className: ["webconsole-filteringbar-wrapper",
> +                    filteredMessagesCount.global > 0
> +                    ? "webconsole-filteringbar-wrapper-with-filtered-message"
> +                    : "webconsole-filteringbar-wrapper-without-filtered-message"].join(" "),

let's only have an additional classname when needed: 

```js
const classnames = ["webconsole-filteringbar-wrapper"];
if (filteredMessagesCount.global > 0) {
  classnames.push("filtered-message-visible");
}

return (
  dom.div({
    className: classnames.join(" "),
    ...
```

Comment 30

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243664/diff/2-3/

Comment 31

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243666/diff/2-3/

Comment 32

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243666/diff/3-4/

Comment 33

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

https://reviewboard.mozilla.org/r/243666/#review249886

Thank you so much!

Yes!
Your suggestion is too reasonable and I'd like to apply this styles.

> let's only have an additional classname when needed: 
> 
> ```js
> const classnames = ["webconsole-filteringbar-wrapper"];
> if (filteredMessagesCount.global > 0) {
>   classnames.push("filtered-message-visible");
> }
> 
> return (
>   dom.div({
>     className: classnames.join(" "),
>     ...
> ```

If I will apply the grid-row and grid-column to filtered message element, I think that this javascript code is no longer needed.

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

https://reviewboard.mozilla.org/r/243666/#review250950

Thanks a lot Mantaroh  for going through all those iterations. This is working well now, and even though I find that we put the hidden label in a new line too early for english locale, we need to accomodate for everyone (and I'd rather have this than overlapping issues)

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

https://reviewboard.mozilla.org/r/243664/#review250952

::: devtools/client/webconsole/new-console-output-wrapper.js:216
(Diff revision 3)
>        const app = App({
>          attachRefToHud,
>          serviceContainer,
>          hud,
>          onFirstMeaningfulPaint: resolve,
> +        dispatchCloseSplitConsole: this.closeSplitConsole.bind(this),

nit: it looks like we don't dispatch anything, I'm sorry if I made you make this change, but I think having it as `closeSplitConsole` makes more sense

Comment 36

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243664/diff/3-4/

Comment 37

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243666/diff/4-5/

Comment 38

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

https://reviewboard.mozilla.org/r/243664/#review250952

> nit: it looks like we don't dispatch anything, I'm sorry if I made you make this change, but I think having it as `closeSplitConsole` makes more sense

Thanks!
I addressed this.

Comment 39

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

https://reviewboard.mozilla.org/r/243666/#review250950

Many thanks, Nicolas.

I think this width will need to be changed by locale or zoom or anything else. I'll file as another bug.

Tried these patchs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f58913440c1f17bc8290f0b28c2faff0e26ad176

After passing these test, I'll land this.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7ebe102efa2
Part 1. Add the close button into the split console. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2af84ea1c49d
Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow. r=nchevobbe

Comment 41

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b7ebe102efa2
https://hg.mozilla.org/mozilla-central/rev/2af84ea1c49d

Comment 42

[Brian Birtles (:birtles)]

Comment 17 suggests this should be uplifted. I think we should apply for that together with bug 1463355 and bug 1464265 once it lands. I don't think bug 1463357 needs to be uplifted (once it's fixed).

Comment 43

[Brian Birtles (:birtles)]

Comment on attachment 8975402 [details]
Bug 1444302 - Part 1. Add the close button into the split console.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1444301
[User impact if declined]: Less intuitive to know how to close the split console. Comment 17 suggests some users find this confusing.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: bug 1463355 and bug 1464265
[Is the change risky?]: Not overly.
[Why is the change risky/not risky?]: It's front-end code only, has automated tests, has baked in Nightly for a week, doesn't contain any particularly complex logic.
[String changes made/needed]: None.

Comment 44

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444302 - Add the close button into the split console.

Thanks brian.

We need to rebase first patch since bug 1425538 doesn't uplift.
This attachment is rebased it.

Approval comment is same to comment 43.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Please request approval on the other two bugs that also need to be uplifted.

Comment 46

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8975403 [details]
Bug 1444302 - Part 2. Make the filtered message element of the split console to be displayed in a new line when devtool width is narrow.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45)
> Please request approval on the other two bugs that also need to be uplifted.

I requested approval for other two bugs.

I added 'beta-approval' flag to the second patch since the requested two bugs need to this changes.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45)
> Please request approval on the other two bugs that also need to be uplifted.

I requested approval for other two bugs.

I added 'beta-approval' flag to the second patch since the requested two bugs need to this changes.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8981295 [details] [diff] [review]
Bug 1444302 - Add the close button into the split console.

Fixes some UX issues around closing the split console pane. Covered by automated tests. Approved for 61.0b10.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dbfb05cdda81
https://hg.mozilla.org/releases/mozilla-beta/rev/3bb86bbf74af

Comment 49

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 60.0a1 (2018.03.08) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b10 and 62.0a1 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.2.

Comment 50

[Irene Smith]

Added a mention of the close button to the split console documentation page. I also added a mention of the button to the release notes.

Spit console page: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Split_console
Release notes: https://developer.mozilla.org/en-US/Firefox/Releases/62

Comment 51

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Irene Smith from comment #50)
> Added a mention of the close button to the split console documentation page.
> I also added a mention of the button to the release notes.
> 
> Spit console page:
> https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Split_console
> Release notes: https://developer.mozilla.org/en-US/Firefox/Releases/62

Thanks Irene!

I confirmed the split console article, it looks good to me. Thanks!