1136299 - Move the codeMirror console input in-line with the logs

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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?

Comment 2

[Rok Garbas [:garbas]]

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.

Comment 3

[Victoria Wang [:victoria]]

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 4

[Abhinav Koppula]

Attachment: Bug 1136299 - Move the console input in-line with the logs.

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

Comment 5

[Abhinav Koppula]

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

[Brian Grinstead [:bgrins]]

Comment on attachment 8929610 [details] [diff] [review]
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");

Comment 8

[Abhinav Koppula]

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

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

Comment 9

[Abhinav Koppula]

Comment on attachment 8929610 [details] [diff] [review]
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

[Brian Grinstead [:bgrins]]

Comment on attachment 8929610 [details] [diff] [review]
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?

Comment 11

[Abhinav Koppula]

Comment on attachment 8929610 [details] [diff] [review]
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 12

[Abhinav Koppula]

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

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

Comment 13

[Abhinav Koppula]

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

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

Have redirected the review to Nicolas.

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: 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

[Nicolas Chevobbe [:nchevobbe]]

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

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

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1136299 - Move the console input in-line with the logs;.

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

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 20

[Brian Grinstead [:bgrins]]

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 21

[Brian Grinstead [:bgrins]]

Attachment: Bug 1136299 - Move the console input in-line with the logs.

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

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

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

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8936813 [details] [diff] [review]
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 ?

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Attachment: 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.

Comment 25

