Move the console input in-line with the logs

ASSIGNED
Assigned to

Status

()

Firefox
Developer Tools: Console
ASSIGNED
3 years ago
2 months ago

People

(Reporter: fitzgen, Assigned: Abhinav Koppula, NeedInfo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

People really don't like how it is always at the bottom, rather than at the bottom of the logs (a la a terminal emulator or whatever).

https://news.ycombinator.com/item?id=9101648
I think this is part of Irakli's plans for the console, cc'ing him. I think I agree as well, we don't gain anything by being different here. I especially notice this on the big monitor at work.

One question for Irakli: is this a big change, or is it a shallow enough to ship sooner rather than later?
Flags: needinfo?(rFobic)
i'd also like to vote for this change.

having console input at the bottom just breaks how you would expect a console to work. imagine your zsh shell console input being at the bottom. i would drive you nut. :) i think this is one those little (but important since a lot of time we're in console playing) things that annoy devs with firefox devtools.
I'm proposing that we go with the inline input behavior until console logs fill up the viewport -- after that, we pin the input field to the bottom so that it's always visible and accessible when scrolling up through the logs. This way we have the familiar behavior but don't take the step back in usability.
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 months ago
I have tried to put together a small patch for this issue which starts inline and sticks at bottom once the scrolling starts.
Let me know if it is in the right direction.

Comment 6

3 months ago
mozreview-review
Comment on attachment 8929610 [details]
Bug 1136299 - Move the console input in-line with the logs.

https://reviewboard.mozilla.org/r/200902/#review206138

This is definitely the right direction, and a very simple change! The next thing I'd want to do is make sure that when you click below the input it remains focused.

We currently do that for the output area with https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#46. Here, 'parentNode' is `#output-container` (which no longer expands to the bottom of the frame): https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.html#24

However, for that space I don't even think we want to wait for `click`, mousedown would be better (to avoid a flicker when it gets blurred and re-focused). So I think the cleanest way to support this would be the following:

1) Add <div id="jsterm-spacer"></div> as a sibling after jsterm-wrapper in the webconsole.html file.
2) In webconsole.css assign `flex: 1; min-height: 0;` to that node (so it takes up full space initially but collapses down).
3) In jsterm.js init function (https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/jsterm.js#245) add something like this:

    this.spacerNode = doc.querySelector("#jsterm-spacer");
    if (this.spacerNode) {
      // This node only exists in the HTML document
      this.spacerNode.addEventListener("mousedown", (event) => {
        this.focus();
        event.preventDefault();
      });
    }
    
Assuming that works as planned, then we should also add a new assertion in an existing test to make sure we don't regress it later. It can go at the bottom of the function in here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_input_focus.js#16. Basically something like:

    inputNode.blur();
    EventUtils.sendMouseEvent({type: "mousedown"}, hud.jsterm.spacerNode);
    ok(hasFocus(inputNode), "Focused after mousedown on spacer");
Attachment #8929610 - Flags: review?(bgrinstead)
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Flags: needinfo?(rFobic)
Duplicate of this bug: 865708
Comment hidden (mozreview-request)
(Assignee)

Comment 9

3 months ago
mozreview-review-reply
Comment on attachment 8929610 [details]
Bug 1136299 - Move the console input in-line with the logs.

https://reviewboard.mozilla.org/r/200902/#review206138

Hi Brian,
Thanks for the detailed comment.
I have updated the patch with the same.

Comment 10

3 months ago
mozreview-review
Comment on attachment 8929610 [details]
Bug 1136299 - Move the console input in-line with the logs.

https://reviewboard.mozilla.org/r/200902/#review207660

The changes look about right to me, but going to forward the review to Nicolas since I'm away the rest of the week

::: devtools/client/themes/webconsole.css:467
(Diff revision 2)
>    overflow-x: hidden;
>    /* Set padding for console input on textarea to make sure it is included in
>       scrollHeight that is used when resizing JSTerminal's input. */
>    padding: 4px 0;
>    padding-inline-start: 20px;
> +  outline: none;

What's the purpose of removing the outline?
Attachment #8929610 - Flags: review?(bgrinstead)
(Assignee)

Comment 11

3 months ago
mozreview-review-reply
Comment on attachment 8929610 [details]
Bug 1136299 - Move the console input in-line with the logs.

https://reviewboard.mozilla.org/r/200902/#review207660

> What's the purpose of removing the outline?

So if we have the outline - it just highlights the jsterm-wrapper and then it created an odd border when focused.
I mean I felt it to be a bit odd but I am fine with reverting this change as well. 
Let me know your thoughts.
Comment hidden (mozreview-request)
(Assignee)

Comment 13

3 months ago
mozreview-review-reply
Comment on attachment 8929610 [details]
Bug 1136299 - Move the console input in-line with the logs.

https://reviewboard.mozilla.org/r/200902/#review207660

Have redirected the review to Nicolas.
It occurred to me that people might want this because they don't know we always focus the jsterm, and so have to click specifically in the jsterm, which might be a longer distance.
Created attachment 8931374 [details]
Screen Shot 2017-11-23 at 15.28.17.png

The code change looks good to me, but it does look a bit odd in dark mode.
You still feel like the input is one line tall, and there is a lighter line on top of it.

I feel like we could have a better HTML for the console structure.
In Bug 1419075, we introduced a sidebar (behind a pref), and to support it, we made the output a grid.

I feel like we could re-use this grid to have all our components as siblings :


+--------------------------------+-------------+
|           filter               |             |
+--------------------------------+             |
|                                |             |
|                                |             |
|                                |             |
|                                |             |
|                                |             |
|                                |             |
|                                |             |
|                                |             |
|           messages             |  sidebar    |
|                                |             |
|                                |             |
+--------------------------------+             |
|           notification         |             |
+--------------------------------+             |
|           input                |             |
|                                |             |
|                                |             |
|                                |             |
|                                |             |
+--------------------------------+-------------+

The 2 jsterm textareas could be placed in the `input` grid area, and set to expand as much as they can, while the `messages` element could have an `auto` height.

For me that would be really great.

Now, how we can do that ?
We may try to copy the markup from https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/devtools/client/webconsole/webconsole.html#25-37 in the React init code : https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#192-200 .

With something like : 
```
      let provider = createElement(
        Provider,
        { store },
        dom.div(
          {className: "webconsole-output-wrapper"},
          filterBar,
          childComponent,
          dom.div({className: "webconsole-notificationbox"}),
          dom.textarea({className: "jsterm-complete-node devtools-monospace", tabIndex: -1}),
          dom.textarea({className: "jsterm-input-node devtools-monospace", tabIndex: 0}),
      ));
```

I am **really** not sure it would work since the jsterm is not handled by the jsterm, but would you mind trying abhinav ?
I would feel much better with a markup which would look like that.

Comment 16

3 months ago
mozreview-review
Comment on attachment 8929610 [details]
Bug 1136299 - Move the console input in-line with the logs.

https://reviewboard.mozilla.org/r/200902/#review207872
Attachment #8929610 - Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request)
Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs.

This is a "fast & dirty" patch, trying to make the console works under a grid (and it kinda does).

If we go that way, there are still things we need to figure out : 
- How we build and display the NotificationBox 
- Resizing the jsterm when entering multi-line code
- Remove elements in webconsole.html (the React app may be rendered directly in the body)
- Find a better way to integrate the jsterm input : this patch moves them in the ReactDOM.render callback. This might be enough to start, but we need to see if this give us weird things (resizing/delay when opening the console)
- scrollToBottom acts weirdly when opening the console with lots of caches messages
Attachment #8931631 - Flags: review-
Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs.

This is a "fast & dirty" patch, trying to make the console works under a grid (and it kinda does).

If we go that way, there are still things we need to figure out : 
- How we build and display the NotificationBox 
- Resizing the jsterm when entering multi-line code
- Remove elements in webconsole.html (the React app may be rendered directly in the body)
- Find a better way to integrate the jsterm input : this patch moves them in the ReactDOM.render callback. This might be enough to start, but we need to see if this give us weird things (resizing/delay when opening the console)
- scrollToBottom acts weirdly when opening the console with lots of caches messages
- jsterm autocomplete popup can be weird when there is not messages. We might want to position it "to the bottom" when we don't have enough room at the top.
Attachment #8931631 - Flags: review-
Nicolas and I chatted and decided that the initial approach here plus some small fixes would be good for initial landing, and that we can spin CSS grid layout changes into future work
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 months ago
mozreview-review
Comment on attachment 8936813 [details]
Bug 1136299 - Move the console input in-line with the logs.

https://reviewboard.mozilla.org/r/207512/#review213432

This is looking good.
I do see an issue though. Clicking below the actual input loses the focus, which is weird because I had the feeling there was a test for that.

::: devtools/client/themes/webconsole.css:460
(Diff revision 1)
>    overflow-x: hidden;
>    /* Set padding for console input on textarea to make sure it is included in
>       scrollHeight that is used when resizing JSTerminal's input. */
>    padding: 4px 0;
>    padding-inline-start: 20px;
> +  outline: none;

Should we take care of this in https://bugzilla.mozilla.org/show_bug.cgi?id=1424404 ?
Attachment #8936813 - Flags: review?(nchevobbe) → review-
Created attachment 8936868 [details]
Screen Shot 2017-12-14 at 11.12.57.png

Another issue is when the sidebar is toggled on. I know the sidebar is behind a pref now, but Mike is really close to put the objectInspector in it, and I want to advertise it so people can give us feedback.

It's not blocking this patch I think, but we should then be fast enough to switch to a grid layout to take care of this situation.
Attachment #8931631 - Attachment is obsolete: true
Attachment #8931631 - Flags: review?(nchevobbe)
Brian, do you have some time to look into this ? If not, we might postpone this work until we have the input as part of the console CSS grid
Flags: needinfo?(bgrinstead)
You need to log in before you can comment on or make changes to this bug.