[Nicolas Chevobbe [: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

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Let's wait for Bug 1425521 to land, then we can work on this more easily since the JSTerm will be part of the CSS grid

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Hello abhinav,

We've been rolling out a new Jsterm (with syntax highlighting), and I don't think your patch would apply now.
I'm taking over for now, but let me know if you want to tackle this.

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

I'm at a point where almost everything look fine, and the code is quite constrained.
The only thing we need to change is how we deal with the autocomplete popup.
In the current jsterm, we always show it to the top, because we have plenty of space. Which means that we reverse the autocompletion results we get from the server to pass them to the autocomplete-popup.
With an in-line input, things are more complicated. For example, if you start with an empty output, you probably want the autocomplete popup to open to the bottom, where there's more space.
Fortunately, the html tooltip, which the autocomplete-popup is based on, should handle this automatically (it checks where there is the more available space and uses that).
But this will require some work either on the autocomplete or the jsterm (to not reverse the list of items when we open the popup to the bottom, to pick the correct automatically selected index), and probably some changes in the tests as well.

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

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

Comment 31

[Nicolas Chevobbe [:nchevobbe]]

Brian, this still misses patches from Bug 1475165, but since the in-line style is only for the codeMirror jsterm, and thus behind a flag, I think it's fine to at least review it now (and maybe land, since 1475165 has patches in review).
Also, not sure if we should have a test for this ? Maybe to make sure that (filterbar + output + input) heights are not bigger than the document height in different UI configuration (filter buttons visible, hidden messages label visible, …).
I'll post another patch with the test.

Comment 32

[Nicolas Chevobbe [:nchevobbe]]

Attachment: in-line.png

Here's how it looks.
I wonder if we should keep the blue outline has it feels quite prominent when there's no messages in the output.

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #32)
> Created attachment 8992631 [details]
> in-line.png
> 
> Here's how it looks.
> I wonder if we should keep the blue outline has it feels quite prominent
> when there's no messages in the output.

I agree the outline is strong in the top-left and top-middle screenshots. Going to needinfo Victoria to get her thoughts on what to do.

Just a couple of ideas - maybe we could turn the focus style into a top border only to smooth over the difference between the top-left and top-right cases? Or maybe the blue arrow icon we already use is enough to indicate that it's focused without an outline?

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Attachment: in-line-proposals.png

here's how it could look only with the blue top border, or without any focus style, in various situations

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1136299 - Add a test for the new in-line input in the console;.

The test relies on the css custom property changes to properly wait since
the resize event is throttled. I confess this is not ideal, and we might
instead fire a "webconsole-resized" event instead.

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

Comment 36

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

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

Comment 37

[Victoria Wang [:victoria]]

Wow this is a good-looking and handy matrix of UI options - thanks Nicolas :D!

I agree that it's better to not have any blue border for no-logs/some-logs. I would even suggest removing the gray border in some-logs since it looks less like a terminal input with that border.

In the lots-of-logs scenario, it does seem important to have the gray border assuming the field will remain visible at the bottom when one scrolls up. I'm a bit stumped about the focus ring in this situation. Generally it's good for accessibility, but I feel it would look odd and distracting for this to suddenly appear when the terminal-mode transitions to a pinned input field. I would suggest we move forward with no blue focus ring, just the blue >>, and have everyone try this in Nightly to see how it feels.

It's very unique UX to transition from a formless terminal input to a pinned text field that can auto-expand to become a text area, and we should all be very proud that this is happening (if boldy assuming that it is the ideal UI, lol) :)

Comment 38

[Nicolas Chevobbe [:nchevobbe]]

Attachment: no borders

The input looks very nice without the blue border I must say, very clean.
I'd like us to keep the grey border for now. When the output is scrolled, and if you don't have borders, it looks weird, with text feeling "clipped".
We could detect when the output overflows and then add the border, but this will add a bit more complexity to the code and some computation overhead that I'd rather not have.

Comment 39

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

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

Comment 40

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

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

Comment 41

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

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

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Looks very nice!

I agree that the gray border can stay there - it looks better in case of scrolling. Note that if we separated every log by the same gray bottom-border, it would be naturally there all the time.

Honza

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

https://reviewboard.mozilla.org/r/257742/#review265050

The CSS changes here should be folded into the previous changeset, right?

::: commit-message-9a3ce:1
(Diff revision 3)
> +Bug 1136299 - Add a test for the new in-line inputt in the console;r=bgrins.

Typo: 'input'

Comment 44

[Victoria Wang [:victoria]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #38)
> I'd like us to keep the grey border for now. When the output is scrolled,
> and if you don't have borders, it looks weird, with text feeling "clipped".
> We could detect when the output overflows and then add the border, but this
> will add a bit more complexity to the code and some computation overhead
> that I'd rather not have.

Yeah, I was suggesting showing the grey border only when detecting overflow, but makes sense that it would be complicated. Sounds good to keep it for now.

Comment 45

[Brian Grinstead [:bgrins]]

How much extra work would it be to enable this behavior in the non-CM input? If we can keep the behavior as close as possible it seems less likely we'll regress just one or the other going forward.

Comment 46

[Brian Grinstead [:bgrins]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

https://reviewboard.mozilla.org/r/202800/#review265282

::: devtools/client/themes/webconsole.css:283
(Diff revision 5)
>  }
>  
>  .jsterm-input-container {
>    background-color: var(--theme-tab-toolbar-background);
>    border-top: 1px solid var(--theme-splitter-color);
> +  max-height: calc(100vh - var(--filter-bar-height));

This feels like something that should be doable in CSS without reading clientHeight at runtime and setting a variable (like by setting min-height of --min-message-height on the .webconsole-output and somehow telling the filter bar and notification container not to shrink when they have height).

I'm sure there's a good reason why that doesn't work though - I think we may have even discussed it before. Let's chat tomorrow if you have time so I can get caught up on the options here.

Comment 47

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Brian Grinstead [:bgrins] from comment #45)
> How much extra work would it be to enable this behavior in the non-CM input?
> If we can keep the behavior as close as possible it seems less likely we'll
> regress just one or the other going forward.

I'll have to check again. I think there was an issue with the way we update the height of the textarea when adding new line

Comment 48

[Nicolas Chevobbe [:nchevobbe]]

> This feels like something that should be doable in CSS without reading clientHeight at runtime and setting a variable (like by setting min-height of --min-message-height on the .webconsole-output and somehow telling the filter bar and notification container not to shrink when they have height).

I went through many CSS-only options and there was always something that wouldn't go well for the design we want.
This max-height ensure that the input does not push above elements to the top, where they would be invisible to the user.
This was "easy" to do with the current design (max-width: 90vh if I recall correctly), because we didn't need the input to take the whole vertical space.

Comment 49

[Brian Grinstead [:bgrins]]

We experimented with a few different things here based on the version you sent me today. Potch made a nice test page where you can scale up and down the number of messages and input lines: https://console-layout-test.glitch.me/. I'm not sure if it'll translate directly into m-c, but the demo page could come in handy for testing out various scenarios.

There are two caveats to this particular iteration:
1) We'd need to re-add the click handling to focus input in the area below the input (since the input has changed to `auto` instead of flexed).
2) This reorganizes the markup a bit so that there's a second grid parent (of which the filterbar is a child). I'm not sure if that's 100% necessary, but the overflow on the input seems to break without it - more investigation would be needed to remove it.

Comment 50

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Brian Grinstead [:bgrins] from comment #49)
> We experimented with a few different things here based on the version you
> sent me today. Potch made a nice test page where you can scale up and down
> the number of messages and input lines:
> https://console-layout-test.glitch.me/. I'm not sure if it'll translate
> directly into m-c, but the demo page could come in handy for testing out
> various scenarios.
> 
> There are two caveats to this particular iteration:
> 1) We'd need to re-add the click handling to focus input in the area below
> the input (since the input has changed to `auto` instead of flexed).
> 2) This reorganizes the markup a bit so that there's a second grid parent
> (of which the filterbar is a child). I'm not sure if that's 100% necessary,
> but the overflow on the input seems to break without it - more investigation
> would be needed to remove it.

I think I tried something similar at some point here. It wasn't working since we had this blue outline on every side of the input, making the input not looks like it was getting the remaining space.
But now that we have decided that we could go without the blue border, this should be doable.
I am going to try to integrate this solution in the current grid so we don't have extra elements.

Comment 51

[Brian Grinstead [:bgrins]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

https://reviewboard.mozilla.org/r/202800/#review265686

Clearing reviews for now

Comment 52

[Brian Grinstead [:bgrins]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

https://reviewboard.mozilla.org/r/257742/#review265688

Comment 53

[Brian Grinstead [:bgrins]]

I made another demo using flex-shrink / flex-grow that allows the CM input to fill up remaining space when there is no input/output: https://glitch.com/edit/#!/sulky-donkey?path=style.css:1:0 / https://sulky-donkey.glitch.me/.

Comment 54

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #53)
> I made another demo using flex-shrink / flex-grow that allows the CM input
> to fill up remaining space when there is no input/output:
> https://glitch.com/edit/#!/sulky-donkey?path=style.css:1:0 /
> https://sulky-donkey.glitch.me/.

The main thing here is the `flex-shrink: 100000;` on the output area. If we do `flex-shrink: 1` then it distributes evenly when both input and output are overflowing. And if we do `flex-shrink: 0` on the input to avoid that problem, then the input never actually overflows.

It's not exactly the same, but we do something similar with flexbox in the toolbox and other places in chrome where we put a high and low flex on siblings: https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/devtools/client/framework/toolbox.xul#57-63.

Comment 55

[Nicolas Chevobbe [:nchevobbe]]

Looks like the high flex-shrink was what I needed, things looks great with it (both with old and new jsterm)
We only needed a min-height on the input so it does not shrink (flex-shrink: 0 makes the overflow fail).
The nice thing is that I was able to put the flex-grow only on the last element of the flex-container, which makes the design ready for when we add eager-evaluation results and/or reverse search input !

I pushed my patch to TRY before cleaning everything up https://treeherder.mozilla.org/#/jobs?repo=try&revision=304cb800609d94ac3e62882aa4650d84bb8dff7c

Comment 56

[Nicolas Chevobbe [:nchevobbe]]

So, TRY only reports failure on devtools/client/webconsole/test/mochitest/browser_jsterm_input_expansion.js which makes sense.

Comment 57

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

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

Comment 58

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

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

Comment 59

[Brian Grinstead [:bgrins]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

https://reviewboard.mozilla.org/r/202800/#review267058

::: devtools/client/themes/webconsole.css:968
(Diff revision 6)
> +.webconsole-flex-wrapper .webconsole-output {
> +  flex-shrink: 100000;
> +}
> +
> +.webconsole-flex-wrapper > .webconsole-output:not(:empty) {
> +  min-height: 19px;

After playing around with this - I think I'd prefer a higher min-height. Subjectively, 30px seems reasonable.

On my local OSX build, the scrollbar doesn't even render until ~28px (may depend on the size of the thumb). I think the case where there's only one message and then we end up with a bit of whitespace between the jsterm and the output is both not super likely and not that big a deal.

::: devtools/client/themes/webconsole.css:977
(Diff revision 6)
> -  grid-column: 1 / 2;
> +  flex-shrink: 0;
> -  grid-row: 3 / 4;
>  }
>  
>  .webconsole-output-wrapper .jsterm-input-container {
> -  grid-column: 1 / 2;
> +  min-height: 28px;

This seems to lead to slightly off-center text (closer to top than to bottom) when the input gets pushed all the way to the bottom. Although I don't think it's a huge deal and could be moved to a follow-up. I'll post a screenshot.

Comment 60

[Brian Grinstead [:bgrins]]

Attachment: slightly-off-vertical-center-text.png

Screenshot referenced in Comment 59

Comment 61

[Brian Grinstead [:bgrins]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

https://reviewboard.mozilla.org/r/257742/#review267062

::: devtools/client/webconsole/test/mochitest/browser_webconsole_in_line_layout.js:12
(Diff revision 4)
> +
> +// Test that the in-line layout works as expected
> +
> +const TEST_URI = "data:text/html,<meta charset=utf8>Test in-line console layout";
> +
> +const MINIMUM_MESSAGE_HEIGHT = 19;

If you do end up changing minimum height as mentioned in Comment 59 - don't forget to change it here too. I'm OK with doing that in a follow up.

Comment 62

[Nicolas Chevobbe [:nchevobbe]]

> After playing around with this - I think I'd prefer a higher min-height. Subjectively, 30px seems reasonable.

The thing is, if you only have 1 message in the output, suddenly it looks a bit weird.

Comment 63

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202800/diff/6-7/

Comment 64

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

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

Comment 65

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

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

Comment 66

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c6f390374fdb0ec5266ee7b802d3a7cc2611f0f4 -d bb6817615317: rebasing 475354:c6f390374fdb "Bug 1136299 - Move the console input in-line with the logs;r=bgrins."
rebasing 475355:859304ad8151 "Bug 1136299 - Add a test for the new in-line input in the console;r=bgrins." (tip)
local [dest] changed devtools/client/webconsole/test/mochitest/browser_jsterm_input_expansion.js which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 67

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931631 [details]
Bug 1136299 - Move the console input in-line with the logs;.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202800/diff/7-8/

Comment 68

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8992906 [details]
Bug 1136299 - Add a test for the new in-line input in the console;.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257742/diff/6-7/

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7ac54e3457f
Move the console input in-line with the logs;r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/4fc591ddb52f
Add a test for the new in-line input in the console;r=bgrins.

Comment 70

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c7ac54e3457f
https://hg.mozilla.org/mozilla-central/rev/4fc591ddb52f

Comment 71

[Irene Smith]

Added the following information to the description of the web console UI (https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Opening_the_Web_Console):

The Web Console's interface is split into three horizontal sections:

    Toolbar: along the top is a toolbar containing two buttons. Click the garbage can button to clear the contents of the console. Click the funnel icon to filter the message that are displayed in the console

    Command Line: the command line starts with double angle brackets (>>). Use it to enter JavaScript expressions

    Message Display Pane: the messages generated by the code in the page and by the commands entered on the command line are displayed following each command

And added this to the release notes:

The command line in the Web Console is now shown immediately following the console output ({{bug(1136299)}})

Comment 72

[Nicolas Chevobbe [:nchevobbe]]

Looks good to me ! Thanks Irene